React: UNSAFE_* Lifecycle hooks don't run if getDerivedStateFromProps is present

[jquense]

https://codesandbox.io/s/xl7k7j1k6w

Not sure if this is intended or not, the longer term existence of these hooks suggested to me that they have their purposes still, so might be needed in conjunction with gDSFP. The particular use-case that caused me to discover this was a migrating: https://github.com/react-bootstrap/react-bootstrap/blob/master/src/Dropdown.js#L137 where document focus needs to be checked _prior_ to an update flushing because the update changes visual state and drops focus. I could move this to render() but React has traditionally yelled at me when doing ref stuff there. Admittedly in the migrated code i don't need findDOMNode so it'd be fine.

[aweary]

@jquense I believe this is intentional, to support the react-lifecycles-compat package:

https://github.com/facebook/react/blob/9d484edc4b64160cb335a23a2cb21667fb2cdf4c/packages/react-reconciler/src/ReactFiberClassComponent.js#L639-L646

Though maybe the intention was to just avoid calling the un-prefixed versions? It does seem like the UNSAFE_ methods should still be called.

[gaearon]

The particular use-case that caused me to discover this was a migrating: https://github.com/react-bootstrap/react-bootstrap/blob/master/src/Dropdown.js#L137 where document focus needs to be checked prior to an update flushing because the update changes visual state and drops focus.

Sounds like use case for https://github.com/reactjs/rfcs/pull/33, cc @bvaughn

[bvaughn]

Thanks for the CC @gaearon!

FWIW @jquense, checking in componentWillUpdate (or UNSAFE_ componentWillUpdate) is not really any better or different than checking in render as far as timing goes. Both are problematic for async though, which is why reactjs/rfcs/pull/33 has been created. I'd love to know your thoughts about this- (although I think I'll probably be changing it slightly soon to hopefully simplify it).

It is intentional that if getDerivedStateFromProps, none of the unsafe lifecycles (regardless of their alias) are invoked. It could be argued, I suppose, that the "UNSAFE_" ones should be run- but I think the existence of the new lifecycle implies that the old ones should have been removed- and I think it's reasonable for us to treat that as a case we warn about, rather than support.

[aweary]

I suppose, that the "UNSAFE_" ones should be run- but I think the existence of the new lifecycle implies that the old ones should have been removed

It makes sense to me to not call the un-prefixed versions, but if the UNSAFE_* methods are permanent it seems like they should be called regardless of other new lifecycle methods. What's the rationale behind not calling them in components that use getDerivedStateFromProps?

It feels like this might force users who still rely on some UNSAFE_ method to just avoid using getDerivedStateFromProps and keep using UNSAFE_componentWillReceiveProps as long as possible

[jquense]

Sounds like use case for reactjs/rfcs#33,

I just read through and yes it seems like exactly the right thing :P ya'll one step ahead.

but if the UNSAFE_* methods are permanent it seems like they should be called regardless of other new lifecycle methods.

This was my intuition. For the cases where they are appropriate it's not ideal to have to opt into UNSAFE for everything, when I feel like i can scope the componentWill* work to specific things that feel safeish? All that being said tho the getSnapshot case is the only one I can think of or have encountered where i don't feel like it's easy enough to migrate from, and if it's being added that seems to cover all the bases for me anyway.

[bvaughn]

but if the UNSAFE_* methods are permanent it seems like they should be called regardless of other new lifecycle methods.

Hm. I guess.

Still seems weird to me though that you could have a component with e.g. componentWillMount and getDerivedStateFromProps - in which case only getDerivedStateFromProps would be called - then you could run our lifecycle codemod so you have UNSAFE_componentWillMount and getDerivedStateFromProps- and now both get called.

Anyone else object to this change?

[bvaughn]

I think the two versions (legacy and unsafe) should be consistent, one way or another. If we decide to change the current behavior, maybe I'll change it back to the approach I used before where I only skip the will-lifecycle if the special _surpress attribute is set on it.

[gaearon]

but if the UNSAFE_* methods are permanent it seems like they should be called regardless of other new lifecycle methods.

I don't quite agree. I think presence of an UNSAFE method works like an opt into legacy mode (and thus turns off the new hooks). But I can see how this can be surprising or confusing. We do warn though.

[jquense]

We do warn though.

So I've only seen a warning for gDSFP mixed with cWRP. Actually the current behavior is nice for folks supporting all of v16 you can right both hooks and let react ignore gDSFP in new old versions...

[gaearon]

So I've only seen a warning for gDSFP mixed with cWRP.

You mean we don't warn for other combinations? That sounds like a bug.

Actually the current behavior is nice for folks supporting all of v16 you can write both hooks and let react ignore gDSFP in new old versions...

