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

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.

About this issue

Original URL
State: closed
Created 6 years ago
Comments: 19 (15 by maintainers)

Most upvoted comments

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.

gaearon on Mar 21, 2018

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.

bvaughn on Mar 21, 2018

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:

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.

bvaughn on Mar 21, 2018

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

aweary on Mar 21, 2018