Guard against SSR leaks in <Router> constructor

[mjackson]

We should have a guard around the history.listen call in the <Router> constructor to be sure we are in a persistent environment first (see #6060 (comment)).

The ideal place for this listener would be componentDidMount, but we can't move it there and preserve backwards compatibility with existing code. It would be a breaking change, so we can talk about doing that in v5.

[timdorr]

Without looking at the code changes (on my phone), I will say in pretty confident you can listen inside of cDM and have it work with Redirect. We've been spending a lot of time on this exact issue with Redux (the last year, including multiple meetings with the React team) and have a solution we're implementing right now.

If anything, cDM is going to be required for forwards compatibility. If React decides to not mount a subtree after initialization, you can easily have weirdness around that subscription.

[frehner]

I don't know if this is the best solution, but in my PR I did a isStatic check for constructor and cDM and called listen depending on the situation.

[mjackson]

@timdorr I tried that route but we did have some failing tests with the way <Redirect> currently works in v4. e.g. we have a test for multiple redirects on the initial render. Now, you could argue that's a bad test and we don't actually want that behavior, and I would whole-heartedly agree. But there's no way we can get that test to work using cDM, and my goal with 4.4 is to have no breaking changes.

@frehner Ya, we should probably do exactly that. Thanks for pointing that out :)

[timdorr]

It's not so much that it's a bad test (multiple Redirects should definitely be handled), but that it's checking the implementation, not the effects. It's assuming a world where the React lifecycle is a one-time, synchronous thing. But that test would pass if it gave the React lifecycle a chance to run its course.

A simple fix to increase correctness would be to listen to the history and expect it to navigate through the redirect chain and then check what the render output is.

[mjackson]

Completely agree, @timdorr. I'm just concerned that someone might be relying on this behavior in a SSR scenario and we will break their app with 4.4 if we change it. Again, I'm totally down with changing this in v5.

[timdorr]

Prior to the 4.4 changes, we did some wacky stuff with checking if we were in a staticContext or not. We could do the same thing to maintain the same behavior. 🤷‍♂️