Warn about "unreachable" routes

[mjackson]

We should warn people who have routes that can never actually be reached because they are put after some other route that will always match first. For example, in this route config:

<Route path="comments" ... />
<Redirect from="comments" to="/somewhere-else" />

the <Redirect> will never actually be reached because the <Route> that precedes it will always match the comments path.

[KyleAMathews]

+1 puzzled over this problem for a bit the other night before realizing I needed to switch the route order.

[taion]

👍 this would be really helpful

[andresgarza]

👍

[jackfranklin]

👍 after a tweet from @knowbody I've made a rough start on this. Hopefully get something into PR state soon :)

[knowbody]

@jackfranklin feel free to send a [WIP] PR so other ppl can help you as well.

thanks for taking it :)

[timdorr]

As mentioned in #2351, we probably want to implement this as an opt-in, development/testing-only behavior. Ideally, this can be a checkUnreachableRoutes() function that can be used in tests or behind a NODE_ENV flag to check them as you're building and that goes away when you go to production.

[taion]

I'm switching this issue to a discussion because there are two ways forward here that are going to have implications for how matching works.

Option 1

This is my preferred approach.

We use a Pareto-principle-style approach where we check route reachability by constructing paths corresponding to leaf routes, then run the matching algorithm to make sure that those paths resolve to the expected routes.

This approach has two main benefits:

• By construction, the results of the check will match the results of actual route matching – as we'll be using the same matching algorithm in both places, there's no risk of the "check" code and the "match" code getting out of sync (external consistency).
• This doesn't impose any restrictions on our path matching; we could merge [PoC] Use Express-style path matching #3105 and use path-to-regexp and strip out our own path building code and be consistent with the rest of the ecosystem, and give users a more flexible path definition syntax if they need it.

The main drawback compared to option 1:

• To run the match, we need to make values for URL parameters. For this to work in full generality, we'll probably need to offer this as a separate utility function, and allow the user to provide "override" values for some of the route parameters to run the match – e.g. if something like [PoC] Use Express-style path matching #3105 means that certain route parameters are restricted to only integers or whatever. If mistakes are made, we could yield false negatives (i.e. if inferred or specified URL parameters conflict with other path segments).

Option 2

This is @mjackson's preferred approach.

We use a Hapi-style static check that statically analyzes routes and paths to make an absolute determination of reachability v route conflicts.

The main advantages:

• This gives absolute verification over whether routes are valid. If implemented correctly, it will alert exactly on unambiguous conflicts.
• As this is static, this can be done automatically in development mode, and won't require any extra exposed API, as it won't need to know route parameters.

The main drawbacks:

• The check algorithm will necessarily be somewhat divorced from the match algorithm, and the two can potentially fall out of sync, leading to incorrect check results.
• This restricts our path specification DSL – like with Hapi, we wouldn't be able to support regex routes, and route parameters would just be opaque blobs. For example, we wouldn't be able to support a case with paths /foo/:intParam, /foo/:stringParam, as this would be a conflict given that we can't restrict the match patterns for the two params separately (by contrast, this would be legal in option 1).

cc @ryanflorence @mjackson

Also, @KyleAMathews, you're already on this thread, and I recall hearing that you were a Hapi user. Any thoughts here?

[KyleAMathews]

I did/do use Hapi (much reduced now that I use GraphQL). I've been perfectly happy with Hapi's routes but not sure how useful that feedback is as my routes are all very vanilla.

It seems like the main advantage of express-style routes is their greater power/flexibility. Is there any common use cases or sites that have run into problems with our Hapi-style routes?

[taion]

A few notes – #2987 is an implementation of my option 1 above. The code in Hapi implementing their equivalent of option 2 is at https://github.com/hapijs/call.

@KyleAMathews

Consider the motivating example from #3098 (comment). Ignoring the optional path segment, if you want to do something like:

<Route path="{category}" />
<Route path="{slug}" />

Then this won't work if we check for static reachability, since the {slug} route won't be reachable. With something like Express-style routes, you could do e.g.

<Route path=":category(foo|bar|baz)" />
<Route path=":slug" />

Or more broadly without static route conflict checking, you can do something like

<Route path=":category" condition={isValidCategory} />
<Route path=":slug" />

[taion]

The other issue here is that getChildRoutes allows for fully dynamic child routes, so it's not strictly possible to know statically what the routes are.

[ryanflorence]

v4 has no routes, woo hoo!