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

Split state into a separate parameter #715

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
Open

Split state into a separate parameter #715

wants to merge 19 commits into from

Conversation

yoshuawuyts
Copy link
Member

@yoshuawuyts yoshuawuyts commented Oct 7, 2020

This PR changes the relationship between request and state. Instead of embedding request and state into the same type, it makes them two separate arguments. So req: Request<State> becomes req: Request, state: State. The idea behind this is that for smaller applications that are only parameterized by a single type calling that type "state" feel somewhat indirect.

Take for example this MongoDB example:

async fn index(req: Request<Database>) -> tide::Result {
    let coll = req.state().collection("fun_numbers"); // `req.state()` == `Database` is a relationship that is non-obvious
    let res = coll.insert_one(doc! { "x": 1 }, None).await?;
    let body = format!("the inserted id was: {:?}", res);
    Ok(Response::builder(200).body(body).into())
}

req.state() feels like an odd indirection to get to the parameterized type. Instead if the type was a direct input to the function it'd be easier to name and use:

async fn index(req: Request, db: Database) -> tide::Result {
    let coll = db.collection("fun_numbers"); // It's clear we're calling a method on the database here
    let res = coll.insert_one(doc! { "x": 1 }, None).await?;
    let body = format!("the inserted id was: {:?}", res);
    Ok(Response::builder(200).body(body).into())
}

This also makes it easier to convert from http_types::Request to tide::Request since the State param does not need to be populated. I don't think this functionally changes much about Tide; except that in some cases it'll just feel that little bit smoother.

@yoshuawuyts
Copy link
Member Author

One question raised on Discord by @jbr is whether we may instead want to merge / combine "global state" and "local state" into a single type. This could take the shape of a typemap that's passed along with both request and response.

Also related are #645 and #643.

@Fishrock123 Fishrock123 self-requested a review October 9, 2020 22:06
alexander-jackson and others added 18 commits October 15, 2021 14:26
Fix a broken documentation link that should point to `serde_qs` so that
users do not have to search themselves.
This should help demystify some cases when client errors are determined by parsing and propagated via `?`.
This should help the default logger be as helpful as possible.
Removes outdated parts of our .github dir that were accidentally added back a few months ago. This removes:

- DCO notices
- issue templates
- PR templates
It now is only installed via the `logger` feature. In order to do this
we need the `std` feature from `log` crate.
… loop

I also added a test for this that shows it now works.
thanks to Hasali19 for noticing this
@mdtusz
Copy link

mdtusz commented Mar 7, 2022

What's the state (heh) of this PR? Having the state as a separate arg does seem to be a better interface to me so no opposition here, but it would be nice to either close or land this. The local/global states would be excellent too (I assume you meant request-local state).

@joshtriplett
Copy link
Member

This and #895 seem mutually exclusive, and #895 seems like a cleaner approach. What do you think, @yoshuawuyts?

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.

8 participants