remix-run / react-router

Declarative routing for React
https://reactrouter.com
MIT License
53.16k stars 10.31k forks source link

Deprecate <Route exact>, match path exactly by default #4958

Closed mjackson closed 4 years ago

mjackson commented 7 years ago

I'm beginning to think that <Route exact> should default to true. Seems like it would be less surprising if it did.

Currently, when someone does:

<Route path="/" ... />
<Route path="/about" ... />

and the URL is /about, both routes render. Of course, there's a logical explanation for why they do, but I think it would be less surprising if they didn't.

I honestly can't think of any valid use cases that would break if we did make this change. Anyone?

mjackson commented 7 years ago

Bah, nevermind. My brain wasn't working when I wrote this issue.

ForbesLindesay commented 7 years ago

I found defaulting to exact useful because I wanted /foo/bar/some-garbage to 404 and that's really difficult if your routes aren't exact by default.

mjackson commented 7 years ago

@ForbesLindesay Can you please say more? I'd like to hear about your use case.

ForbesLindesay commented 7 years ago

I want to make sure I always show an appropriate error page when the user tries to browse to a page that doesn't exist.

If I have something like:

function MyComponent() {
  return (
    <Switch>
      <Route path="/some-path" component={LeafComponent}/>
      <Route component={NoMatch}/>
    </Switch>
  );
}

then if the user accidentally types /some-path/some-non-existant-path into their browser, it will seem to match, and will display LeafComponent. If I then change the code in the future, there might be invalid links in the wild that I don't know about (because I didn't expect people to add arbitrary strings to the end of URLs).

This problem is made worse, by the fact that I won't notice it in normal operation. I rarely test the error pages, so I won't notice that there are lots of URLs that should error, but instead just display content. The solution is to add exact, but I may not notice the problem for a long time.

Conversely, if we had a hasChildren property, which defaulted to false, we could instantly throw an error when it was used incorrectly, because we can detect that the moment someone tries to render some child routes.

timdorr commented 7 years ago

Yeah, this would default to not matching on 404-ish URLs, which is the same behavior as <= 3.0.

pshrmn commented 7 years ago

FWIW, path-to-regexp's default end value is true*.

Switching to this would break a lot of code, but adding exact={false} is a pretty easy fix.

*Also, is the description for the end option really confusing for other people too?

When false the path will match at the beginning.

To me, that makes it sound like it starts matching anywhere when end is true, when really it just adds a $ to the end of the regex.

ForbesLindesay commented 7 years ago

I think exact is ok as a name, although I dislike having any options that default to true. I don't want to derail this with a naming bikeshed though unless we first agree on the default value.

mjackson commented 7 years ago

Agree this change would break a lot of code @pshrmn, but I can't shake the feeling that we chose the wrong default behavior just for the sake of child routes.

Just so everyone is clear what I mean by that, take e.g. our Basic example code. There's a <Route> that looks like this:

<Route path="/topics" component={Topics}/>

Inside the Topics component there are more routes, one of which matches the URL /topics/:topicId.

If we were to change the default behavior to try to match the entire URL, this pattern would break and we would need a new way to indicate that the parent route might have child <Route>s. Right now, we just assume that every route could possibly have child routes.

I also think it's very telling that (as @pshrmn pointed out) the path-to-regexp folks chose the opposite default to what we did. That right there should give us cause to think twice about this.

So right now I'm probably 75% convinced we chose the wrong default.

mjackson commented 7 years ago

Also a HUGE +1 for being able to warn when someone uses the router incorrectly, as @ForbesLindesay suggested in https://github.com/ReactTraining/react-router/issues/4958#issuecomment-293613721. Making child routes opt-in lets us do that.

pshrmn commented 7 years ago

Thinking about this more, I think that swapping to end=true would make more sense.

Also, because it is a breaking change, I agree that it would be a good opportunity to rethink the exact prop name. I like how clean using exact as a boolean attribute looks.

<Route exact path='/here' ... />

(side note, why were they removed from the jsx docs https://facebook.github.io/react/docs/jsx-in-depth.html#boolean-attributes?)

While it is superficial, it would be nice to maintain that with a defaults-to-false prop.

<Route exact={false} path='/here' ... />
<Route hasChildren path='/here' />
timdorr commented 7 years ago

partial might be better.

mehcode commented 7 years ago

:+1: to startsWith

<Route startsWith path='/here' ... />
ForbesLindesay commented 7 years ago

@pshrmn I think, long term, the react team might prefer to make <Route exact /> be a short hand for <Route exact={exact} /> rather than a shorthand for <Route exact={true} />, but that would be a big breaking change. This may explain them downplaying property shorthands in the docs.

@mehcode I quite like startsWith

mjackson commented 7 years ago

Well it sounds like we're all leaning toward making this change. If we do, I'd like to:

I'm also 👍 for <Route startsWith>, especially since that's already the name of a native String method. The less we have to explain the better :)

