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

Improve State and Router docs #1543

Merged
merged 4 commits into from Nov 22, 2022

Conversation

jimmycuadra
Copy link
Contributor

Per my comment in #1313, this improves discoverability of how State works with nested/merged Routers.

I made a few other minor documentation fixes while I was reading:

  • Corrected the docs for Router::nest to say that it nests another Router rather than a Service. There is a separate function (Router::nest_service) for the latter.
  • Changed one of the headings in nest.md to use more idiomatic English.
  • Changed awkward phrasing under the "Sharing state with handlers" heading of the root documentation page.
  • Removed a trailing period from one of three list items for consistency.

Per my comment in tokio-rs#1313, this improves discoverability of how `State`
works with nested/merged `Router`s.

I made a few other minor documentation fixes while I was reading:

* Corrected the docs for `Router::nest` to say that it nests another
  `Router` rather than a `Service`. There is a separate function
  (`Router::nest_service`) for the latter.
* Changed one of the headings in nest.md to use more idiomatic English.
* Changed awkward phrasing under the "Sharing state with handlers"
  heading of the root documentation page.
* Removed a trailing period from one of three list items for
  consistency.
Co-authored-by: David Pedersen <david.pdrsn@gmail.com>
Copy link
Member

@davidpdrsn davidpdrsn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@jimmycuadra
Copy link
Contributor Author

jimmycuadra commented Nov 19, 2022

Before merging this I want to make sure what I wrote is actually correct and clear. After reading #1532 more closely I noticed:

Nesting (and merging) is only possible with a Router which doesn't have the state so it becomes more type safe.

After experimenting with nested routers, that statement seems more accurate, and this current sentence from the docs of Router::nest is misleading:

By default nest requires a Router with the same state type as the outer Router.

This doesn't really make sense, because you can't construct a Router with a non-unit state type to pass to Router::nest. For the nested router to have a non-unit state type, it must be constructed by calling with_state on a Router, which turns it into a RouterService, and hence must be nested with Router::nest_service, even if it happens to have the same state type as the outer router.

Am I understanding this correctly? If so, I'll need to revise the sentence I added and would also like to change the sentence from Router::nest's docs quoted above.

@jimmycuadra
Copy link
Contributor Author

In fact, it seems that once you have a Router with a non-unit state type, you can't use Router::nest at all, because by definition a Router has () as its state type if it's not converted to a RouterService by calling with_state, no?

For example:

#[derive(Clone)]
struct AppState {
    count: u64,
}

pub fn api() -> RouterService {
    Router::<AppState>::new()
        .nest("/api/v1", api_v1())
        //               ^^^^^^^^ expected struct `AppState`, found `()`
        .fallback(fallback)
        .with_state(AppState { count: 0 })
}

fn api_v1() -> Router {
    Router::new()
        .route("/teams", get(get_teams))
        .route("/teams/:slug", get(get_team))
}

How can you possibly use Router::nest on a Router that you ultimately call with_state on with a non-unit state type?

@davidpdrsn
Copy link
Member

davidpdrsn commented Nov 19, 2022

In fact, it seems that once you have a Router with a non-unit state type, you can't use Router::nest at all, because by definition a Router has () as its state type if it's not converted to a RouterService by calling with_state, no?

Sure you can. Router::new doesn't always give a router whose state is (). The state type param only defaults to (). So unless specified it becomes ().

Just do:

fn api_v1() -> Router<AppState> {
    Router::new()
        .route("/teams", get(get_teams))
        .route("/teams/:slug", get(get_team))
}

@davidpdrsn
Copy link
Member

Nesting routers with different state is possible by calling with_state to get a RouterService then using nest_service. Like shown here

@jimmycuadra
Copy link
Contributor Author

Ah, I see. So the state type of a router is inferred if with_state is called on it, but it can also be specified with an explicit annotation, even if the router doesn't actually (or yet) contain state of that type.

I'm going to revise my changes tomorrow now that I understand it better. I suspect I won't be the last person to be confused by this without better docs. Thanks for the example!

@davidpdrsn
Copy link
Member

Ah, I see. So the state type of a router is inferred if with_state is called on it, but it can also be specified with an explicit annotation, even if the router doesn't actually (or yet) contain state of that type.

Yes exactly.

I'm going to revise my changes tomorrow now that I understand it better. I suspect I won't be the last person to be confused by this without better docs. Thanks for the example!

Thanks!

@davidpdrsn davidpdrsn changed the title Improve State and Router docs. Improve State and Router docs Nov 19, 2022
@davidpdrsn davidpdrsn added T-docs Topic: documentation A-axum labels Nov 19, 2022
@jimmycuadra
Copy link
Contributor Author

Okay, I pushed up another commit which expands the docs on combining stateful routers. I added a section to the State docs that talks about it in detail, with links to the relevant methods. I moved the example of combining routers with different state types from Router::nest to Router::nest_service, because that's the function that actually makes that possible. I added relevant links between State, Router::nest, Router::nest_service, and Router::merge so it's easier to find all the information about how these types and methods differ and interact, regardless of which one you navigate to first. Ready for another review!

axum/src/docs/routing/merge.md Outdated Show resolved Hide resolved
axum/src/routing/mod.rs Show resolved Hide resolved
@davidpdrsn
Copy link
Member

By the way you might be interested in #1552

Co-authored-by: David Pedersen <david.pdrsn@gmail.com>
@davidpdrsn davidpdrsn merged commit 6771729 into tokio-rs:main Nov 22, 2022
@jimmycuadra jimmycuadra deleted the docs-state-router-tweaks branch November 22, 2022 08:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-axum T-docs Topic: documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants