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.

Sign up for GitHub

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

Jump to bottom

Move FromRequest and IntoResponse into their own crate? #561

Closed
davidpdrsn opened this issue Nov 24, 2021 · 7 comments · Fixed by #564
Closed

Move FromRequest and IntoResponse into their own crate? #561

davidpdrsn opened this issue Nov 24, 2021 · 7 comments · Fixed by #564
Labels
C-musings Category: musings about a better world

Comments

@davidpdrsn
Copy link
Member

Most crates that provide integrations with axum only need the FromRequest and IntoResponse traits. Examples include maud, tower-request-id, tower-cookies, axum-flash, and others.

These traits are somewhat more stable than the rest of axum. They haven't changed since 0.2 back in August, and also wont change in 0.4. So it is inconvenient for libraries since they have to bump their axum version to pull in changes that are essentially irrelevant, such as routing.

Because of this I think we should consider moving FromRequest and IntoResponse to their own crate alongside implementations for non-local types such as the strings from http and bytes. The traits would be re-exported from axum in the location they exist today.

I think we should be quite conservative about what goes into this new crate. For example I wouldn't move Html and Json even though those might be useful to some libraries. Ideally only what has to be moved should be moved.

Basically the same pattern as futures-core and tower-service and many other crates I'm sure.

I suggest naming this crate axum-core.

@jplatte thoughts?
@davidpdrsn davidpdrsn changed the title Move FromRequest and IntoResponse into their own crates? Move FromRequest and IntoResponse into their own crate? Nov 24, 2021
@davidpdrsn davidpdrsn added C-musings Category: musings about a better world I-needs-decision Issues in need of decision. labels Nov 24, 2021
@kellytk
Copy link
Contributor

kellytk commented Nov 24, 2021

I've two packages that extend axum.

  • The first uses async_trait, FromRequest, and RequestParts.
  • The second uses IntoResponse.

For any project that may be extended, I move user-facing types into a separate project-types package.

@jplatte
Copy link
Member

jplatte commented Nov 24, 2021

This sounds like a very good idea! It's a little bit unfortunate that the generic type default of hyper::body::Body will tie the version of axum-core to hyper's version, I wonder whether there is some clever way to avoid that while still being compatible with hyper without (much) additional boilerplate when depending on the "full" axum crate.

@davidpdrsn
Copy link
Member Author

I wonder whether there is some clever way to avoid that while still being compatible with hyper without (much) additional boilerplate when depending on the "full" axum crate.

Uh yeah thats worth exploring when we do this. Not depending hyper would mean loosing impl FromRequest<Body> for Body. I'm not sure how much its used in the wild but feels a bit unfortunate to loose.

@jplatte
Copy link
Member

jplatte commented Nov 24, 2021 •
edited
Loading

I think it wouldn't hurt too much to have axum::extract::Body as struct Body(pub hyper::body::Body); since full Body extraction is uncommon enough. I'm more worried about the type parameter defaults.

@davidpdrsn
Copy link
Member Author

I agree with that 👍

@davidpdrsn davidpdrsn removed the I-needs-decision Issues in need of decision. label Nov 24, 2021
@davidpdrsn davidpdrsn mentioned this issue Nov 24, 2021
Merged
1 task
@davidpdrsn
Copy link
Member Author

I gave it a shot in #564

@SabrinaJewson
Copy link
Contributor

SabrinaJewson commented Nov 27, 2021 •
edited
Loading

It's a little bit unfortunate that the generic type default of hyper::body::Body will tie the version of axum-core to hyper's version

This can be resolved by tying it to a hyper014 feature flag if you want to keep that impl.

Edit: Oh no, looks like you had the idea before me 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
C-musings Category: musings about a better world
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Move FromRequest and IntoResponse into new axum-core crate tokio-rs/axum
4 participants
@davidpdrsn @jplatte @kellytk @SabrinaJewson