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

Add a new constructor and an accessor to Upgrade #105

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

fasterthanlime
Copy link

Upgrading to HTTP/2.0 is common enough that I feel it should get its own
constructor.

Because the client can send values other than "websocket" as a request
header, I feel like having a getter for the inner value is important.

Upgrading to HTTP/2.0 is common enough that I feel it should get its own
constructor.

Because the client can send values other than "websocket" as a request
header, I feel like having a getter for the inner value is important.
Copy link
Member

@seanmonstar seanmonstar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Upgrading to HTTP/2.0 is common enough that I feel it should get its own
constructor.

The mechanism using an HTTP header is quite rare, now. All browsers stopped using it, as there are too many servers in the wild that freak out when they see an Upgrade header they don't understand. It's mostly a dead feature now. The world either users ALPN, or just "prior knowledge".

pub fn http2() -> Upgrade {
// must be uppercase, see
// <https://datatracker.ietf.org/doc/html/rfc7230#section-2.6>
Upgrade(HeaderValue::from_static("HTTP/2.0"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The spec says the header value would be h2c.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I'm not sure we need this, it's not really used.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood - I added an h2c constructor for that case and noted that ALPN/prior knowledge are overwhelmingly more common for the "http/2 over TLS" case.

/// Returns the header value, e.g. "websocket" or "HTTP/2.0", or a
/// comma-separated list of protocols, see RFC 7230:
/// <https://datatracker.ietf.org/doc/html/rfc7230#section-6.7>
pub fn value(&self) -> &HeaderValue {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of the typed headers don't hold onto a HeaderValue internally, we might change it here too. So I wouldn't want to return a reference to that type specifically...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. For now I changed it to return an Option<&str> instead.

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.

2 participants