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

Introduce Response type alias as a shorthand for Response<BoxBody> #590

Merged
merged 2 commits into from
Dec 5, 2021
Merged

Introduce Response type alias as a shorthand for Response<BoxBody> #590

merged 2 commits into from
Dec 5, 2021

Conversation

SabrinaJewson
Copy link
Contributor

@SabrinaJewson SabrinaJewson commented Dec 5, 2021

Motivation

Nowadays Response is pretty much only used with BoxBody as its body type, but it becomes tedious to type out Response<BoxBody> every time and have to import them both.

Solution

Provide a new type alias to allow writing Response instead of the full Response<BoxBody>:

pub type Response<T = BoxBody> = http::Response<T>;

This type is defined in axum_core::response but is re-exported in axum, axum_core, axum::response and axum_core::response axum::response. The "default type parameter" trick is used to not lose any functionality over http::Response.

Prior art: Warp does this.

Copy link
Member

@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.

Haven't reviewed all of the changes, but from the PR description this sounds good to me!

@davidpdrsn
Copy link
Member

I think the ergonomics gained from this are nice but I have two concerns:

  1. How common is using Response<BoxBody> actually? I only think I use it when implementing IntoResponse, otherwise I've been able to get away with impl IntoResponse. At least I don't think we should re-export it from the root modules in axum and axum-core. IMO having them in (axum|axum_core)::response is fine.
  2. Are we doing users a disservice by not having a default for Request? The only appropriate default request body type would be hyper::Body but, and this is just speculation, I know Sean is thinking about 1.0 for hyper which will probably involve changes to the body type.. So maybe an eventual hyper/http-body 1.0 will have a different body type that would require us to make breaking changes to update to. I would like to avoid axum-core depending on hyper, as discussed earlier. Of course all that only matters if we want consistency between Request and Response.

Personally I think we should wait with this and see what happens in hyper/http-body over the next 6 months or so.

We could also only add this to axum and not axum-core. I think I'd be more open to that. However this might not matter in the long run because a 1.0 of hyper would probably also mean a 1.0 of http, which would probably include breaking changes that would propagate to axum-core 🤷

@SabrinaJewson
Copy link
Contributor Author

How common is using Response<BoxBody> actually?

In my personal project I end up using it quite a lot, because it's more convenient for callers (and equally easy to implement if there are branches) if a function returns Response<BoxBody> instead of impl IntoResponse, since they might have to call .into_response() less. It also can be useful when transforming responses, e.g. with a MapResponseLayer.

At least I don't think we should re-export it from the root modules in axum and axum-core. IMO having them in (axum|axum_core)::response is fine.

I will do that.

Are we doing users a disservice by not having a default for Request?

I don't think so, because Axum places a much greater focus on generic request bodies. It might be best if the situation around request bodies is left to evolve separately after we see where Hyper goes.

We could also only add this to axum and not axum-core.

I decided against this because:

  1. Clicking on the return type of into_response in Rustdoc should take you to the type alias, which is the type you'll probably want to use.
  2. Most people who implement into_response will be library crates that depend on axum-core, so they would benefit from having the type alias.
  3. axum-core itself can benefit from the type alias.
  4. Its semver stability is pretty much entirely tied to axum-core - there isn't really a situation where it would change but not other parts of axum-core.

@davidpdrsn
Copy link
Member

Its semver stability is pretty much entirely tied to axum-core - there isn't really a situation where it would change but not other parts of axum-core.

I also kinda realized that while I wrote my reply so because of that I think it makes sense!

@davidpdrsn davidpdrsn merged commit dfb06e7 into tokio-rs:main Dec 5, 2021
@davidpdrsn
Copy link
Member

Thanks!

@SabrinaJewson SabrinaJewson deleted the response-type-alias branch December 5, 2021 18:40
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.

3 participants