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

Upgrade to http-body 1.0 #348

Merged
merged 59 commits into from
Nov 21, 2023
Merged

Upgrade to http-body 1.0 #348

merged 59 commits into from
Nov 21, 2023

Conversation

davidpdrsn
Copy link
Member

@davidpdrsn davidpdrsn commented Mar 24, 2023

Still some things to do but should allow tokio-rs/axum#1882 to move forward.

TODO

sleep_data: Option<Sleep>,
#[pin]
sleep_trailers: Option<Sleep>,
sleep: Option<Sleep>,
Copy link
Member Author

Choose a reason for hiding this comment

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

@LucioFranco does the changes here seem right to you?

@rakshith-ravi
Copy link

Hey. New to tower-http codebase. Complex tasks would certainly take me time to understand and work on. Are there any low-hanging fruits that I can help with, and then I can slowly work my way up?

Just want to see if there's any way I can help push this ahead

Copy link
Collaborator

@jplatte jplatte left a comment

Choose a reason for hiding this comment

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

Some more small comments, have now reviewed completely and looks good to me overall (obviously 😄).

tower-http/src/body.rs Outdated Show resolved Hide resolved
tower-http/src/compression_utils.rs Outdated Show resolved Hide resolved
tower-http/src/compression_utils.rs Outdated Show resolved Hide resolved
tower-http/src/compression_utils.rs Outdated Show resolved Hide resolved
tower-http/src/lib.rs Outdated Show resolved Hide resolved
tower-http/src/lib.rs Outdated Show resolved Hide resolved
@@ -1,3 +1,8 @@
fn main() {
eprintln!("this example has not yet been updated to hyper 1.0");
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not? I did it with little effort for the tower-async version. Want me to contribute this as a PR to your PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

Same reason as #348 (comment), axum doesn't yet support http 1.0

Copy link
Contributor

Choose a reason for hiding this comment

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

True. For now I was using your branch. I get you might not want to do that, but now you do not have an example at all... so dunno.

Copy link
Contributor

Choose a reason for hiding this comment

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

axum doesn't yet support http 1.0

but now it does, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep it does.

Copy link
Contributor

Choose a reason for hiding this comment

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

A PR to restore this example using axum 0.7: #448

@@ -1,3 +1,8 @@
fn main() {
eprint!("this example has not yet been updated to hyper 1.0");
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not? I did it with little effort for the tower-async version. Want me to contribute this as a PR to your PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

How did you port it? Tonic doesn't support http 1.0. Did you do a bunch of mapping of the types?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry I was wrong. Tonic and Warp examples I deleted during my original port as I do not use these libraries. I only kept in the hyper and axum ones and those are the only once I fixed this time as well.

@@ -39,6 +39,10 @@ impl AllowPrivateNetwork {
Self(AllowPrivateNetworkInner::Predicate(Arc::new(f)))
}

#[allow(
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm, I wonder why I don't need these on tower-async. Are we running different versions of clippy or different configs?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure. I really wanna cut down on the number of feature flags we use anyway. A job for another day 😅

Copy link
Contributor

@GlenDC GlenDC left a comment

Choose a reason for hiding this comment

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

Besides the few, mostly open or nitpick comment. All looks excellent to me.

@davidpdrsn davidpdrsn enabled auto-merge (squash) November 21, 2023 13:40
@davidpdrsn davidpdrsn merged commit 04361b7 into master Nov 21, 2023
11 checks passed
@davidpdrsn davidpdrsn deleted the http-body-1.0 branch November 21, 2023 13:42
@jplatte
Copy link
Collaborator

jplatte commented Nov 21, 2023

Hm, we do not have an example on how to combine this w/ tower-util to serve a ServeDir or Redirect directly without any extra routing from axum, warp or whatever?

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.

5 participants