Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

React lifecycles compat #12105

Merged
merged 4 commits into from
Jan 29, 2018
Merged

Conversation

bvaughn
Copy link
Contributor

@bvaughn bvaughn commented Jan 26, 2018

In order to support the upcoming react-lifecycles-compat module and various edge-cases that are possible when considering eg class inheritance and other React-like libraries, this PR makes the following changes:

  • If a component defines static getDerivedStateFromProps, React will not invoke any of the following instance methods: componentWillMount, componentWillReceiveProps, and componentWillUpdate.
  • If React detects that a component has been polyfilled by react-lifecycles-compat (based on the presence of a __suppressDeprecationWarning flag) 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.

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.
Copy link
Collaborator

@gaearon gaearon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Accept to unblock. I haven't checked in detail. A few inline comments on what I'd find helpful as a reviewer.

ReactDOM.render(<PolyfilledComponent />, container);
});

it('should not warn about unsafe lifecycles within "strict" tree for polyfilled components', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should also say "deprecated cWM/cWRP" rather than "unsafe lifecycles"?

Copy link
Contributor Author

@bvaughn bvaughn Jan 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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',
);
});

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth adding a similar test for unsafe lifecycles maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure 👍

Component.prototype.componentWillMount = function() {};
Component.prototype.componentWillMount.__suppressDeprecationWarning = true;
Component.prototype.componentWillReceiveProps = function() {};
Component.prototype.componentWillReceiveProps.__suppressDeprecationWarning = true;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice call.

static getDerivedStateFromProps() {
return null;
}
UNSAFE_componentWillMount() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Collaborator

@gaearon gaearon Jan 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@bvaughn bvaughn Jan 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@bvaughn bvaughn Jan 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@bvaughn bvaughn merged commit a7b9f98 into facebook:master Jan 29, 2018
@bvaughn bvaughn deleted the react-lifecycles-compat branch January 29, 2018 16:06
NMinhNguyen referenced this pull request in enzymejs/react-shallow-renderer Jan 29, 2020
* Suppress unsafe/deprecation warnings for polyfilled components.
* Don't invoke deprecated lifecycles if static gDSFP exists.
* Applied recent changes to server rendering also
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants