-
Notifications
You must be signed in to change notification settings - Fork 82
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
New HTTP/2 currency types. #44
Conversation
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 really good! A few nits
Sources/NIOHTTP2/HTTP2Frame.swift
Outdated
/// A single HTTP/2 frame, along with the state change it triggered in the state machine. | ||
public struct HTTP2FrameWithMetadata { | ||
/// The received HTTP/2 frame. | ||
var frame: HTTP2Frame |
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.
shouldn't those var
s be public
Sources/NIOHTTP2/HTTP2Frame.swift
Outdated
/// the state change triggered by parsing this frame. | ||
public struct HTTP2FrameWithMetadataRequest { | ||
/// The HTTP/2 frame to send. | ||
var frame: HTTP2Frame |
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.
and these guys too?
0b7245e
to
e2f085a
Compare
Motivation: As discussed in our HTTP/2 types manifesto, it is helpful to run more metadata through the HTTP/2 pipeline. This makes it easier to write ChannelHandlers that fully participate in the HTTP/2 state machine while avoiding needing to too tightly couple the handlers. Modifications: - Added new HTTP/2 pipeline data types. - Removed HTTP2Frame.FrameFlags, moved flags into relevant payloads. Result: Right now, not much. All tests pass with new payload types. The full data path rewrite will follow in a separate patch, as this one is large enough.
e2f085a
to
36378a3
Compare
Ok, ready for re-review. |
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.
🚢it, that looks good!
Motivation:
As discussed in our HTTP/2 types manifesto, it is helpful to run more
metadata through the HTTP/2 pipeline. This makes it easier to write
ChannelHandlers that fully participate in the HTTP/2 state machine
while avoiding needing to too tightly couple the handlers.
Modifications:
Result:
Right now, not much. All tests pass with new payload types. The full
data path rewrite will follow in a separate patch, as this one is large
enough.