Skip to content
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

Merged
merged 2 commits into from
Feb 11, 2018
Merged

Replace rouille by hyper #53

merged 2 commits into from
Feb 11, 2018

Conversation

BenoitZugmeyer
Copy link
Contributor

@BenoitZugmeyer BenoitZugmeyer commented Jan 21, 2018

Following #49 discussion, here is a first PR to support auto-reload with WebSockets. It only replaces rouille by hyper. I tried to keep the same logic as much as possible.

I initially wanted to use hyper-staticfile, but there is some issues:

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.

src/build.rs Outdated
@@ -143,6 +145,8 @@ impl BuildArgs {
None
};

let auto_reload = matches.is_present("auto-reload");
Copy link
Owner

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. (:

StaticDirectory( PathBuf )
}

struct Route {
key: String,
Copy link
Owner

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)
))
}
Copy link
Owner

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;
Copy link
Owner

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)",
]

Copy link
Owner

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.

@koute
Copy link
Owner

koute commented Jan 22, 2018

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?

That's something I was planning to do since quite some time, however the problem is that (last time I've checked) rustfmt isn't configurable enough to precisely support my style, which means before I can do that I'd have to contribute to rustfmt first adding the configurability I'd need. So for now I just let everyone use whatever style they want with the intent to clean it up in the future.

because it resolves the file location itself, rethinking the whole cargo-web artifacts logic would be too much changes for this PR.

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 deploy subcommand etc.

@BenoitZugmeyer
Copy link
Contributor Author

Ok I did most of what you asked. The only thing I couldn't do is creating an abstraction on hyper types, to be as simple as rouille. I almost got it working, but I'm blocked in the test_chromium module, I cannot figure out how to retrieve the bound port. You can look at it here: master...BenoitZugmeyer:hyper_simple

@koute
Copy link
Owner

koute commented Jan 29, 2018

Hmm... can't you just move the Http::new().bind( address, move || Ok( SimpleService { handler: &handler } ) ) part to new (which would make the API just like in rouille)? Then save the Server in Self, and expose local_addr.

@BenoitZugmeyer
Copy link
Contributor Author

I'm still working on it. Storing the hyper::Server would be nice but because of the closure, I can't find a way to express the type of this value. I think the impl Trait feature would help, but that's not in rust stable yet.

@koute
Copy link
Owner

koute commented Feb 4, 2018

Can't you Box it?

@BenoitZugmeyer
Copy link
Contributor Author

PR updated, with boxed closures! The diff is much smaller, so I made a single commit.

@koute
Copy link
Owner

koute commented Feb 8, 2018

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?

@BenoitZugmeyer
Copy link
Contributor Author

Done!

@koute
Copy link
Owner

koute commented Feb 11, 2018

Awesome! Thanks a lot!

@koute koute merged commit 096e75e into koute:master Feb 11, 2018
@BenoitZugmeyer BenoitZugmeyer deleted the hyper branch February 25, 2018 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants