React-router: Issue with <Miss /> and SCu interference

[alisd23]

There seems to be an issue with the publish/subscribe functionality when rendering misses and registering matches. because if an element returns false from shouldComponentUpdate it prevents the Miss rendering until the _next_ time the location changes.

In this simplest case:

//...
const App = () => (
  <div>
    <Link to='/not-a-url'>404 me</Link> */}
    <Match pattern='/' exactly component={Home} />
    <Miss component={NotFound} />
  </div>
)

class Blocker extends Component {
  shouldComponentUpdate() {
    return false; // For whatever reason i.e. connect seeing store has not changed
  }
  render() {
    return <App />
  }
}

ReactDOM.render(
  <HashRouter>
    <Blocker />
  </HashRouter>,
  document.getElementById('container')
);

I recreated it on codepen to test it too

Miss does not render on transition to /not-a-route, but when transitioning BACK to / it renders (incorrectly). It always seems to be 1 route behind.

So far I've tracked it down to the fact that the top level <MatchProvider /> (inside StaticRouter) calls notifySubscribers before <Match /> calls removeMatch on the match context here, because it unmounts after the top level <MatchProvider /> updates.

[alisd23]

I think there must still be some context changes propagating down causing re-renders - which means Miss and Match are not entirely dependent on the broadcasting, which they probably should be. If the updates were always caused by the broadcast, adding in a shouldComponentUpdate => false would not affect anything.

[ryanflorence]

When I added the LocationSubscriber to Match and Link I forgot about Miss.

[ryanflorence]

Changes happened here bb7d8ee2af9f67744a3c9a483a0476e307930ca1 then ce59676cb13bb8e016a72e7230e1b569594aa4cc

Got time to take this @alisd23?

[alisd23]

I was playing around with this example yesterday and I'm still not 100% sure it's the lack of LocationSubscriber alone which is causing this issue, as adding that in didn't fix this example. I'll dig into it more today.

[alisd23]

Ok here's what I've done/found in the above example when clicking on the '404' link.

• Added LocationSubscriber to Miss in the same way as Match - didn't fix the issue

Rendering tree looks roughly like this:

BrowserRouter
  BrowserHistory
    StaticRouter
      LocationBroadcast (*causes the subscribers to re-render on location change*)
        MatchProvider
        ---------------------- (*children of MatchProvider re-render on location change*)
          Blocker
            App
              Match [pattern='/'] [exactly]
              Miss

Without a Blocker (usual case)

• Location change causes the MatchProvider in StaticRouter to render children again _and_ broadcast the new location to any subscribers.
• This render causes a full top down render (tree-diff).
• componentDidUpdate for Match triggers - calling removeMatch on the MatchProvider in StaticRouter
• Then componentDidUpdate triggers on the MatchProvider in StaticRouter, which notifies the Miss component of any match changes. Miss then renders correctly.

With a Blocker

• Location change causes the MatchProvider in StaticRouter to render children again _and_ broadcast the new location to any subscribers. (Same as above)
• Top down diffis stopped at <Blocker />, so match does not render at this point.
• The MatchProvider in StaticRouter finishes rendering so componentDidUpdate is called, causing Miss to be notified of the match situation. However Match has not yet called removeMatch as it hasn't updated yet, so Miss still thinks there is a match, so it does not render, incorrectly.
• The setState in the LocationSubscribers in Match and Miss (from the when the location change first happened) then causes them to re-render. (Because setState is asynchronous this happens after that initial synchronous top-down render).
• Match then finishes rendering and calls removeMatch on the MatchProvider in StaticRouter, however there is nothing to tell Miss that something has changed because the MatchProvider has already finished rendering.

---

Basically, the top-down render of the MatchProviders children caused by the change in location has to happen currently because it forces Match to call addMatch or removeMatch before the parent MatchProvider's componentDidUpdate is called (so subscribers are notified with the correct match info).

But when a blocker exists this top-down render can be stopped half way, so the addMatch or removeMatch isn't called in time, which breaks Miss.

If we want this solved there needs to be another way to allow Match components to notify the parent MatchContext that their match situation has changed, OR change the way MatchProvider notifies subscribers.

Not sure about a proper solution yet, but thought I'd write this down before I forget everything...

[ryanflorence]

thanks, this is great, I'll take a look soon

[ryanflorence]

