Invoke both legacy and UNSAFE_ lifecycles when both are present #12134
Conversation
This is to support edge cases with eg create-react-class where a mixin defines a legacy lifecycle but the component being created defines an UNSAFE one (or vice versa). I did not warn about this case because the warning would be a bit redundant with the deprecation warning which we will soon be enabling. I could be convinced to change my stance here though.
|
I think it's confusing that in some cases mixing "old" and "new" works (UNSAFE_cWRP + cWRP) but in other cases it doesn't (gDSFP + cWRP). Did you consider always enforcing just one case? e.g. you either have cWRP, or UNSAFE_cWRP, or gDSFP, but never a mix, otherwise throw? Are there downsides to this? |
Yeah, I see that POV. I think it's slightly different though, since the presence of gDSFP indicates that a human took the time to migrate a component and (presumably) to think through the implications. We also warn about this case. The presence of U_cWRP just means that someone ran a codemod script and didn't necessarily think through anything further. And if they may have external dependencies with legacy mixins, as I've seen internally.
It would break the polyfill, for one. Our approach to backwards compat relies on the presence of gDSFP and cWRP. We could enforce that cWRP and U_cWRP can't co-exist but I think that might just make the transition more pain (actual example: me, running codemods internally). |
I don't see why. Old React versions don't have the check / support for new lifecycles. I imagined that polyfill on the newer versions would not "declare" fake cWRP/cWM in the first place. But maybe that changed since my initial polyfill proposal. In that case it makes sense. |
The polyfill always defines the legacy methods, because feature detection ended up feeling too brittle (particularly when considering React-like libraries like Preact). Instead, I changed React not to warn about cWRP+gDSFP if it detected that the polyfill had added cWRP and to not invoke any of the unsafe lifecycles if gDSFP was present. This relates to PR #12105 |
|
These conditions are getting nasty. I'm nervous about creating a bug there in some permutation, now or later, but I trust you various permutations are tested. Can't wait for 17. |
| } else if (typeof Component.getDerivedStateFromProps !== 'function') { | ||
| } | ||
| if ( | ||
| inst.UNSAFE_componentWillMount && |
There was a problem hiding this comment.
Maybe change this to test for function type explicitly for consistency? I know it didn't before but still.
|
What is this all about |
|
https://github.com/reactjs/rfcs/blob/master/text/0006-static-lifecycle-methods.md There'll be a blog post. Stay tuned. |
* Invoke both legacy and UNSAFE_ lifecycles when both are present This is to support edge cases with eg create-react-class where a mixin defines a legacy lifecycle but the component being created defines an UNSAFE one (or vice versa). I did not warn about this case because the warning would be a bit redundant with the deprecation warning which we will soon be enabling. I could be convinced to change my stance here though. * Added explicit function-type check to SS ReactPartialRenderer
This is to support edge cases, specifically with
create-react-class, where a mixin defines a legacy lifecycle but the component being created defines an UNSAFE_ one (or vice versa).I did not warn about this case because the warning would be a bit redundant with the deprecation warning which we will soon be enabling. I could be convinced to change my stance here though.
Added some new tests for this on the various renders that it impacts (default, ssr, shallow). Also verified that this change fixes several (all that I've tested so far) of the test that failed internally for me on www after running the rename codemod.