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

Move FromRequest and IntoResponse into new axum-core crate #564

Merged
merged 10 commits into from
Nov 30, 2021

Conversation

davidpdrsn
Copy link
Member

@davidpdrsn davidpdrsn commented Nov 24, 2021

Fixes #561

What changed:

  • Default for B type param in FromRequest and RequestParts is gone 😞 Users will now have to do impl FromRequest<axum::body::Body> ... to get the previous behavior. This is annoying I think. Not quite sure if getting rid of the dependency on hyper is worth it. Haven't made up my mind quite yet.
  • BodyAlreadyExtracted, HeadersAlreadyExtracted, and ExtensionsAlreadyExtracted now have public constructors (they're unit structs) since they're used in both axum-core and axum.
  • axum::Error has moved to axum_core::Error (still re-exported from the old location) and its new constructor is now public since its used in both crates. I'm not very happy about this since users outside of axum shouldn't create this type. It only exists as a wrapper that makes BoxError implement std::error::Error.
  • JsonRejection has changed a bit. It now contains BytesRejection instead of BodyAlreadyExtracted since it calls Bytes::from_request.
  • Had to copy hyper::body::to_bytes into axum-core. Luckily it had no dependencies on hyper itself.
  • hyper::Body no longer implements FromRequest. We previously had RawBody as the wrapper for it. I think thats a fine.
  • Headers needs to live in core because of impl IntoResponse for (StatusCode, Headers) and friends. Not very happy with this but loosing those impls would hurt ergonomics so think the tradeoff is fine.
  • Had to duplicate some macros between the two crates. No biggie imo.

These things are obviously breaking so will have to be released in 0.4

TODO

  • Changelog

@davidpdrsn davidpdrsn added this to the 0.4 milestone Nov 24, 2021
@davidpdrsn davidpdrsn changed the title Move FromRequest and IntoResponse into their own crate Move FromRequest and IntoResponse into new axum-core crate Nov 24, 2021
@jplatte
Copy link
Member

jplatte commented Nov 24, 2021

I have an idea on how to resolve the type parameter default issue! We can have a hyper0_14 crate feature that adds the dependency and the default. I'll elaborate when I'm home, currently on the move.

@jplatte
Copy link
Member

jplatte commented Nov 24, 2021

Okay, so to elaborate: The crate feature will be a default feature such that people can just do the obvious thing. When a new version of hyper is released, we would still put releasing a new axum-core version on the todo list, but people could upgrade hyper independently of axum and kick out the old hyper version from their dependencies entirely by making sure that all of their indirect axum-core dependencies use default-features = false and then they get rid of any reliance on the default and add the same setting (I guess this will be threaded through axums features then).

With this scheme, we could even keep the FromRequest<hyper::body::Body> impl and additionally add more FromRequest<hyper::body::Body> impls for different hyper versions under separate feature flags. I.e. if this goes through as axum-core 0.1 and we start off with a hyper0_14 feature, once 0.15 releases we

  • add hyper0_15 as an off-by-default feature that only adds a trait impl in a non-breaking release
  • prepare for axum-core 0.2 which switches both the default Cargo feature and the conditional associated type default to hyper 0.15

So.. Does that sound good or tad to crazy and unprecedented? 😄

@tesaguri
Copy link

You can do something like pub type RequestParts<B = crate::body::Body> = axum_core::extract::RequestParts<B>; in axum crate. This cannot be applied to FromRequest trait (not without RFC 1733, at least), though.

@jplatte
Copy link
Member

jplatte commented Nov 25, 2021

You can do something like pub type RequestParts<B = crate::body::Body> = axum_core::extract::RequestParts<B>; in axum crate.

Ooohh, this is also an interesting possibility. It doesn't allow to keep the Body extraction as-is, but otherwise sounds better than my (somewhat crazy) idea 😄

This cannot be applied to FromRequest trait (not without RFC 1733, at least), though.

Actually, it should not be a problem to just have a separate FromRequest trait in axum like this:

trait FromRequest<B = crate::body::Body>: axum_core::FromRequest<B> {}
impl<B, T> FromRequest<B> for T where T: axum_core::FromRequest<B> {}

Which effectively works as a trait alias.

@davidpdrsn
Copy link
Member Author

davidpdrsn commented Nov 25, 2021

trait FromRequest<B = crate::body::Body>: axum_core::FromRequest<B> {}
impl<B, T> FromRequest<B> for T where T: axum_core::FromRequest<B> {}

This would mainly benefit users using FromRequest in trait bounds, right? Because to implement the trait you'd still have to implement the version from core, which doesn't have the default. And the only times you're using FromRequest in bounds is probably when implementing FromRequest, which makes the alias less useful 🤔

@davidpdrsn
Copy link
Member Author

It would be so nice if hyper::Body was in its own crate 😞

@davidpdrsn
Copy link
Member Author

Also worth noting that there is discussion in hyper about splitting the Body type into separate types that each are more focused hyperium/hyper#2345. What that means for axum is hard to say but I guess that would break whatever default type we have.

@jplatte
Copy link
Member

jplatte commented Nov 25, 2021

This would mainly benefit users using FromRequest in trait bounds, right? Because to implement the trait you'd still have to implement the version from core, which doesn't have the default. And the only times you're using FromRequest in bounds is probably when implementing FromRequest, which makes the alias less useful 🤔

Right, yeah. The alternative would be to copy the core FromRequest definition and only have a Sized supertrait for both, then adding a blanket impl that delegates. With that setup, you could implement the version that has the default.

FWIW, I think trait aliases also won't allow impls on them.

@davidpdrsn
Copy link
Member Author

I'll try an implement the suggestions here and see what it feels like to use them.

@davidpdrsn
Copy link
Member Author

I’m leaning towards dropping the default types. It’s less ergonomic but also easier to understand and easy to document. When hyper moves towards 1.0 we can reevaluate.

@davidpdrsn
Copy link
Member Author

@jplatte How do you feel about the state of this? As I wrote above I'm fine with it, even though its slightly less ergonomic. Would rather keep things conceptually simple and explore different solutions in the future.

I would also like to get 0.4 out sometime soon-ish together with axum-extra since I need those for something else I'm building 🤓

@jplatte
Copy link
Member

jplatte commented Nov 30, 2021

The ergonomic hit only applies to writing custom extractors now with IntoResponse no longer being generic, right? I think that'd be fine. It should already be the best practice to implement it generically anyways.

@davidpdrsn
Copy link
Member Author

The ergonomic hit only applies to writing custom extractors now with IntoResponse no longer being generic, right?

Yep that’s correct.

@davidpdrsn davidpdrsn enabled auto-merge (squash) November 30, 2021 13:43
@davidpdrsn davidpdrsn merged commit 254d8fd into main Nov 30, 2021
@davidpdrsn davidpdrsn deleted the axum-core-crate branch November 30, 2021 13:46
@david-perez
Copy link
Contributor

@davidpdrsn Shouldn't the macros in the axum-core crate now be depended upon from the axum crate, instead of maintaining two copies?

@davidpdrsn
Copy link
Member Author

That would require the macros to be public API, which I don’t want.

@david-perez
Copy link
Contributor

@davidpdrsn Why the #[allow(deprecated)]s (example) in the macros.rs file?

@davidpdrsn
Copy link
Member Author

For when we have deprecated enum variants in some of the rejections.

Please hop on discord for future questions like this. Otherwise everyone watching the repo keep getting notifications.

@david-perez
Copy link
Contributor

david-perez commented Dec 22, 2021

@davidpdrsn

For when we have deprecated enum variants in some of the rejections.

I was confused because #[allow(deprecated)] is not in the other impl blocks (Display, Error, Default), so I don't understand how it could work if you add a deprecated enum variant.

Please hop on discord for future questions like this. Otherwise everyone watching the repo keep getting notifications.

Sorry, I don't have Discord. I thought that asking a question about a particular piece of code in the PR where it was last touched was appropiate. If you don't want watchers to get notified about questions, you should probably update the readme, since it states that opening issues to ask questions is fine, and that also creates notifications for watchers.

@davidpdrsn
Copy link
Member Author

davidpdrsn commented Dec 22, 2021

I was confused because #[allow(deprecated)] is not in the other impl blocks (Display, Error, Default), so I don't understand how it could work if you add a deprecated enum variant.

Hm good question 🤔 I don't specifically remember.

Sorry, I don't have Discord. I thought that asking a question about a particular piece of code in the PR where it was last touched was appropiate. If you don't want watchers to get notified about questions, you should probably update the readme, since it states that opening issues to ask questions is fine, and that also creates notifications for watchers.

If you don't have discord then its no big deal, the questions just feel slightly unrelated to this PR but its alright. Questions are very welcome 😊

EDIT: You're also welcome to open a discussion.

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.

Move FromRequest and IntoResponse into their own crate?
4 participants