Updated history subscription in ConnectedRouter

[klis87]

Moved history subscription from componentWillMount to componentDidMount to avoid memory leaks during server side rendering.

[timdorr]

Thanks!

[Benjamin-Dobell]

@klis87 Where is the memory leak that this pull request purportedly fixed?

It's true that when using SSR we're adding a subscription without explicitly removing it, however how is that a memory leak? Your history objects are created and garbage collected before/after each request. The listeners are not global, they're simply added to a scoped array, which will itself be garbage collected:

https://github.com/ReactTraining/history/blob/master/modules/createTransitionManager.js#L49

This change broke the core functionality of ConnectedRouter (#5655).

The listener was added in componentWillMount and not componentDidMount for a reason. The first render happens before componentDidMount, with this commit merged we miss our callback (and dispatching actions) for any <Redirect> rendered as part of the initial render.

Additionally, as someone using SSR, I want this listener registered on my server. We need this action dispatched so we can inspect the redux store and perform server-side 302 redirects.

@timdorr Could you please role this back and push a new beta release to NPM? You should then be able to close off #5655.

[josepot]

I completely agree with @Benjamin-Dobell this PR adds more issues than it tries to solve. Another example is that the following test won't pass:

  it('redirects properly', () => {
    expect(store.getState()).toHaveProperty('router.location', null)

    renderer.create(
      <ConnectedRouter store={store} history={history}>
        <Switch>
          <Route path="/test" render={() => null} />
          <Redirect to="/test" />
        </Switch>
      </ConnectedRouter>
    )

    expect(store.getState()).toHaveProperty('router.location.pathname', '/test')
  })

Because the subscription will take place after the redirect. I think that this should be undone, I'm currently preparing a PR to undo this, while adding this test.

[timdorr]

The listener is never cleaned up on an SSR render. renderToString will run componentWillMount, but never runs componentWillUnmount. Moving that to componentDidMount solves that problem.

[timdorr]

That's also why we noop listening in <StaticRouter>

[Benjamin-Dobell]

@timdorr What do you mean by the listener is never cleaned up? Is it not garbage collected along with everything else (all the rendered components etc)? What led you to believe otherwise?

The only reason it won't be garbage collected is if there's a global (or active) reference back to the history object or the ConnectedRouter component - no such reference will exist. A new history is created per request.

EDIT: Note that browsers (and JS VMs) use garbage collection and resolve cycles, not ARC (like iOS). The fact that history and ConnectedRouter hold a reference to each other is irrelevant, they're orphaned.

[timdorr]

That may be some legacy thinking from 2.0/3.0. I had an app that maintained a common history instance to avoid some creation overhead on each request. I just reset entries each time to get it into the right state.

The downside of this leaving hanging subscriptions does remain. That can cause problems if you rely on the history instance after rendering. But that's fairly uncommon at this point.

[Benjamin-Dobell]

@timdorr I guess if you're wanting to support reusable history between requests then the most reliable solution would be to modify history to have an unlisten/unlistenAll style method that removes all subscribers. Then that method can simply be called when resetting the history object/entries. Alternatively you could simply add a reset method that does everything you've described.

I think in general the reuse of state between server-side renders is a bit of an anti-pattern, however if that is being done with history, then ideally I think that ought to be a concern of the "history" project and not "react-router".

[timdorr]

Alpha 9 is up. We'll get this right at some point...

[Benjamin-Dobell]

@timdorr Woah! That was quick. Awesome maintenance, very much appreciated!