-
Notifications
You must be signed in to change notification settings - Fork 321
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
Remove State and for<'a> as requirements from Middleware and Request #895
Open
nyxtom
wants to merge
21
commits into
http-rs:main
Choose a base branch
from
nyxtom:patch-state-request
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Removes for<'a> and <'_> &'a lifetimes on Next - Changes all references to endpoints and middleware to use Arc<T> - Updates selection routing to return Arc handlers instead of Boxed - Simplfy server respond to pass along endpoint and middleware
nyxtom
changed the title
Remove State from Middleware and Request, Use Extensions
Remove State and for<'a> from Middleware and Request, Use Extensions
Jul 16, 2022
nyxtom
changed the title
Remove State and for<'a> from Middleware and Request, Use Extensions
Remove State and for<'a> from Middleware and Request, Use Extensions and Arc
Jul 16, 2022
nyxtom
changed the title
Remove State and for<'a> from Middleware and Request, Use Extensions and Arc
Remove State and for<'a> as requirements from Middleware and Request
Jul 16, 2022
nyxtom
changed the title
Remove State and for<'a> as requirements from Middleware and Request
Remove State and for<'a> as requirements from Middleware, Request, Next
Jul 16, 2022
nyxtom
changed the title
Remove State and for<'a> as requirements from Middleware, Request, Next
Remove State and for<'a> as requirements from Middleware and Request
Jul 16, 2022
nyxtom
force-pushed
the
patch-state-request
branch
from
July 18, 2022 19:58
4d2fe65
to
ac0abd3
Compare
jbr
reviewed
Jul 18, 2022
nyxtom
force-pushed
the
patch-state-request
branch
from
July 31, 2022 23:51
33c6ce0
to
1f6e5e9
Compare
@Fishrock123 would you mind taking a look at this as well? Trying to get this one to move forward for 0.17 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There are a few existing #645 #715 and #643 related to improving ergonomics. As it currently stands the generic
State
is required onRequest<State>
, as well asMiddleware<State>
,Next<'_, State>
,Endpoint<State>
,Listener<State>
, andServer<State>
and other places. Since we already have the ability to use extensions for things likereq.ext::<User>()
I figured why not just merge the two and call it a day.I recognize that moving the State object out of the generics and into the Extensions may impact performance so this is something to consider - but I think this improvement in ergonomics and overall simplicity in navigating the code is worth it. The code now treats the state like any other middleware via the
StateMiddleware<S>
.When you call
app.with_state(state)
this simply wraps the<T: Clone + Send + Sync + 'static>
in aStateMiddleware::new(state)
. The StateMiddleware will set the request extensions viaset_ext(data.clone())
and also on the response after callingnext.run
.The new
req.state::<State>()
has an.expects
on it when you call it which will automatically unwrap for you (or error out when you pass it a type that wasn't set in the middleware. The same goes forres.state::<State>()
which I like because now you can get the application state in the response as well.The original
.with_state
on theServer
was there for the generic type, but since state is now in middleware that function has been changed to require passing in the mutable server itself. The above example is the same as writing it below:The nice thing about this is now you can add multiple different state types rather than force everything into a single struct. You can also directly add the
StateMiddleware
by callingapp.with(StateMiddleware::new(s))
although in practice I think people will stick toapp.with_state
.Overall I think this is a great gain in ergonomics!
Update on Next<'_>
I've also now refactored the use of Next<'_> to no longer need the extra lifetimes in favor of Arc for endpoints and middleware. Selection routing will return these as well rather than explicit borrows with lifetime requirements. This simplifies some of the code and opens up the possibility for passing along
impl Middleware
function closures.This is the new format as it no longer requires
<State>
orNext<'_, State>
orNext<'_>
or anyuser_loader<'a>
as the requirementfor<'a>
is now gone. I will update this pull request to also support the use of a function closure. This will solve #854 as well through the use ofimpl Middleware
In the process I've simplified the use of the
Next
so that it can be used as anEndpoint
(rather than have a separate MiddlewareEndpoint). This was useful for theServer
to wrap the existingNext
selection and add in its own middleware.