-
Notifications
You must be signed in to change notification settings - Fork 43
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
Optional http/1.0 support #170
Conversation
1cc1a42
to
d35dacc
Compare
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 looks good!
pub fn with_default_host(mut self, default_host: &str) -> Self { | ||
self.default_host = Some(default_host.into()); | ||
self | ||
} |
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.
Side note: I'm increasingly convinced that async-h1 could be nicer to use if we had concrete Client
and Server
structs, and not the intermediate short-hand constructors. Rather than using a separate Options
struct, exposing the host
param through the constructor may be easier:
let mut server = Server::new();
assert_eq!(server.host(), None);
let mut server = Server::with_host("website.com");
assert_eq!(server.host(), Some("website.com"));
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.
Entirely agreed
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.
That's been my intent for a few PRs — I've been leaving the functions around until the interface for the structs feels right. My hunch is that implementing future for the struct will simplify a bunch of things
/// Sets the default http 1.0 host for this server. If no host | ||
/// header is provided on an http/1.0 request, this host will be | ||
/// used to construct the Request Url. | ||
/// | ||
/// If this is not provided, the server will respond to all | ||
/// http/1.0 requests with status `505 http version not | ||
/// supported`, whether or not a host header is provided. |
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.
💯 -- I really like how this works! This can very naturally translate to a CLI flag which sets the default URL, or a config which mandates the hostname is set. This feels exactly right.
this pr adds support for http/1.0, enabled by the
accept_with_opts
api caller providing a default host in ServerOptions. http frameworks like tide could surface this as a configuration option to the end userThere may be further work that needs to be done in order to completely support 1.0, but this addresses the biggest blocker and enables us to explore which of the other differences need to be handled at the async-h1 layer
Useful Reference: http://www.ra.ethz.ch/cdstore/www8/data/2136/pdf/pd1.pdf
closes #152
closes #122