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

Fix <Route children> #6362

Closed
mjackson opened this issue Oct 1, 2018 · 25 comments
Closed

Fix <Route children> #6362

mjackson opened this issue Oct 1, 2018 · 25 comments
Labels

Comments

@mjackson
Copy link
Member

mjackson commented Oct 1, 2018

There was a lot of discussion in #5583 about removing the <Route children> prop and adding a new prop about when to render. The conversation in that issue stalled, so I'd like to resume it here, but with a completely different proposal.

The core issue is that it's not very clear what the difference is between <Route children> and <Route render>. The API doesn't really say anything about when to use one over the other.

In contrast to the proposal in #5583 (remove children and add a new prop about when to render) I'd like to propose that we just tweak the way children works and get rid of render (eventually). Here's why:

Currently, <Route children> always renders, which doesn't make any sense. Wanna see something crazy?

<Route path="/about" children={() => (
  <About />
)} />

// or even

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

You might think these routes only render when the URL path is /about, but nope. They both will render at any URL. 🙃 This is a huge mistake in the v4 API, and I'd be very surprised if anyone is actually using <Route children> like this.

The reason this API exists is because you're supposed to check the match prop to see if the <Route> matched before rendering anything, e.g.:

<Route path="/about" children={props => (
  props.match ? <About /> : <NotFound />
)} />

So now if the URL is /about you'll get the <About> page, otherwise <NotFound>. That at least makes a little more sense than the previous examples, and hopefully redeems us a little. But it doesn't change the fact that the first examples are just crazy.

The vast majority of the time you want to render something only when the route matches the URL, and ignore misses. So we added the <Route render> and <Route component> APIs that will only render something when it matches. These are used much more commonly than <Route children> in all our docs and examples.

The Proposal

It's quite simple, really: The default behavior of children should be to only render something when the route matches the URL. We don't need any new API to <Route>, and we definitely don't want to get rid of the children prop. We just need to fix it.

What about misses?

The majority of the time someone wants to handle a "miss", they actually want to render a "not found" page. We actually already have great support for this inside a <Switch>:

<Switch>
  <Route exact path="/" component={Home} />
  <Route path="/about" component={About} />
  <Route component={NotFound} />
</Switch>

For the rare use case that someone wants to render a standalone <Route> that does not match the URL, we already provide the matchPath function. If we want a component API, we can provide a lower-level component, call it a <Match>, that will just compute the match and tell you whether or not the path matched:

<Match path="/about">
  {match => match ? <About /> : <NotFound />}
</Match>

Pros:

  • <Route children> is a lot more intuitive
  • Since <Route children> would behave the same as <Route render>, we can eventually remove <Route render> (less API is almost always a good thing)
  • If a <Route> always matches, it will be a lot easier to implement relative <Route>s and <Link>s (see RFC: Relative Links and Routes #5127)
  • It should be easier (but still not super easy) to wrap a <Route> now with your own component, since you have one less render prop to worry about

Cons:

  • It is technically a breaking change for what I suspect is a very small percentage of our users
  • ??

The Plan

  • 4.x: Add a <Match> component and encourage everyone who is using children to render something when a <Route> doesn't match a URL (probably a very small % of users) to use a <Match> instead. Unfortunately, there's no easy way for us to warn users to stop using children for non-matches.
  • 5.0: Make the switch so <Route> only renders something when it matches. I would expect this to break very few apps, but hey, you never know. Also, keep the render prop around for a while to ease the upgrade.
  • 5.x: Deprecate the render prop since it's identical to children now and warn people to just use children instead.

Thoughts?

@bradwestfall
Copy link
Contributor

If the eventual plan is that <Route render> and <Route children> are effectively the same and you'll be removing one (because of the smaller API), my vote is to remove <Route children> because it's probably the one of the two used the least now (so this will require less updates for people using RR), plus, this is just a personal preference, but I like render props with an actual render prop over using children as a render prop. Maybe poll twitter for preference of <Comp children> vs <Comp render> when it comes to render props?

But either way, seems like a good plan.

@mjackson
Copy link
Member Author

mjackson commented Oct 1, 2018

Thanks for the feedback, @bradwestfall :) The reason I like children, especially in this case, is because it's more flexible than render because there are 2 different syntaxes for the children prop in JSX.

<Route path="/about" children={() => (
  <About />
)} />

// or:

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

vs

<Route path="/about" render={() => (
  <About />
)} />

Plus I'm seeing the community settle more on children than render as far as prop name goes (e.g. the new context API).

@rgdelato
Copy link

rgdelato commented Oct 1, 2018

As a rebuttal, I'd prefer to see children stay over render (and I don't necessarily think a Twitter poll is a great way to determine an API). But yeah, the proposed changes look great to me, including the addition of a <Match> component that just gives its children the result from matchPath.

@bogdansoare
Copy link
Contributor

I think it's a good path forward, also the usage of children over render makes sense 👍

@TrySound
Copy link
Contributor

TrySound commented Oct 1, 2018

@mjackson I often use Route only to get history from context. I think it would be nice to export original context which could be reused in a few ways.

Class contextType api

const Component = class extends React.Component {
  contextType = RouterContext;
  handler = () => {
    this.context.history.push('/users')
  }
}

or context.unstable_read api

const Component = () => {
  const { history } = RouterContext.unstable_read();
  return <div onClick={() => history.push('/users')}></div>
}

@mjackson
Copy link
Member Author

mjackson commented Oct 1, 2018

