React-router: Unsafe Lifecycle warning in React >= 16.3

[morlay]

Version

react-router@create-context-api
[email protected] under React.StrictMode

Steps to reproduce

use react-router under React.StrictMode

Expected Behavior

without warning

Actual Behavior

warning for componentWillReceiveProps componentWillMount

[morlay]

For Router, we could write like this to fix the componentWillMount warning.
It works, but not sure is this a correct way.

https://github.com/ReactTraining/react-router/blob/react-context-api/packages/react-router/modules/Router.js#L79-L81

render() {
    const { children } = this.props;
    return (
      <RouterContext.Provider value={this.getChildContext()}>
        <HistoryListener history={this.props.history} onLocationChange={() => { this.setState(...) }} />
        {children ? React.Children.only(children) : null}
      </RouterContext.Provider>
    );
 }

class HistoryListener extends React.Component<{ history: History, onLocationChange: () => void }> {
  unlisten: any = null

  componentDidMount() {
    this.unlisten = this.props.history.listen(() => {
      this.props.onLocationChange()
    })
  }

  componentWillUnmount() {
    if (this.unlisten) {
      this.unlisten()
    }
  }

  render() {
    return null
  }
}

[pke]

This should be fixed in v3 too.

[timdorr]

@morlay No need to farm that out to another component. Instead, we can move to cDM and simply do a second check for if the location changed during that time.

Another option, if we wanted to go all-in on 16+, would be to use a setState callback function for the initial read from history.

[timdorr]

Easy next step: Upgrade to React 16.3 locally and add a failing test, like I did here: reactjs/react-redux#918

[morlay]

@timdorr
In StrictMode,constructor static getDerivedStateFromProps and render will be double-invoked.

constructor -> static getDerivedStateFromProps -> render -> componentDidMount
There are no opportunity to do the second check. So i think to move history listener to another component.

Then we can have
Router#constructor -> Router.getDerivedStateFromProps -> Router#render -> HistoryListener#constructor -> HistoryListener#componentDidMount -> Others Components >Router#componentDidMount

logic in HistoryListener#componentDidMount will be same as old Router#componentWillMount.

I tried providing another callback method in context to omit location changes or proxy history push replace methods.
It only handle changes from children of Router and will miss others changes from history itself, like navigating by redux actions.

[timdorr]

That's fine. We can add checks to avoid double execution.

[grzegorzjudas]

Is there a PR to fix this?

[gustavo-depaula]

I opened a PR, but I need someone to help me test this.

[frehner]

Created #6341 which hopefully is on the path of resolving this issue.

[mjackson]

This is fixed in master. You can see some of my commits here and here.

You can expect a new beta release sometime this week! 😅

[jaredpalmer]

WOOOO!

[morlay]

@mjackson
There will be double listened under StrictMode, and the first listener will be an un-controlled callback and may cause some issue when history changed.
https://github.com/ReactTraining/react-router/blob/master/packages/react-router/modules/Router.js#L42-L44

as I said https://github.com/ReactTraining/react-router/issues/6060#issuecomment-378432863

if we need to add listener in constructor, may try to unlisten before adding listener
js this.unlisten && this.unlisten() this.unlisten = .... ;

[timdorr]

Yeah, that listen call should be in a cDM. It's going to leak in server rendering too.

[mjackson]

Ah, good call @morlay. I'll fix in the next beta release.

@timdorr We can't use cDM because of the way <Redirect> works. But we can filter out the listen call based on whether we're in a persistent environment (the browser) or not.

[mjackson]

@morlay @timdorr Let's follow up in #6363