pshrmn commented 7 years ago

My only issue with startsWith is that as a boolean it doesn't sound right to me. I would expect a prop called startsWith it to take a string.

<Route startsWith={true} path='/test' ... />

I think that partial or parent make more sense in that respect.

<Route partial={true} path='/test' ... />
<Route parent={true} path='/test' ... />

At the end of the day though, as long as the documentation is there, it doesn't really matter to me what the name is.

merrywhether commented 7 years ago

Another option would be to change things so that path=... is always an exact match and something like pathStartsWith=... or partialPath=... be inexact. Then you don't need the boolean for signaling at all.

The downside is that would lead to more code breakage.

mehcode commented 7 years ago

Yet another option is drop this "partial" idea and do something more (pseudo-)standard:

// Exact match
<Route path='/document' ... />

// Exact match with parameter
<Route path='/document/:param' ... />

// "Starts With" Match
<Route path='/document/*' ... />

This matches expectations set by the community:


The more I think on this, the more I like it. It's simple. It's intuitive.

pshrmn commented 7 years ago

Relying on the path would not work.

Given the pathname /document/one/two, the route:

<Route path='/document/*' ... />

The generated match.url would be /document/testing. However, only the parent part of the path should be included in match.url (i.e. /document or maybe /document/).

/document/* also would fail to match the pathname /document.

mehcode commented 7 years ago

/document/* also would fail to match the pathname /document.

We control this here. We can either make the trailing slash unimportant (not sure what react-router's stance on that is) or allow /document* which exists and works just as well in all frameworks I linked.


The generated match.url would be /document/testing. However, only the parent part of the path should be included in match.url (i.e. /document or maybe /document/).

I don't follow your assertions. We can control this. To me, it's far more intuitive for match.url to be /document or /document/ (depending on what is before the * or depending on what react-router's stance on the importance of trailing slashes).

mjackson commented 7 years ago

@mehcode We don't actually control (or want to control) this anymore. As of v4 we delegate all pattern matching to path-to-regexp. Doing something different than what they do would just mean we have to explain ourselves.

ryanflorence commented 7 years ago

Alright, I'm convinced we got exact wrong here (thanks for bearing w/ my idiocy in Austin @mjackson, as usual).

When I first added exact in the v4 rewrite I viewed it as nothing more than the replacement for IndexRoute. Since index routes were less common than routes, the default was true. However, the v4 API, especially with <Switch> makes it harder to use/understand than we expected.

I'd like to do partial (I actually was debating using exact vs. partial when exact was first added), but I'm fine with hasChildren too. One references the algorithm (match partially) the other the developer's intent ("I'm gonna nest some routes"). It's a dumb reason, but I like partial because it's shorter, no camelCase, so it's prettier in the docs (🤦🏽‍♂️).

Providing a smooth upgrade on a default boolean that flips the other way is kind of a pain though.

In order to do the react-like thing which is:

  1. upgrade
  2. get warnings
  3. fix warnings
  4. upgrade
  5. app works the same

I think we'd need to

  1. ship a 4.x+1 that warns you should add an exact prop on every route, no more default to true.
  2. ship 5.0 that warns to remove all exact props (😂) and only add partial when needed, which, as @ForbesLindesay points out, we can actually know their intent now (!!!) and give a great console message ("Hi! It looks like you're trying to nest routes, add a partial prop to the parent route so")

So developer intent? hasChildren, isParent, hasChildRoutes Or algorithm branch? partial, matchPartially, startsWith

ForbesLindesay commented 7 years ago

I wonder if you could get away with:

  1. Ship a 4.x+1 that warns you to add "partial" to every rout that we detect has children.
  2. Ship a 5.0 that warns you to remove "exact" as it's now the default.

It's still two steps, but at least the steps aren't "Hey, you need to add this", "Joke, you can remove it again now".

Personally I still prefer hasChildren or hasChildRoutes for the clear expression of intent, but I have no big objection to partial.

ryanflorence commented 7 years ago

@ForbesLindesay Then in 5.0 do we log "Hey, you used false, that's the default, you can remove it"?

ryanflorence commented 7 years ago

Another option is to just export a new Route

import { Route } from 'react-router-dom/next'

Then warn about usage of the current Route completely. Then in 5.0 we remove /next.

I think I like that a lot, actually. Let's us keep the code cleaner in the /next and just a basic warning in the current Route, instead of branching and warning and making a mess of things.

pshrmn commented 7 years ago

I'd say that the more descriptive the prop name is, the better.

<Route hasChildRoutes path='/who' component={Who} />
<Route hasChildren path='/what' component={What} />
<Route isParent path='/where' component={Where} />
<Route parent path='/when' component={When} />
<Route partial path='/why' component={Why} />

I like hasChildRoutes/isParent/parent the most. If I had to pick one, I would say parent for the same dumb reason of it being shorter.

CrocoDillon commented 7 years ago

I definitely agree on the previous comment that because children is already a React concept, hasChildren would be too confusing. I also like short names.

sercanov commented 7 years ago

I certainly think so, was felt awkward when both parent and child component rendered at the same time.

dbackeus commented 7 years ago

Personally prefer the algorithmically based prop names. hasChildren etc can be confused with non route-related children while partial is immediately associated with the routing. Also as mentioned it is shorter / cleaner which tips the scale in a tie breaker situation.

rnYulianto commented 7 years ago

Hello im new to react router or react, i have 2 router with param

<Route path='/myhome/:param' ... />
<Route path='/myhome/:param/:param2' ... />

How to match one route but not both?

sorahn commented 7 years ago

@rnYulianto if you wrap them with a <Switch> tag it will only ever render one route.

dnaploszek commented 7 years ago

Is this thread dead? I had these thoughts exactly, and was pointed here from there. hasChildren sounds great 👍

timdorr commented 7 years ago

See #5612

alex996 commented 7 years ago

@mjackson Why wasn't your brain working? I find it very counter-intuitive that the router doesn't perform exact matching by default. What's the argument against this idea?

r3wt commented 6 years ago

What's the argument against this idea?

@alex996 it was completely arbitrary from what i can tell. maybe an exercise in discovering what not to do.

imgnx commented 6 years ago

This ended up happening, didn't it? I am using React ^16.0.0 and I'm experiencing trouble when I use exact. Everything works fine without it. No Whoops404 pages but that's something that could be fixed with a server-side redirect. Why apologize with a "Whoops 404!" page when you can just fix the problem?

timdorr commented 6 years ago

@internationalhouseofdonald Nothing has changed. No code has been committed, or even PR'ed at this point.

ForbesLindesay commented 6 years ago

@internationalhouseofdonald How you handle the "no route matched" is a separate issue, it would still be up to the maintainer to choose to 404 or redirect, but without exact, you don't get the opportunity to do either.

medv commented 6 years ago

non-exact by default is better for my apps, because generally I have a few top-level routes that catch-all and then several catch-all routes as children and in turn childrens' children. I would imagine it's a common pattern of using this package because it allows nested components to behave as their own views that could be taken entirely out of context of the app and still work (given a root match url of the parent view component it's mounted in, or string url if it's mounted standalone). Children routes define their own path with params, so the parent view has a clean match to pass into these children for their own subnavs and is unaware of any functionality other than mounting child views.

exact/!exact default choice is arbitrary in the first place, so at least it's fewer lines of code the way it is right now. Across 6 apps, about one in ten routes are exact for me. I could see how I would need more exact routes if I had to state my routes from the get-go (outdated pattern for my liking). just 2c from what I experience.

image

TripView fetches by id from graphql if given id, or fetches last trip and redirects to trip/id, and all subnav+subviews within are mounted as standalone Routes or within a Switch. They are given TripView.match / TripView.location to extract params/location.state but also form its own subnav/subviews that might have their own params/location.state

image

This way the original top-level route still matches, and we also don't care that the sub-route is non-exact because it might have its own sub-views using the same pattern. You can take any piece of the chain and mount it in a completely separate app or Storybook story and pass them dummy parent matches, or written to just support base_url: string prop or similar. You can also remove any children routes and not affect the functionality of the containing component or route definitions anywhere above in the chain.

gnapse commented 6 years ago

To makes matters worse, you cannot even think of making your own Route wrapper that would put exact={true} in your routes, and then use that inside a Switch. Because the Switch relies on traversing its direct children list and finding out if each one of them is exact or not.

What if the Switch had a flag to this end? Like <Switch exact={true}>...</Switch> in which case it will assume each child route is exact? There must be something wrong with this idea, or maybe someone already posted it, because I find it deceivingly simple, yet I failed to found that suggestion in the discussion above, and I hardly can think I'm the first that came up with it. So what's wrong with it? Because I'm considering creating such a switch in my app.

stale[bot] commented 5 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

ForbesLindesay commented 5 years ago

How will these issues be resolved in the next version?

mjackson commented 5 years ago

The short answer is that in v6 all paths will match exactly by default. If you want to match only the beginning of a path, use a trailing *. This was suggested earlier in this thread, and is similar to how many other routing libraries do path matching.

v6 will not use <Route exact> and will probably throw if you use it. To help you migrate, we are planning on introducing an alternative API in v5 that has this new behavior. That way you can migrate your routes gradually over time, and we can add warnings to help you along. When there are no more warnings, you should be able to swap v5 out for v6 and everything will work.

ForbesLindesay commented 5 years ago

Thanks for the thoughtful answer :)

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

mjackson commented 4 years ago

For anyone who was following along here, this behavior is changed in v6 (currently in alpha). In v6 all <Route path> values are exact. If a <Route> has descendant routes, it should use a * on the end of its path so they have a chance to match.

There's a small section about using descendant routes in the getting started guide for v6 if you want to see an example. We'll be writing more about this in the tutorial and other guides for v6.