It's very easy to mess up. We intentionally wrote a polyfill and that's what we suggest using instead of trying to implement the correct semantics in every library.

[bvaughn]

Actually the current behavior is nice for folks supporting all of v16 you can write both hooks and let react ignore gDSFP in new old versions...

The way this should be done is using the react-lifecycles-compat polyfill. Not by defining both old and new lifecycles. The latter approach is problematic for a couple of reasons:

1. You're doing twice as much work to update state, since you would have to have the same logic in componentWillReceiveProps and getDerivedStateFromProps.
2. The will lifecycles were deprecated because they're "unsafe". Keeping them around and depending on them increases the likelihood of a shared component actually having async problems (and also triggering strict-mode dev warnings).

So I've only seen a warning for gDSFP mixed with cWRP.

We warn because this particular combination indicates a potential serious misunderstanding. The other two lifecycles (componentWillMount and componentWillUpdate) have their own strict-mode warnings, but not one specific to getDerivedStateFromProps.

I can see how this is not good enough though. If we don't execute those methods, we should explicitly warn about those combinations as well.

The take away from this issue then will be either:

1. Always execute both gDSFP _and_ the new lifecycles _unless_ the special polyfill __suppressDeprecationWarning flag is detected, or...
2. Broaden the dev warning about gDSFP and the old lifecycles being on the same component to also include cWM and cWU.

[gaearon]

Broaden the dev warning about gDSFP and the old lifecycles being on the same component to also include cWM and cWU.

This makes sense to me. The warning should fire whenever some of the methods don't get called, and it should explain that by using new APIs you lose the ability to use the old ones.

[bvaughn]

If we go with the broadened warning approach, what about a message like this:

Unsafe legacy lifecycles will not be called for components using the new getDerivedStateFromProps() API.

%s uses getDerivedStateFromProps() but also contains the following legacy lifecycles:
  componentWillMount
  componentWillReceiveProps
  componentWillUpdate.

(Where the specific list is dynamic, based on the legacy lifecycles the component actually contains)

[aweary]

I think the messaging around UNSAFE_* methods will need to be explained carefully, because it's not intuitive to introduce lifecycle methods that are both new and legacy. Especially since there's no precedence for conditional lifecycle method calls. This behavior also wasn't mentioned in the RFC.

I think presence of an UNSAFE method works like an opt into legacy mode (and thus turns off the new hooks)

This is reasonable for the 17 release, but in a minor I would expect that these new UNSAFE_* methods co-exist with the other new lifecycles unless you explicitly opt into strict mode using React.StrictMode.

[bvaughn]

Especially since there's no precedence for conditional lifecycle method calls.

Some form of conditional lifecycle is necessary in order for the polyfill to work- whether it's being driven off of the presence of the new getDerivedStateFromProps or __suppressDeprecationWarning.

This behavior also wasn't mentioned in the RFC.

The polyfill discussions happened after the RFC was closed. Generally, it's probably not possible to encompass all aspects of a feature in an RFC. A best effort attempt is made.

To be honest, I think I'm more a fan of option 1 above:

1. Always execute both gDSFP and the new lifecycles unless the special polyfill __suppressDeprecationWarning flag is detected, or...

But I'm open to feedback. I took the fact that you both 👍 Dan's comment to suggest I was in the minority with that opinion.

[aweary]

The polyfill discussions happened after the RFC was closed. Generally, it's probably not possible to encompass all aspects of a feature in an RFC. A best effort attempt is made.

Definitely! I'm just saying that most people who have participated in discussions around this feature aren't aware of this behavior since it wasn't part of the public feedback process.

Y'all have probably thought this through more than I have, but I think the behavior should look like:

16.3: call both deprecated and UNSAFE_ methods (except when a component defines both, call UNSAFE_ only), warn about components with both defined. Offer React.StrictMode as a way to enforce upcoming conditional lifecycle logic in 17

16.4: get rid of deprecated lifecycle methods Warn about using deprecated lifecyles

17.0: Get rid of deprecated lifecycle methods. Stop calling UNSAFE_ methods for components with other new lifecycle methods

To be honest, I think I'm more a fan of option 1 above:

I agree 👍

[bvaughn]

Regarding the 16.4 note above- we won't get rid of deprecated lifecyles in a 16.x release since that would be backwards breaking.

[aweary]

Oh yeah, my mistake. My wires got crossed, it should be a warning.

[bvaughn]

Sebastian and I just chatted about this at length, and I'm going to proceed with broadening dev warnings. I prefer option one (above) in _most_ cases, but Sebastian was concerned that it was less _safe_ in the case where people forked the polyfill into their source, and then codemodded it. In that case they would end up with e.g. UNSAFE_componentWillMount that has the __suppressDeprecationWarning. This muddied the 16-to-17 upgrade behavior.