@TrySound I feel like whether or not we export context is a slightly different discussion that probably deserves its own issue. Please feel free to open another one :)

@TrySound
Copy link
Contributor

TrySound commented Oct 1, 2018

I mean this should be considered too since you remove this possibility from <Route children />.

@mjackson
Copy link
Member Author

mjackson commented Oct 1, 2018

@TrySound You can always render a <Route> that will match any URL by using either path="*" or no path at all:

<Route children={({ history }) => (
  // off you go!
)} />

@TrySound
Copy link
Contributor

TrySound commented Oct 1, 2018

Ah, I get it. So no actual breaking changes in my app. I opened an issue about more ways to access context.

@gnoff
Copy link

gnoff commented Oct 1, 2018

@mjackson will it be valid to render a Match inside of a Switch? what would the Switch behavior be if the Match object did not actually match the path?

@timdorr
Copy link
Member

timdorr commented Oct 1, 2018

Switch reads its children's props directly and chooses which one to render. You can actually put anything under a Switch, as long as it has the right props. I wouldn't advise it, but it is possible.

So the true behavior of Switch wouldn't change. It might mess with your perceptions of what it does, but that would be up to us to provide better documentation.

@gnoff
Copy link

gnoff commented Oct 1, 2018

right, my question was does it render the first renderable thing (the Match) regardless of whether the Match actually matches? it kind of breaks the notion of switching since it will always claim the render slot so to speak

@mjackson
Copy link
Member Author

mjackson commented Oct 1, 2018

@gnoff I tend to think of <Match> more like an if statement. What's your use case?

@gnoff
Copy link

gnoff commented Oct 1, 2018

Oh i don't have one, just thinking we should explicitly scope Switch to Route components then and forbid this use. Just exploring the API. I guess the problem/feature already existed

@gnoff
Copy link

gnoff commented Oct 1, 2018

Ah re-reading the Switch src I see Match and Route would basically work the same when the a Match matched the route and Match would not be rendered at all if the match failed. So there is no point in using Match inside of Switch but if you did you would get new Route behavior. Seems harmless but maybe worth identifying for people in __DEV__ mode

@mattcolman
Copy link

mattcolman commented Oct 4, 2018

https://greensock.com/react shows a good use case for rendering a route even when it doesn't match the url. See route animation demo.
In regards to your proposal is it odd that if you want to animate routes you will need to use <Match/> instead of <Route/>?

bpedersen added a commit to bpedersen/react-router that referenced this issue Oct 9, 2018
@bpedersen
Copy link

I made a PR for this issue. I may have misunderstood the idea, but my take was that we create a <Match> component that doesn't call/render the children prop if the path prop doesn't match. Let me know if that is not the intention and I can make the changes.

@gnoff
Copy link

gnoff commented Oct 9, 2018

@PiereDome I didn't review in detail but it seems you've got Match and Route inverted. Route would be updated to remove render as a prop and only support children. children would then be updated to work like render where it only returns if the Route matches

Then Match would be written to work like Route does today where it always renders and the user can opt to handle match and non-match cases as they wish

the smallest delta is probably something like

  • clone Route to Match and remove render support
  • update Route by removing existing children support and rename render to children

@mjackson
Copy link
Member Author

@mattcolman Thank you for mentioning animation and linking me to the animation example on GSAP. I hadn't seen that before.

This change won't require you to use a <Match> for all of your animations. Instead, you should be able to continue to use <Route>s as we already do in our animation example. Instead of doing the animating inside the <Route>, just wrap your <Route> or <Switch> in a <Transition> element and you'll be fine.

Remind me to include a GSAP example when we ship this... 😅

@stale
Copy link

stale bot commented Sep 10, 2019

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.

@stale stale bot added the stale label Sep 10, 2019
@mjackson mjackson removed the stale label Sep 11, 2019
@StringEpsilon
Copy link
Contributor

StringEpsilon commented Sep 18, 2019

I'm a bit surprised this made it to master, as this a pretty major / breaking API change.

Should I revive my <Match /> PR?

@MeiKatz
Copy link
Contributor

MeiKatz commented Sep 18, 2019

@StringEpsilon Maybe we don't need a separate <Match /> component at all: we could also add a validate prop to <Route /> where you can determine if a route matches or not. I think this api is much clearer than a separate <Match /> component.

// using a pseudo api as parameters (has to be defined)
<Route validate={({ location }) => location == "/admin" && isAdmin(user)}>
  <AdminPanel />
</Route>```

@mjackson
Copy link
Member Author

The only thing that made it into master is that we now only render <Route> children elements if the route matches. That was always the intention, so we aren't considering it a breaking change. It's more of a bugfix, really.

Before:

<Route path="/home">
  {/* This always rendered, even with the path was not /home, which was a bug */}
  <HomePage />
</Route>

After:

<Route path="/home">
  {/* This only renders when the path matches */}
  <HomePage />
</Route>

@ryanflorence
Copy link
Member

ryanflorence commented Sep 19, 2019

Additionally, before this commit:

<Route path="/home">
  <HomePage/>
</Route>

Is identical in behavior to:

<HomePage/>

That doesn't make any sense to have as an API. We already have <Fragment> if you want wrappers that don't do anything (which of course is confusing why you'd want to do that anyway).

@maciekmp
Copy link

maciekmp commented Oct 3, 2019

The reason of code above was strictly explained in the docs. So still I think this behavior change should be introduces as a breaking change and docs should be updated.

@lock lock bot locked as resolved and limited conversation to collaborators Dec 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests