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 1.0 and hyper 1.1 #4

Merged
merged 3 commits into from
Jan 25, 2024
Merged

Conversation

jorendorff
Copy link
Contributor

This unavoidably breaks our examples and public API since Server was deleted.

However, I replaced it with axum::serve and found out that axum::Router is a decent replacement for our Router type and we get to delete a bunch of code.

Bumps both crates' version numbers to 0.2.

This unavoidably breaks our examples and public API since `Server` was deleted.

However, I replaced it with `axum::serve` and found out that `axum::Router` is a decent replacement
for our `Router` type and we get to delete a bunch of code.
@jorendorff jorendorff requested a review from a team as a code owner January 23, 2024 06:17
@@ -22,7 +22,12 @@ reqwest = { version = "0.11", features = ["default", "gzip", "json"], optional =
url = { version = "2.5", optional = true }

# For the server feature
hyper = { version = "0.14", features = ["full"], optional = true }
axum = "0.7"
Copy link
Member

Choose a reason for hiding this comment

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

Is there a way to do this without taking a dependency on axum? I like axum, but was attempting to have this library be only dependent on hyper...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's possible, but I think it's a good idea to return a tower::Service. I think it'll make more people able to use this library, not less, and I think it'll be more convenient for us.

Note that users do not have to buy into axum entirely in order to use this library. Maybe the README should mention this. Because Router is a tower::Service, calling it from some other async server framework that uses http::Request and Response is mainly just my_twirp_router.call(request).await. The tests exercise the router this way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe I'm missing the point you're making? If you want, I can revert that and make only docs and examples depend on axum.

Copy link
Member

@tclem tclem left a comment

Choose a reason for hiding this comment

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

Thanks for opening #7, I can see the challenges a bit better now.

I guess for our purposes, a dependency on axum (and tower) is fine (and really much nicer to use). There's really not a great reason to re-invent our own Body type...

@jorendorff, I trust your judgement here—do you have a sense of which direction is going to be better in the long run here?

@jorendorff
Copy link
Contributor Author

Axum makes the most sense now. It's a pretty established project for a relative newcomer -- two years old, 14k stars on github, 250 contributors, 100k+ monthly downloads (more than Warp)...

Hyper 1.x is designed to be a very low-level back-end library. I think we have to have something on top of Hyper, and Axum is the best existing choice.

@jorendorff jorendorff merged commit 4fc11f2 into main Jan 25, 2024
1 check passed
@jorendorff jorendorff deleted the jorendorff/hyper-update branch January 25, 2024 14:19
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