@alisd23 think you can make a failing test first?

[alisd23]

Yep I'll create one tomorrow

[alisd23]

@ryanflorence I think the MatchGroup I've seen you mention might solve this problem, if the idea is something like this:

<MatchGroup>
  <Match pattern='/' component={Home} />
  <Miss component={NotFound} />
</MatchGroup>

Maybe worth creating an issue for MatchGroup to open up the discussion?

[pshrmn]

I took a stab at this by making sure that the <MatchProvider> has access to the current location, which it uses to inform all of its <Match> and <Miss> children. It passes the "FAILING MISS TEST" that @alisd23 included in #4047. I'm not sure that it is the approach that you want to take, but if you're curious, this is the commit: https://github.com/pshrmn/react-router/commit/e63a17195789a2839cb4a7606817dcaef8355369

[alisd23]

@pshrmn Looks like a good way to go! I've been playing around with some scenarios with your fork, and there's a few other cases to consider - but they are solvable. Also there's some general things to review. Maybe worth putting in a pull request as a WIP so we can start reviewing specific things and discuss cases that need to be covered?

[kyeotic]

I ran into this issue when following the ambiguous matches example. For those who need a workaround, this is what I am doing.

Failing ambiguous match

<Match pattern='/projects/:projectName' render={(matchprops) => (
  <div>
    <Match pattern='/projects/new' component={ProjectCreatePage} />
    <Match pattern='/projects/:projectName/versions' component={ProjectVersionsPage} />
    <Match pattern='/projects/:projectName/settings' component={ProjectSettingsPage} />
    <Match pattern='/projects/:projectName/collaborators' component={ProjectMemberPage} />
    <Miss render={() => <ProjectEditPage {...matchprops} />} />
  </div>
)} />

Working ambiguous match

<Match pattern='/projects/:projectName' render={(matchprops) => (
    matchprops.params.projectName === 'new'
    ? <ProjectCreatePage {...matchprops} />
    : <ProjectEditPage {...matchprops} />
  )} />

<Match pattern='/projects/:projectName/versions' component={ProjectVersionsPage} />
<Match pattern='/projects/:projectName/settings' component={ProjectSettingsPage} />
<Match pattern='/projects/:projectName/collaborators' component={ProjectMemberPage} />

[ryanflorence]

sCU is fixed in miss 3423b1d939425356e6656f0dff62e386bda3461a (but not released yet)

[alisd23]

This is definitely still failing with alpha-5. Try clicking around this codepen example: http://codepen.io/alisd23/pen/RGJWjJ

In particular try clicking 404. You'll see the 404 page doesn't render, but if you click 404 again, it does.

[alisd23]

_Summarizing previous findings:_

The underlying issue here is that on location change <StaticRouter> will re-render its children (effectively rendering the whole app). The cDU's called in this pass are what allow the <Match> components to notify any parent <MatchContext>'s that their matching state has changed, and therefore correctly render the <Miss> components.

However we can't guarantee that all of the <Match>/<Miss>'s (and therefore their cDU's) will run in this render pass (due to the fact that any component implementing sCU will block the render pass from going any deeper), so any 'Blocker' breaks the <Miss> API.

NB:
I think this is another PR to the same issue: #4146

[taurose]

I just did some investigating as well, and can mostly confirm what @alisd23 said here.

One remark about the blocker case though: Not considering the outdated match state passed to Miss, both Miss and RegisterMatch are scheduled to update via setState in the first render pass. However, React updates all scheduled (dirty) components in a single pass, i.e. RegisterMatch.didUpdate will always fire after Miss.render (no matter their order in the tree or the order of setState calls).

So even if Miss.render pulled the most recent match state from MatchProvider via context, it still wouldn't work. Neither would it help if RegisterMatch.willReceiveProps did the updating (only if Match came before Miss in the render tree).

The only way I see this work is to have Miss update in response to a RegisterMatch update, leading to three render passes in total. And then, to fix #4146 (i.e., too early updates of Miss), we'd need to somehow use setState and sCU to delay rendering Miss until all Matches have rendered, if that's even possible.

I'm a big fan of plan B (MatchGroup) though. Let's call it plan A2 :smile:

[mjackson]

<Miss> is gone in current v4. Additionally, we've addressed sCU issues using our withRouter HOC that uses a subscription model to get around blockers.