-
Notifications
You must be signed in to change notification settings - Fork 50.3k
RFC 6: Deprecate unsafe lifecycles #12028
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
Changes from 1 commit
e709e30
404944e
bd300b3
2868176
8679926
64f27d7
1047182
035c220
8d0e001
b71ca93
09c39d0
286df77
b699543
68f2fe7
2d9f75d
8f125b7
1d3e3d5
d95ec49
b940938
6cd0a8e
7572667
361a2cf
8178d52
8d67e27
53770c3
3772ee2
4dfa6e1
5609031
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
Updated tests accordingly, by temporarily splitting tests that were specific to this feature-flag into their own, internal tests. This was the only way I knew of to interact with the feature flag without breaking our build/dist tests.
- Loading branch information
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,66 @@ | ||
| /** | ||
| * Copyright (c) 2013-present, Facebook, Inc. | ||
| * | ||
| * This source code is licensed under the MIT license found in the | ||
| * LICENSE file in the root directory of this source tree. | ||
| * | ||
| * @emails react-core | ||
| */ | ||
|
|
||
| 'use strict'; | ||
|
|
||
| let React; | ||
| let ReactDOM; | ||
| let ReactFeatureFlags; | ||
|
|
||
| /** | ||
| * TODO: We should make any setState calls fail in | ||
| * `getInitialState` and `componentWillMount`. They will usually fail | ||
| * anyways because `this._renderedComponent` is empty, however, if a component | ||
| * is *reused*, then that won't be the case and things will appear to work in | ||
| * some cases. Better to just block all updates in initialization. | ||
| */ | ||
| describe('ReactComponentLifeCycle', () => { | ||
| beforeEach(() => { | ||
| jest.resetModules(); | ||
|
|
||
| ReactFeatureFlags = require('shared/ReactFeatureFlags'); | ||
| ReactFeatureFlags.warnAboutDeprecatedLifecycles = true; | ||
|
|
||
| React = require('react'); | ||
| ReactDOM = require('react-dom'); | ||
| }); | ||
|
|
||
| // TODO (RFC #6) Merge this back into ReactComponentLifeCycles-test once | ||
| // the 'warnAboutDeprecatedLifecycles' feature flag has been removed. | ||
| it('warns about deprecated unsafe lifecycles', function() { | ||
| class MyComponent extends React.Component { | ||
| componentWillMount() {} | ||
| componentWillReceiveProps() {} | ||
| componentWillUpdate() {} | ||
| render() { | ||
| return null; | ||
| } | ||
| } | ||
|
|
||
| const container = document.createElement('div'); | ||
| expect(() => ReactDOM.render(<MyComponent x={1} />, container)).toWarnDev([ | ||
| 'Warning: MyComponent: componentWillMount() is deprecated and will be ' + | ||
| 'removed in the next major version. ' + | ||
| 'Please use UNSAFE_componentWillMount() instead.', | ||
| ]); | ||
|
|
||
| expect(() => ReactDOM.render(<MyComponent x={2} />, container)).toWarnDev([ | ||
| 'Warning: MyComponent: componentWillReceiveProps() is deprecated and ' + | ||
| 'will be removed in the next major version. ' + | ||
| 'Please use UNSAFE_componentWillReceiveProps() instead.', | ||
|
||
| 'Warning: MyComponent: componentWillUpdate() is deprecated and will be ' + | ||
| 'removed in the next major version. ' + | ||
| 'Please use UNSAFE_componentWillUpdate() instead.', | ||
| ]); | ||
|
|
||
| // Dedupe check (instantiate and update) | ||
| ReactDOM.render(<MyComponent key="new" x={1} />, container); | ||
| ReactDOM.render(<MyComponent key="new" x={2} />, container); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,43 @@ | ||
| /** | ||
| * Copyright (c) 2013-present, Facebook, Inc. | ||
| * | ||
| * This source code is licensed under the MIT license found in the | ||
| * LICENSE file in the root directory of this source tree. | ||
| * | ||
| * @emails react-core | ||
| */ | ||
|
|
||
| 'use strict'; | ||
|
|
||
| let React; | ||
| let ReactFeatureFlags; | ||
| let ReactDOMServer; | ||
|
|
||
| describe('ReactDOMServerLifecycles', () => { | ||
| beforeEach(() => { | ||
| ReactFeatureFlags = require('shared/ReactFeatureFlags'); | ||
| ReactFeatureFlags.warnAboutDeprecatedLifecycles = true; | ||
|
|
||
| React = require('react'); | ||
| ReactDOMServer = require('react-dom/server'); | ||
| }); | ||
|
|
||
| // TODO (RFC #6) Merge this back into ReactDOMServerLifecycles-test once | ||
| // the 'warnAboutDeprecatedLifecycles' feature flag has been removed. | ||
| it('should warn about deprecated lifecycle hooks', () => { | ||
| class Component extends React.Component { | ||
| componentWillMount() {} | ||
| render() { | ||
| return null; | ||
| } | ||
| } | ||
|
|
||
| expect(() => ReactDOMServer.renderToString(<Component />)).toWarnDev( | ||
| 'Warning: Component: componentWillMount() is deprecated and will be removed ' + | ||
| 'in the next major version. Please use UNSAFE_componentWillMount() instead.', | ||
| ); | ||
|
|
||
| // De-duped | ||
| ReactDOMServer.renderToString(<Component />); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,7 @@ import warning from 'fbjs/lib/warning'; | |
| import checkPropTypes from 'prop-types/checkPropTypes'; | ||
| import describeComponentFrame from 'shared/describeComponentFrame'; | ||
| import {ReactDebugCurrentFrame} from 'shared/ReactGlobalSharedState'; | ||
| import {warnAboutDeprecatedLifecycles} from 'shared/ReactFeatureFlags'; | ||
| import { | ||
| REACT_FRAGMENT_TYPE, | ||
| REACT_CALL_TYPE, | ||
|
|
@@ -489,16 +490,18 @@ function resolve( | |
| if (inst.UNSAFE_componentWillMount || inst.componentWillMount) { | ||
| if (inst.componentWillMount) { | ||
| if (__DEV__) { | ||
| const componentName = getComponentName(Component) || 'Unknown'; | ||
| if (warnAboutDeprecatedLifecycles) { | ||
| const componentName = getComponentName(Component) || 'Unknown'; | ||
|
|
||
| if (!didWarnAboutDeprecatedWillMount[componentName]) { | ||
| warning( | ||
| false, | ||
| '%s: componentWillMount() is deprecated and will be removed in the ' + | ||
| 'next major version. Please use UNSAFE_componentWillMount() instead.', | ||
| componentName, | ||
| ); | ||
| didWarnAboutDeprecatedWillMount[componentName] = true; | ||
| if (!didWarnAboutDeprecatedWillMount[componentName]) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know they're not enabled yet but I don't think it's reasonable to emit one warning per component when we enable them. I would probably coalesce them across components. Gather data during reconciliation and then print after commit: This doesn't block the PR though.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Interesting. Do we do this (coalescing warnings) anywhere else? I will add this to the list of follow-up items.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, we do (or, rather, did, since we removed the whitelist in 16) this for unknown DOM props. Before we did this, the users were super annoyed because we literally printed hundreds of warnings. I think we need to be very unobtrusive here because this change is already very churning.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds like a good follow up thing to do then. Thanks for the suggestion! |
||
| warning( | ||
| false, | ||
| '%s: componentWillMount() is deprecated and will be removed in the ' + | ||
| 'next major version. Please use UNSAFE_componentWillMount() instead.', | ||
| componentName, | ||
| ); | ||
| didWarnAboutDeprecatedWillMount[componentName] = true; | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about something like this?
(link) can point to some fburl that we write later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now the link can be your RFC :)