Consider moving State off of Request

[Fishrock123]

It would be nice to not have Request have a generic parameter, for e.g. the AsyncRead trait which requires pinning. See #642 (comment) for where this was originally encountered.

However in interest of keeping middleware simple it would still be nice to allow (Request, Next) -> Fut<Result<>>, but for State it would probably ideal to have an extra parameter, kinda like Surf's Client, e.g. (Request, State, Next) -> Fut<Result<>>.

I was thinking maybe server.middleware() / endpoints could take impl Into<Middleware<State>> / endpoint, and so then that trait could be applied to both of the above with an extra function wrapper for the simple case, allowing both to work, and scoping State to the middleware/endpoint stack.

[Silentdoer]

please consider actix-web design, it's state can be multi different type in a server.

[Fishrock123]

@Silentdoer Could you elaborate more?

[Fishrock123]

#645 has an impl conflict for some reason but is effectively what I'm proposing here.

[fundon]

Actix uses a struct Data<T> to wrap a state T, so T can be different types.

async fn handler(a: Data<DB>, b: Data<Template>) ...

[Silentdoer]

we can get multi states by State argument in handler，like Request ext function? Such as state.server_state(), state.global_state()

[jbr]

I think there are likely performance implications of doing this, but ext and State could be the same thing potentially.

As it is, if you want multiple State types, you could set up a middleware that clones whatever state into each request's ext typemap.

Similarly, if State weren't a generic, it could be a starting value for each request's ext typemap, which then gets additional values inserted into it.

Without any tide change, it would be possible to experiment with this by using a typemap as State and cloning values onto the request typemap in a middleware. I think this might be a nice improvement to tide, but I'd like to see how it looks in an actual application and do some benchmarking

[Silentdoer]

Ok, thanks for your patience.

[yoshuawuyts]

@Fishrock123 This is not a direction I feel comfortable with; as per #642 (comment) in case we cannot set State: Clone we should make it so App::with_state(Arc::new(state)). But it seems like #644 enables State: Clone, so I don't understand what the problem is? It's normal to use pin-project in internals; that's entirely internal to the framework and abstracts away complexity from end-users.

[Fishrock123]

Just to be clear, this isn't to solve the issue that #644/#642 solves... to me this is just general ergonomics cleanup that was encountered during those. It seems like there would be a lot less for users to type when they don't need state and also less for middleware which will probably never interact with a specific state type. Related, this is probably a compilation type boost, since there will be less generic functions to worry about.

[yoshuawuyts]

@Fishrock123 We considered going down this path a while ago, and one downside of this is that it does break the Extension trait model; in order for this to work both State and Request must be co-located. We haven't really seen this being used much in the wild yet, but I wouldn't quite want to count us on it yet -- I could see it be incredibly useful for enabling third-party support for e.g. GraphQL, or for people building domain logic and wanting to create models that operate over Request + a DB connection.

[Fishrock123]

As discussed yesterday, this doesn't prevent an extension trait model, although it does change it.

If you're just trying to extend Request, that's easy. It doesn't change:

pub trait RequestExt {
    fn bark(&self) -> String;
}

impl RequestExt for Request {
    fn bark(&self) -> String {
        "woof".to_string()
    }
}

However if you have something like DatabaseState you would no longer impl RequestExt for Request<DatabaseState> but rather like so:

pub trait StateRequestExt {
    async fn fetch_from_db(&self, req: Request) -> Result;
}

impl StateRequestExt for DatabaseState {
    async fn fetch_from_db(&self, req: Request) -> {
        //  do something with request and state
    }
}