withRouter: Change of location doesn't trigger a re-render

[SamyPesse]

When a deep component uses withRouter, because of how React handles context (facebook/react#2517), the component is not updated when the location changed.

The main reason is because the location value is directly provided by the context. When this value change on the top level component, the deep component doesn't re-render if one component of the stack has a shouldComponentUpdate.

One solution may be to provide a constant subscribe function in the context instead of the value it self.

Current solution:

react-router/modules/Route.js#L36

class Route extends React.Component {
  ...

  getChildContext() {
    return {
      router: {
        ...this.context.router,
        route: {
          location: this.props.location || this.context.router.route.location,
          match: this.state.match
        }
      }
    }
  }
}

Proposed solution

Instead of having router in the context, provide a subscribeRouter function. This is a solution similar to what react-sticky is doing.

Here is an example:

class Route extends React.Component {
    ...

    constructor(props) {
        super(props);

        // Array of functions
        this.subscriptions = [];

        // Sorry crappy code ...
        let first = false;
        this.context.subscribeRouter(router => {
            if (first) {
                this.state = {
                    router,
                    match: this.computeMatch(props, router)
                };
            } else {
                this.setState({
                    router,
                    match: this.computeMatch(props, router)
                }, () => {
                    this.triggerSubscribers();
                });
            }
        });
    }

    getChildContext() {
        return {
            subscribeRouter: (fn) => {
                this.subscriptions.push(fn);
                this.triggerSubscriber(fn);
            }
        };
    }

    triggerSubscriber(fn) {
        const router = {
          ...this.state.router,
          route: {
            location: this.props.location || this.state.router.route.location,
            match: this.state.match
          }
        };
        
        fn(router);
    }

    triggerSubscribers() {
        this.subscriptions.forEach(fn => {
            this.triggerSubscriber(fn);
        })
    }
}

---

If you agree with the proposed solution, I can try a PR.

[pshrmn]

React Router intentionally does not use subscribers.

There is a somewhat long article I wrote about why here https://medium.com/@pshrmn/ditching-subscriptions-in-react-router-6496c71ce4ec, but the gist is that subscriptions end up making the code base overly complicated without much benefit.

You end up having to write your own scheduler to ensure that you don't get duplicate re-renders for one location change. Context usage also ends up a bit hacky because you have to manipulate the same object (Object.assign) to ensure that the child components that are subscribed are accessing the current context object.

Instead, React Router relies on updates propagating from the router to all of its children. If you are using update blockers (PureComponent or shouldComponentUpdate), you should refer to this guide to see how to deal with them in a React Router application (you just pass them the location as a prop so that shallow checks detect the change).

[tnrich]

Hmm I feel like this is a pretty big gotcha the way things are currently set up! Basically we're told to use withRouter whenever we want to react to the route changing... BUT, in certain cases where you have a nested use of withRouter it just won't work.... that seems very bad, right?

I for one am definitely going to shift to some other solution than withRouter. Too bad though because other than that defect it seemed great.

Also of note, I read the following warning and didn't understand it because there is an important comma missing!

It should read: If you are using withRouter COMMA to prevent updates from being blocked by shouldComponentUpdate ...etc

Or better yet, get rid of the first part and just start the sentence: To prevent updates from being blocked by shouldComponentUpdate ...etc