-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Update matchit and fix nesting inconsistencies #1086
Conversation
f0b6e1d
to
415965e
Compare
f6dbabe
to
f6bab0f
Compare
Co-authored-by: Jonas Platte <[email protected]>
Soon I'll do a more thorough PR description+comments that details the changes a bit better. |
@jplatte This is ready for review now. I hope the docs and changelog makes the changes clear but let me know if you have questions :) |
I've been thinking about this and believe the right move is not delegate to fallbacks to outer If you're nesting a lot of fn router() -> Router {
Router::new().fallback(...)
}
fn make_some_routes() {
router().route("/", ...)
} I've made the changes here and will fix the merge conflicts now so we can start the review process. |
I still think this is a footgun, where people are going to nest routers and then configure a root fallback and forget to configure the same fallback on every level. Even if they make a This also means that splitting up your route definitions into nested groups means it doesn't merge all the routes into a single route set anymore, which was previously being done as an "optimization" and so I assume there is in fact a measurable impact there. I really don't like that this new design means the best performance is to skip nesting routers and instead set a bunch of routes with identical prefixes. Have you given any thought to having a separate |
I'm fine with that. I still think it's the right trade-off.
I have not but like to not complicate the routing more than it is already. The purpose of those is make nesting simpler so adding more methods goes against that. |
While updating to the new matchit I ran into a bunch of inconsistencies regarding
nest
. Specifically cases where nesting aRouter
or an opaque service had different behavior. That is also something people have asked about on discord before.This addresses that by always nesting services opaque in
nest
, i.e. we don't attempt to downcast it to aRouter
and change the behavior based on that. This makes the behavior more consistent and simplifies the implementation.Old, outdated, description:
While updating to the new matchit I ran into a bunch of inconsistencies regarding
nest
. Specifically cases where nesting aRouter
or an opaque service had different behavior. That is also something people have asked about on discord before.This addresses that by splitting
nest
intonest
: Which only acceptsRouter
s.nest_service
: Which accepts any opaqueService
.Once this is merged I'm gonna change
fallback
to work how I described in #1053 (comment). If we didn't addnest_service
that would introduce yet another inconsistency so wanted to get this in first.