Fix: make v5 Router compatible with v18 StrictMode

[jgoz]

Fixes #7870.

In React 18, Strict Mode attempts to detect unexpected side effects that might happen when rendering in concurrent mode. For class components, it intentionally double-invokes the constructor, render(), and shouldComponentUpdate(). It also double-invokes both layout and passive effects by mounting, unmounting, and then remounting.

Router in v5 was not compatible with this primarily because the main history subscription was created in the component constructor. Here is a simplified outline of the component lifecycle in StrictMode in v5.3.1:

• constructor — listen (this instance is not used)
• constructor — listen
• render
• render
• componentDidMount
• componentWillUnmount — unlisten(!)
• componentDidMount

At this point, there were no history subscriptions, so setState was never being invoked.

The fix presented in this PR moves the main history subscription into componentDidMount to make it resilient to unmount/remount cycles. Here is the new simplified outline as of this PR:

• constructor — listen (this instance is not used)
• constructor — listen
• render
• render
• componentDidMount — unlisten, listen
• componentWillUnmount — unlisten
• componentDidMount — listen

The listener in the constructor is only used for detecting redirects caused by child mounts and the subscription is only active until componentDidMount, at which point it is disposed and replaced with the main listener. Since the main listener is attached in componentDidMount, it will be recreated if the component is unmounted and then remounted without being fully reconstructed.

Tests

Rather than changing the local react dependency to v18, I added synthetic tests that simulate the new StrictMode behavior. If the maintainers would prefer to see tests using actual rendering in v18, I'd be happy to amend this.

[levibuzolic]

Just tested these changes on a large app at the moment and it looks to be working great. 👏

I've still got one small test case that's broken, but I suspect it's not actually relevant and probably pointing to a bug in our own codebase. Was a bug in our code.

[jgoz]

@timdorr 👋 friendly ping. Any thoughts on the approach I took here?

[timdorr]

I think this looks good. I'll merge it in and see if we can get this pushed in the next few days.

[d-pollard]

@timdorr any way we could get this in today?

[timdorr]

I'm dealing with a burst water line in my house, so I've been derailed a bit. I'm still planning on releasing this, though! Sorry for the delay in my end.

[d-pollard]

@timdorr no worries! Good luck with your burst water line, hope it gets fixed fast!

[timdorr]

OK, got it published now!

[francois-codes]

@timdorr looks like something went wrong when publishing this.
I see in the package.json of react-router-native that it has react-router: "2" as dependency.
This is crashing our apps as react-router@2 is trying to call React.PropTypes which no longer exists.

our work around was to add a resolution to force react-router >= 3.0.0 but it is likely that this version may cause issues

this is the problematic bit:

react-router/packages/react-router-native/package.json

Line 35 in 1a69e8a

"react-router": "2"

[timdorr]

Just fixed that in 5.3.3 😵

[ajtgt]

Just fixed that in 5.3.3 😵

Thanks