React-router: Improve docs when using dynamic routes under Route component

[engineersamuel]

I could be misunderstanding some interaction here, but if what I've observed is true, then I've encountered a pitfall which I believe needs to be documented more.

In the project I work on I have effective this:

        <Router history={history}>
            <Switch>
                <Route name="foo" path={`/foo`} component={FooComponent} />
                <Route component={({ match }) => <BarComponent />} />
            </Switch>
        </Router>

The BarComponent contains a list of Routes which are code split (which shouldn't matter for this)

        <div>
            <Header />
            <Sidebar />
            <Container>
                <Switch>
                    <Route name="a1" path={`/a1`} component={({ match }) => <AuthenticationWrapper componentName="A1" match={match} />} />
                    <Route name="a2" path={`/a2`} component={({ match }) => <AuthenticationWrapper componentName="A2" match={match} />} />
                </Switch>
            </Container>
        </div>

And now here is the problem. Within A2 there are tabs in that component, I handle the tables by a declarative route:

                <Route path={`/a2/:tab`} children={({match, ...rest}) => {

Long story short, whenever I clicked a tab, which contained a react-router-dom Link, I noticed every single component under the dynamic route (and in reality probably the top level Route) was being re-mounted (componentDidMount being called).

So after digging into the react-router docs I saw that when defining a Route with component, which I do at the top level:

When you use component (instead of render, below) the router uses React.createElement to create a new React element from the given component. That means if you provide an inline function, you are creating a new component every render. This results in the existing component unmounting and the new component mounting instead of just updating the existing component. For inline rendering, use the render prop (below).

What I did then, at the top level is instead of using component with the a2 component, I switched it to render and now no more re-mounts. I'm still not entirely clear that this is the right approach.

If this really is the right approach, and RRv4 is meant to be used with dynamic routing, then shouldn't the docs be very explicit in the children and render Route section and also in the example? From an outside perspective, it took a lot of searching in the issues section + docs + trial and error to finally realize that maybe I shouldn't be using component at the top level. Nearly every example out there uses component instead of render at the top level, so it makes it a bit confusing. It looks like: https://reacttraining.com/react-router/web/example/animated-transitions uses render at the top level, while using dynamic routing below, which makes sense now, but there is no explanation of that in the example itself, or why it was used that way.

Apologies for being long winded, I hope that by logging this issue I can also save some other people time when dealing with this issue as well. Having the entire sub-tree re-mounted on each route change, due to dynamic routes, is a performance killer.

So maybe, correct me if I'm wrong, what I'm looking for is confirmation and addition to the docs to the effect of: "If a top level Route ultimately contains dynamic Routes under it, then that top level Route should use render instead of component otherwise all children will be re-mounted when the url changes".

[engineersamuel]

Let me make one correction to the above, it appears that by simply avoiding the () => ... anonymous function within <Route component /> that the re-mounting is avoided. I think my request above then is still valid, this should be clarified, especially when using dynamic routing as it can cause a lot of re-renders.

[pshrmn]

<Route render> should only be used for inline components (when you need to pass variables from the local scope to the route's component). As far as I am aware, there are no examples that use inline components with the component prop (because that should not be done).

Is there anywhere in particular in the documentation that led to you using <Route component={() => ...}/>? There have been other issues/questions raised about this, so you're not alone, but it is difficult for me (as someone who has been fairly heavily involved with RRv4) to know what leads to people doing this, so any insight is helpful.

[engineersamuel]

@pshrmn Interesting question, I think it may just be a combination of things. As I know, solidly, that () => has ramifications in React, but for some reason that didn't click until recently with react-router.

Potentially what led me to use this for <Route component={() => ...}/> is that we were only passing match, not history or location, so we did <Route component={({match}) => ...}/> when in reality we should have (and are now doing:

function someComponent({match}) {
  return <SomeComponent match={match} />
}

<Route component={someComponent} />

We've had a lot of issues with connect and withRouter so we've worked to eliminate withRouter everywhere we can and just pass down match where applicable, or use dynamic routes, or rely on the history exported from createBrowserHistory.

Potentially one problem I see with the docs is that for more complicated setups, there aren't many examples. There are great examples for certain use cases, but say Redux + React Router + Code splitting, ect.. there don't seem to be solid direction for, so then it's a matter of piecing together examples from random projects on github, which all seem to do things a little differently.

[pshrmn]

@engineersamuel I added a section to my "basic components guide" PR (#4926) (specifically, this) that discusses the route render props.

Obviously it is a matter of discoverability, but if that existed and you had seen it when you were going through the documentation, do you think that it would have been enough to ensure you didn't have the issues that you did?

[engineersamuel]

@pshrmn Awesome, thank you for adding a section, I think the one key sentence is, "You should not use the component prop to pass in-scope variables because you will get undesired component unmounts/remounts," but I may suggest adding the works "inline function" in there as a keyword, otherwise it wouldn't necessarily be clear unless you really focused on the example or intuitively knew that "pass in-scope variables" implied an inline function. I do hope with this new documentation + this issue (which is searchable) it will definitely be more discoverable to those in the future.

[vinnymac]

@engineersamuel thank you for making this issue. As it led me to finding a solution for a very related problem.

If you find something like this in a project:
<Route path="/users" component={authenticate(Users)} />
where authenticate is a simple HOC wrapper like:
(C) => class extends React.Component {}

this will have the same behavior that you speak about in your OP. Fortunately both the docs and this issue helped me discover exactly what was causing the unmounting/mounting behavior. Simply moving the authenticate HOC to within the Users file fixes this. I hope this helps others who might glance over a HOC in a Route that might go overlooked and become detrimental in the rendering lifecycle.

The docs specifically state the case for not using an inline function with the component prop, but it isn't so obvious when it is simply a wrapper like a HOC.

It makes me wonder if we could somehow detect in RR when a user of the component Route prop is in development mode and provide them a warning when they are dynamically creating a component rather than passing a static one in.

[engineersamuel]

@vinnymac Awesome! Yes one of my motivations in creating this was to hopefully give some visibility to this edge case and how to fix it, glad you stumbled across this.

[vinnymac]

@engineersamuel I think we can actually fix this issue rather gracefully.

If we modify Route.js to include a new warning on componentWillReceiveProps we can help people prevent unnecessary rerenders.

I could put together a PR, it would look something like:

warning(
      !(nextProps.component !== this.props.component),
      'You should not use an inline function with <Route component>. Try using <Route render> instead if you need dynamic renders.'
    )

[timdorr]

@vinnymac Unfortunately, that would warn you if you had something like <Route component={loggedIn ? NewsFeed : IntroPage}/>

[vinnymac]

@timdorr if that is a supported use case then I don't think we can provide a useful warning to people who end up providing inline functions and wrappers. It seems improved documentation is all we can offer for this particular issue.

[timdorr]

It's not so much that it's supported, but more that it's possible, so folks will do it. And there's nothing invalid about that pattern, it's just weird.

It will also commonly happen if they have some sort of async loading component, which will change the component once it loads. That would be where I'd see this more likely to happen.

[moneytree-doug]

It would be really nice if there were more documentation around dynamic routing. It's still very ambiguous.

[timdorr]

I feel like we've done this already with the documentation. However, for docs issues, a PR is really the only way to go. Issues just point out problems and offer no solutions. We need solutions; we need PRs.