Removal of React 16.3 unsafe lifecycles

[gustavo-depaula]

This is intended to resolve #6060.

The warning that was activated in componentWillReceiveProps was moved to component shouldComponentUpdate, and the this.unlisten function is now being set in constructor.

WARNING: The unit tests are all green, but I was not able to run a real life setup (at the time of this writing, accepting help) to test this. If someone can test this with React.StrictMode, I would be glad.

[timdorr]

This change isn't needed. The class field above transpiles/desugars into this exact same form.

[gustavo-depaula]

makes sense!

[timdorr]

Because we're mainly doing a subscription here, we should be following the React async guidelines and move the listener to a cDM: https://reactjs.org/blog/2018/03/27/update-on-async-rendering.html#adding-event-listeners-or-subscriptions

I'm not sure where the best place for the invariant check would be. Possibly just the render method.

[renatoagds]

@timdorr I think that invariant could stay in constructor and just move the subscription for cDM, since, IMO the invariant needs to be the first thing to do here.

[grzegorzjudas]

Technically, you could change the children of <Router /> when component is already rendered, causing it to re-render. If it's in constructor, you won't get the message then. Possibly static getDerivedStateFromProps again? As it's called every time component receives props.

[renatoagds]

@grzegorzjudas That's a good point. I think we could follow with getDerivedStateFromProps for invariant, since it's called in Mounting and Updating phases.

[timdorr]

I think this will have an unintended negative perf side effect. While not the worst thing in the world (we don't do a whole lot in the render), technically this will cause this component to render on every update, even if it's unneeded.

While the error message will need to change (we're not preventing the change, only warning that it will have no effect), I believe this actually belongs in cDU: https://reactjs.org/blog/2018/03/27/update-on-async-rendering.html#side-effects-on-props-change

[renatoagds]

@timdorr maybe we could use getDerivedStateFromProps here? It could guarantee that we're warning before the render.

[grzegorzjudas]

I think that's exactly what is suggested (to use getDerivedStateFromProps I mean) - article.

static getDerivedStateFromProps(nextProps) {
    warning(
        this.props.history === nextProps.history,
        "You cannot change <Router history>"
    );

    return null;
}

[timdorr]

You can't use this in a static function.

[renatoagds]

@timdorr @grzegorzjudas following what is proposed here (https://github.com/reactjs/rfcs/blob/master/text/0006-static-lifecycle-methods.md#state-derived-from-propsstate), I think we should mirror history in state and compare it in getDerivedStateFromProps, using the second parameter prevState.

[timdorr]

gDSFP is intended to be a pure function, so I don't think that would work. cDU might be a better option.

[grzegorzjudas]

@timdorr Ah, right, my bad.

[grzegorzjudas]

@gustavo-depaula are you planning on making the changes according to feedback? I think it should be clear by now from the discussion what changes we're talking about:

• Using componentDidUpdate instead of shouldComponentUpdate on line 68
• Move the invariant/warning from constructor to static getDerivedStateFromProps

[gustavo-depaula]

@grzegorzjudas I'm planning on doing changes. I'm onboard with moving from shouldComponentUpdate to componentDidUpdate, but gDSFP is intended to be a pure function. So I do not agree with that.

[grzegorzjudas]

What would make a following function non-pure:

static getDerivedStateFromProps (nextProps) {
    invariant(
        nextProps.children == null || React.Children.count(nextProps.children) === 1,
        "A <Router> may have only one child element"
     );
}

[pshrmn]

If the React version is required to be 16+, the single child invariant() call should be unnecessary (see #5712).

[timdorr]

The subscription can't go in the constructor. If the component is removed from the tree before rendering (which may happen in async rendering), we'll leak a subscription to the history instance. The subscription needs to be put in a componentDidMount.

[frehner]

Trying to catch up on this to see if I can help -- is the only thing left to do is change the stuff in constructor to componentDidMount?

[frehner]

I've created a new PR #6341, which should update all locations that were using the outdated lifecycles. It also is passing all tests.

[mjackson]

Thanks for the PR, @gustavo-depaula :)

I did some major refactoring over the past week and eliminated all the deprecated componentWill methods in the process, so we won't need this anymore. You can see some of my commits here and here.

My apologies for not guiding you through this work so we could get this merged, but it was a little tricky due to the context API changes and I wanted to make sure I understood all of the implications of making this change, so I decided to do it myself. You can expect this to be published in our next beta release sometime this week. 😅