-
Notifications
You must be signed in to change notification settings - Fork 80
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Replace rouille by hyper #53
Conversation
src/build.rs
Outdated
@@ -143,6 +145,8 @@ impl BuildArgs { | |||
None | |||
}; | |||
|
|||
let auto_reload = matches.is_present("auto-reload"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't belong here; BuildArgs
is for shared arguments related to building, so please move it back. (:
src/deployment.rs
Outdated
StaticDirectory( PathBuf ) | ||
} | ||
|
||
struct Route { | ||
key: String, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now the StaticDirectory
handling doesn't indeed use the key
, but that will change in the future once I'll make potential static directory paths configurable, so please revert this change.
src/utils.rs
Outdated
sync_response_from_data("text/plain", format!("{}", status).into_bytes()) | ||
.with_status(status) | ||
)) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you move those http server related utils to a separate file?
src/cmd_start.rs
Outdated
type Response = Response; | ||
type Error = hyper::Error; | ||
|
||
type Future = FutureResponse; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you perhaps wrap the hyper
-related stuff in an simple interface similar to rouille
, put that in a separate file (along with the stuff you've added to utils.rs
), and then use it here? I really like how simple rouille
is and how straightforward its interface is, so I'd love to keep it if possible. (Also, if we decide to switch to something else in the future or refactor how the HTTP server itself works having only one place where it's all located would be nice.)
Cargo.lock
Outdated
dependencies = [ | ||
"memchr 0.1.11 (registry+https://github.com/rust-lang/crates.io-index)", | ||
] | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove the Cargo.lock
changes. I always update it in a separate commit before bumping up the version.
That's something I was planning to do since quite some time, however the problem is that (last time I've checked)
That's something I wouldn't want to do anyway (if you look in the commit history it actually used to be that the server itself searched for the artifacts!) - I want the embedded server to act as a thin layer serving the file blobs, and pretty much do nothing else. Locating files, reading them, and all of the routing I want to do myself, mostly to be able to reuse as much logic as possible for |
7c883cb
to
b8d0911
Compare
Ok I did most of what you asked. The only thing I couldn't do is creating an abstraction on |
Hmm... can't you just move the |
I'm still working on it. Storing the |
Can't you |
PR updated, with boxed closures! The diff is much smaller, so I made a single commit. |
This looks great now! Thanks a lot! One last thing I'd like to request before I merge it - could you add cache control headers like those here? |
25678f3
to
cebf10c
Compare
Done! |
Awesome! Thanks a lot! |
Following #49 discussion, here is a first PR to support
auto-reload
with WebSockets. It only replacesrouille
byhyper
. I tried to keep the same logic as much as possible.I initially wanted to use
hyper-staticfile
, but there is some issues:futures
crate (because of Consider removing.boxed()
combinators and theBox
aliases rust-lang/futures-rs#228 )cargo-web
artifacts logic would be too much changes for this PR.Instead, I based the file streaming on the Hyper guide. But still, it's a project to consider.
Note: you are using a different code style than mine (especially on spaces inside brackets and parens). Would you mind commiting a rustfmt configuration file reflecting your tastes so it'd be easier for other folks to contribute? See this configuration value in particular.