React lifecycles compat #12105
React lifecycles compat #12105
Conversation
In support of the (soon to be released) react-lifecycles-compat library. This enables shared components to support new and old versions of React in a fairly safe way without triggering noisy DEV-warnings.
Update tests accordingly.
| ReactDOM.render(<PolyfilledComponent />, container); | ||
| }); | ||
|
|
||
| it('should not warn about unsafe lifecycles within "strict" tree for polyfilled components', () => { |
There was a problem hiding this comment.
This should also say "deprecated cWM/cWRP" rather than "unsafe lifecycles"?
There was a problem hiding this comment.
Technically this test is verifying that the polyfill can add the old, deprecated methods (not the new ones with the UNSAFE_ prefix) without triggering a deprecation warning. This only really matters in the window between when we turn on the deprecation warnings and when 17 is released and those old methods go away entirely.
| 'Defines both componentWillReceiveProps', | ||
| ); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Worth adding a similar test for unsafe lifecycles maybe?
| Component.prototype.componentWillMount = function() {}; | ||
| Component.prototype.componentWillMount.__suppressDeprecationWarning = true; | ||
| Component.prototype.componentWillReceiveProps = function() {}; | ||
| Component.prototype.componentWillReceiveProps.__suppressDeprecationWarning = true; |
There was a problem hiding this comment.
This pollutes the react module but there's no resetModules() in afterEach. Let's add it so that any tests added later to this file aren't affected by this monkeypatching.
| static getDerivedStateFromProps() { | ||
| return null; | ||
| } | ||
| UNSAFE_componentWillMount() { |
There was a problem hiding this comment.
Do you want to have a similar test for non-unsafe naming too?
| // Don't warn about react-lifecycles-compat polyfilled components | ||
| if ( | ||
| typeof instance.componentWillMount === 'function' && | ||
| instance.componentWillMount.__suppressDeprecationWarning === true |
There was a problem hiding this comment.
I expect that people will use this secret flag to fool React and "hide" valid deprecations on components. Do we care about this use case? Probably not; I'm just noting that for people who do that, it would be counter-intuitive that we only check for that flag on componentWillMount but not on other methods here.
There was a problem hiding this comment.
I expect that people will use this secret flag to fool React and "hide" valid deprecations on components. Do we care about this use case?
No, not really. I believe this was the best of all options considered, despite that minor drawback.
I only check that one method because it's all that's required to verify the presence of the polyfill.
| if (typeof element.type.getDerivedStateFromProps !== 'function') { | ||
| this._instance.componentWillMount(); | ||
| } | ||
| } else if (typeof element.type.getDerivedStateFromProps !== 'function') { | ||
| this._instance.UNSAFE_componentWillMount(); | ||
| } | ||
|
|
There was a problem hiding this comment.
I haven't looked through those changes in detail.
I think it would be good to write up a table of what happens when both of either of methods is present (e.g. gDSFP, cWM/u_cWM, cWRP/u_CRWP).
My concern is that maybe some edge cases (e.g. "both cWRP and gDSFP" or "both u_CWRP and gDSFP" etc) may be handled differently between Fiber, SSR, and Shallow renderer. Of course we'd warn in this case but consistent behavior is still important.
Note I'm not saying that there are mistakes. Just that this logic is getting complex, and it's not obvious to me that all these cases work the same way. Making a table with different permutations and expected result would help us fix bugs if we find any later. It might also be helpful for the blog post.
There was a problem hiding this comment.
I think it would be good to write up a table of what happens when both of either of methods is present (e.g. gDSFP, cWM/u_cWM, cWRP/u_CRWP).
I have tests that codify that for all three renderers (ReactComponentLifeCycle-test, ReactDOMServerLifecycles-test, and ReactShallowRenderer-test).
tl;dr is that none of the unsafe lifecycles are invoked if gDSFP is present. This is necessary for the polyfill approach to work.
There was a problem hiding this comment.
Making a table with different permutations and expected result would help us fix bugs if we find any later. It might also be helpful for the blog post.
Where would such a table live? Seems like codified in tests is as good as any.
I can add some inline comments.
* Suppress unsafe/deprecation warnings for polyfilled components. * Don't invoke deprecated lifecycles if static gDSFP exists. * Applied recent changes to server rendering also
In order to support the upcoming
react-lifecycles-compatmodule and various edge-cases that are possible when considering eg class inheritance and other React-like libraries, this PR makes the following changes:static getDerivedStateFromProps, React will not invoke any of the following instance methods:componentWillMount,componentWillReceiveProps, andcomponentWillUpdate.react-lifecycles-compat(based on the presence of a__suppressDeprecationWarningflag) it will suppress both deprecation and unsafe lifecycle warnings. (Those lifecycles won't be executed anyway, and in this case it can be assumed the lifecycles only exist for backwards compatibility purposes.)All affected tests have been updated and some new ones have been added.
I have left a TODO comment in a few tests to replace an inline approximation of the new polyfill with the actual module once it's been published.