From 55e75da77d0d69df493188cceeb3345b33622db7 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Thu, 22 Mar 2018 13:32:19 -0700 Subject: [PATCH] Don't invoke legacy lifecycles if getSnapshotBeforeUpdate() is defined. DEV warn about this. --- .../__tests__/ReactComponentLifeCycle-test.js | 128 ++++++++++++++++-- .../src/ReactFiberClassComponent.js | 84 +++++++----- .../src/ReactShallowRenderer.js | 68 +++++----- .../__tests__/ReactShallowRenderer-test.js | 42 ++++++ .../createReactClassIntegration-test.js | 36 ++++- 5 files changed, 276 insertions(+), 82 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactComponentLifeCycle-test.js b/packages/react-dom/src/__tests__/ReactComponentLifeCycle-test.js index 31846ab454988..c8264410b64ea 100644 --- a/packages/react-dom/src/__tests__/ReactComponentLifeCycle-test.js +++ b/packages/react-dom/src/__tests__/ReactComponentLifeCycle-test.js @@ -673,10 +673,38 @@ describe('ReactComponentLifeCycle', () => { const container = document.createElement('div'); expect(() => ReactDOM.render(, container)).toWarnDev( - 'Unsafe legacy lifecycles will not be called for components using the new getDerivedStateFromProps() API.', + 'Unsafe legacy lifecycles will not be called for components using new component APIs.', ); }); + it('should not invoke deprecated lifecycles (cWM/cWRP/cWU) if new getSnapshotBeforeUpdate is present', () => { + class Component extends React.Component { + state = {}; + getSnapshotBeforeUpdate() { + return null; + } + componentWillMount() { + throw Error('unexpected'); + } + componentWillReceiveProps() { + throw Error('unexpected'); + } + componentWillUpdate() { + throw Error('unexpected'); + } + componentDidUpdate() {} + render() { + return null; + } + } + + const container = document.createElement('div'); + expect(() => ReactDOM.render(, container)).toWarnDev( + 'Unsafe legacy lifecycles will not be called for components using new component APIs.', + ); + ReactDOM.render(, container); + }); + it('should not invoke new unsafe lifecycles (cWM/cWRP/cWU) if static gDSFP is present', () => { class Component extends React.Component { state = {}; @@ -698,9 +726,10 @@ describe('ReactComponentLifeCycle', () => { } const container = document.createElement('div'); - expect(() => ReactDOM.render(, container)).toWarnDev( - 'Unsafe legacy lifecycles will not be called for components using the new getDerivedStateFromProps() API.', + expect(() => ReactDOM.render(, container)).toWarnDev( + 'Unsafe legacy lifecycles will not be called for components using new component APIs.', ); + ReactDOM.render(, container); }); it('should warn about deprecated lifecycles (cWM/cWRP/cWU) if new static gDSFP is present', () => { @@ -720,7 +749,7 @@ describe('ReactComponentLifeCycle', () => { } expect(() => ReactDOM.render(, container)).toWarnDev( - 'Unsafe legacy lifecycles will not be called for components using the new getDerivedStateFromProps() API.\n\n' + + 'Unsafe legacy lifecycles will not be called for components using new component APIs.\n\n' + 'AllLegacyLifecycles uses getDerivedStateFromProps() but also contains the following legacy lifecycles:\n' + ' componentWillMount\n' + ' UNSAFE_componentWillReceiveProps\n' + @@ -741,7 +770,7 @@ describe('ReactComponentLifeCycle', () => { } expect(() => ReactDOM.render(, container)).toWarnDev( - 'Unsafe legacy lifecycles will not be called for components using the new getDerivedStateFromProps() API.\n\n' + + 'Unsafe legacy lifecycles will not be called for components using new component APIs.\n\n' + 'WillMount uses getDerivedStateFromProps() but also contains the following legacy lifecycles:\n' + ' UNSAFE_componentWillMount\n\n' + 'The above lifecycles should be removed. Learn more about this warning here:\n' + @@ -761,7 +790,7 @@ describe('ReactComponentLifeCycle', () => { } expect(() => ReactDOM.render(, container)).toWarnDev( - 'Unsafe legacy lifecycles will not be called for components using the new getDerivedStateFromProps() API.\n\n' + + 'Unsafe legacy lifecycles will not be called for components using new component APIs.\n\n' + 'WillMountAndUpdate uses getDerivedStateFromProps() but also contains the following legacy lifecycles:\n' + ' componentWillMount\n' + ' UNSAFE_componentWillUpdate\n\n' + @@ -781,7 +810,7 @@ describe('ReactComponentLifeCycle', () => { } expect(() => ReactDOM.render(, container)).toWarnDev( - 'Unsafe legacy lifecycles will not be called for components using the new getDerivedStateFromProps() API.\n\n' + + 'Unsafe legacy lifecycles will not be called for components using new component APIs.\n\n' + 'WillReceiveProps uses getDerivedStateFromProps() but also contains the following legacy lifecycles:\n' + ' componentWillReceiveProps\n\n' + 'The above lifecycles should be removed. Learn more about this warning here:\n' + @@ -789,6 +818,88 @@ describe('ReactComponentLifeCycle', () => { ); }); + it('should warn about deprecated lifecycles (cWM/cWRP/cWU) if new getSnapshotBeforeUpdate is present', () => { + const container = document.createElement('div'); + + class AllLegacyLifecycles extends React.Component { + state = {}; + getSnapshotBeforeUpdate() {} + componentWillMount() {} + UNSAFE_componentWillReceiveProps() {} + componentWillUpdate() {} + componentDidUpdate() {} + render() { + return null; + } + } + + expect(() => ReactDOM.render(, container)).toWarnDev( + 'Unsafe legacy lifecycles will not be called for components using new component APIs.\n\n' + + 'AllLegacyLifecycles uses getSnapshotBeforeUpdate() but also contains the following legacy lifecycles:\n' + + ' componentWillMount\n' + + ' UNSAFE_componentWillReceiveProps\n' + + ' componentWillUpdate\n\n' + + 'The above lifecycles should be removed. Learn more about this warning here:\n' + + 'https://fb.me/react-async-component-lifecycle-hooks', + ); + + class WillMount extends React.Component { + state = {}; + getSnapshotBeforeUpdate() {} + UNSAFE_componentWillMount() {} + componentDidUpdate() {} + render() { + return null; + } + } + + expect(() => ReactDOM.render(, container)).toWarnDev( + 'Unsafe legacy lifecycles will not be called for components using new component APIs.\n\n' + + 'WillMount uses getSnapshotBeforeUpdate() but also contains the following legacy lifecycles:\n' + + ' UNSAFE_componentWillMount\n\n' + + 'The above lifecycles should be removed. Learn more about this warning here:\n' + + 'https://fb.me/react-async-component-lifecycle-hooks', + ); + + class WillMountAndUpdate extends React.Component { + state = {}; + getSnapshotBeforeUpdate() {} + componentWillMount() {} + UNSAFE_componentWillUpdate() {} + componentDidUpdate() {} + render() { + return null; + } + } + + expect(() => ReactDOM.render(, container)).toWarnDev( + 'Unsafe legacy lifecycles will not be called for components using new component APIs.\n\n' + + 'WillMountAndUpdate uses getSnapshotBeforeUpdate() but also contains the following legacy lifecycles:\n' + + ' componentWillMount\n' + + ' UNSAFE_componentWillUpdate\n\n' + + 'The above lifecycles should be removed. Learn more about this warning here:\n' + + 'https://fb.me/react-async-component-lifecycle-hooks', + ); + + class WillReceiveProps extends React.Component { + state = {}; + getSnapshotBeforeUpdate() {} + componentWillReceiveProps() {} + componentDidUpdate() {} + render() { + return null; + } + } + + expect(() => ReactDOM.render(, container)).toWarnDev( + 'Unsafe legacy lifecycles will not be called for components using new component APIs.\n\n' + + 'WillReceiveProps uses getSnapshotBeforeUpdate() but also contains the following legacy lifecycles:\n' + + ' componentWillReceiveProps\n\n' + + 'The above lifecycles should be removed. Learn more about this warning here:\n' + + 'https://fb.me/react-async-component-lifecycle-hooks', + ); + }); + it('calls effects on module-pattern component', function() { const log = []; @@ -1037,9 +1148,6 @@ describe('ReactComponentLifeCycle', () => { ReactDOM.render(
, div); expect(log).toEqual([]); - - // De-duped - ReactDOM.render(, div); }); it('should warn if getSnapshotBeforeUpdate returns undefined', () => { diff --git a/packages/react-reconciler/src/ReactFiberClassComponent.js b/packages/react-reconciler/src/ReactFiberClassComponent.js index 58f48faca9e74..271a415412af1 100644 --- a/packages/react-reconciler/src/ReactFiberClassComponent.js +++ b/packages/react-reconciler/src/ReactFiberClassComponent.js @@ -370,6 +370,7 @@ export default function( if ( typeof instance.getSnapshotBeforeUpdate === 'function' && typeof instance.componentDidUpdate !== 'function' && + typeof instance.UNSAFE_componentDidUpdate !== 'function' && !didWarnAboutGetSnapshotBeforeUpdateWithoutDidUpdate.has(type) ) { didWarnAboutGetSnapshotBeforeUpdateWithoutDidUpdate.add(type); @@ -452,24 +453,30 @@ export default function( adoptClassInstance(workInProgress, instance); if (__DEV__) { - if (typeof ctor.getDerivedStateFromProps === 'function') { - if (state === null) { - const componentName = getComponentName(workInProgress) || 'Component'; - if (!didWarnAboutUninitializedState[componentName]) { - warning( - false, - '%s: Did not properly initialize state during construction. ' + - 'Expected state to be an object, but it was %s.', - componentName, - instance.state === null ? 'null' : 'undefined', - ); - didWarnAboutUninitializedState[componentName] = true; - } + if ( + typeof ctor.getDerivedStateFromProps === 'function' && + state === null + ) { + const componentName = getComponentName(workInProgress) || 'Component'; + if (!didWarnAboutUninitializedState[componentName]) { + warning( + false, + '%s: Did not properly initialize state during construction. ' + + 'Expected state to be an object, but it was %s.', + componentName, + instance.state === null ? 'null' : 'undefined', + ); + didWarnAboutUninitializedState[componentName] = true; } + } - // If getDerivedStateFromProps() is defined, "unsafe" lifecycles won't be called. - // Warn about these lifecycles if they are present. - // Don't warn about react-lifecycles-compat polyfilled methods though. + // If new component APIs are defined, "unsafe" lifecycles won't be called. + // Warn about these lifecycles if they are present. + // Don't warn about react-lifecycles-compat polyfilled methods though. + if ( + typeof ctor.getDerivedStateFromProps === 'function' || + typeof instance.getSnapshotBeforeUpdate === 'function' + ) { let foundWillMountName = null; let foundWillReceivePropsName = null; let foundWillUpdateName = null; @@ -503,16 +510,18 @@ export default function( foundWillUpdateName !== null ) { const componentName = getComponentName(workInProgress) || 'Component'; + const newApiName = ctor.getDerivedStateFromProps + ? 'getDerivedStateFromProps()' + : 'getSnapshotBeforeUpdate()'; if (!didWarnAboutLegacyLifecyclesAndDerivedState[componentName]) { warning( false, - 'Unsafe legacy lifecycles will not be called for components using ' + - 'the new getDerivedStateFromProps() API.\n\n' + - '%s uses getDerivedStateFromProps() but also contains the following legacy lifecycles:' + - '%s%s%s\n\n' + + 'Unsafe legacy lifecycles will not be called for components using new component APIs.\n\n' + + '%s uses %s but also contains the following legacy lifecycles:%s%s%s\n\n' + 'The above lifecycles should be removed. Learn more about this warning here:\n' + 'https://fb.me/react-async-component-lifecycle-hooks', componentName, + newApiName, foundWillMountName !== null ? `\n ${foundWillMountName}` : '', foundWillReceivePropsName !== null ? `\n ${foundWillReceivePropsName}` @@ -696,11 +705,12 @@ export default function( } // In order to support react-lifecycles-compat polyfilled components, - // Unsafe lifecycles should not be invoked for any component with the new gDSFP. + // Unsafe lifecycles should not be invoked for components using the new APIs. if ( + typeof ctor.getDerivedStateFromProps !== 'function' && + typeof instance.getSnapshotBeforeUpdate !== 'function' && (typeof instance.UNSAFE_componentWillMount === 'function' || - typeof instance.componentWillMount === 'function') && - typeof ctor.getDerivedStateFromProps !== 'function' + typeof instance.componentWillMount === 'function') ) { callComponentWillMount(workInProgress, instance); // If we had additional state updates during this life-cycle, let's @@ -741,11 +751,12 @@ export default function( // during componentDidUpdate we pass the "current" props. // In order to support react-lifecycles-compat polyfilled components, - // Unsafe lifecycles should not be invoked for any component with the new gDSFP. + // Unsafe lifecycles should not be invoked for components using the new APIs. if ( + typeof ctor.getDerivedStateFromProps !== 'function' && + typeof instance.getSnapshotBeforeUpdate !== 'function' && (typeof instance.UNSAFE_componentWillReceiveProps === 'function' || - typeof instance.componentWillReceiveProps === 'function') && - typeof ctor.getDerivedStateFromProps !== 'function' + typeof instance.componentWillReceiveProps === 'function') ) { if (oldProps !== newProps || oldContext !== newContext) { callComponentWillReceiveProps( @@ -852,11 +863,12 @@ export default function( if (shouldUpdate) { // In order to support react-lifecycles-compat polyfilled components, - // Unsafe lifecycles should not be invoked for any component with the new gDSFP. + // Unsafe lifecycles should not be invoked for components using the new APIs. if ( + typeof ctor.getDerivedStateFromProps !== 'function' && + typeof instance.getSnapshotBeforeUpdate !== 'function' && (typeof instance.UNSAFE_componentWillMount === 'function' || - typeof instance.componentWillMount === 'function') && - typeof ctor.getDerivedStateFromProps !== 'function' + typeof instance.componentWillMount === 'function') ) { startPhaseTimer(workInProgress, 'componentWillMount'); if (typeof instance.componentWillMount === 'function') { @@ -913,11 +925,12 @@ export default function( // during componentDidUpdate we pass the "current" props. // In order to support react-lifecycles-compat polyfilled components, - // Unsafe lifecycles should not be invoked for any component with the new gDSFP. + // Unsafe lifecycles should not be invoked for components using the new APIs. if ( + typeof ctor.getDerivedStateFromProps !== 'function' && + typeof instance.getSnapshotBeforeUpdate !== 'function' && (typeof instance.UNSAFE_componentWillReceiveProps === 'function' || - typeof instance.componentWillReceiveProps === 'function') && - typeof ctor.getDerivedStateFromProps !== 'function' + typeof instance.componentWillReceiveProps === 'function') ) { if (oldProps !== newProps || oldContext !== newContext) { callComponentWillReceiveProps( @@ -1038,11 +1051,12 @@ export default function( if (shouldUpdate) { // In order to support react-lifecycles-compat polyfilled components, - // Unsafe lifecycles should not be invoked for any component with the new gDSFP. + // Unsafe lifecycles should not be invoked for components using the new APIs. if ( + typeof ctor.getDerivedStateFromProps !== 'function' && + typeof instance.getSnapshotBeforeUpdate !== 'function' && (typeof instance.UNSAFE_componentWillUpdate === 'function' || - typeof instance.componentWillUpdate === 'function') && - typeof ctor.getDerivedStateFromProps !== 'function' + typeof instance.componentWillUpdate === 'function') ) { startPhaseTimer(workInProgress, 'componentWillUpdate'); if (typeof instance.componentWillUpdate === 'function') { diff --git a/packages/react-test-renderer/src/ReactShallowRenderer.js b/packages/react-test-renderer/src/ReactShallowRenderer.js index 8d80221cbbd1e..131db2d417511 100644 --- a/packages/react-test-renderer/src/ReactShallowRenderer.js +++ b/packages/react-test-renderer/src/ReactShallowRenderer.js @@ -139,20 +139,18 @@ class ReactShallowRenderer { ) { const beforeState = this._newState; - if (typeof this._instance.componentWillMount === 'function') { - // In order to support react-lifecycles-compat polyfilled components, - // Unsafe lifecycles should not be invoked for any component with the new gDSFP. - if (typeof element.type.getDerivedStateFromProps !== 'function') { - this._instance.componentWillMount(); - } - } + // In order to support react-lifecycles-compat polyfilled components, + // Unsafe lifecycles should not be invoked for components using the new APIs. if ( - typeof this._instance.UNSAFE_componentWillMount === 'function' && - typeof element.type.getDerivedStateFromProps !== 'function' + typeof element.type.getDerivedStateFromProps !== 'function' && + typeof this._instance.getSnapshotBeforeUpdate !== 'function' ) { - // In order to support react-lifecycles-compat polyfilled components, - // Unsafe lifecycles should not be invoked for any component with the new gDSFP. - this._instance.UNSAFE_componentWillMount(); + if (typeof this._instance.componentWillMount === 'function') { + this._instance.componentWillMount(); + } + if (typeof this._instance.UNSAFE_componentWillMount === 'function') { + this._instance.UNSAFE_componentWillMount(); + } } // setState may have been called during cWM @@ -173,20 +171,20 @@ class ReactShallowRenderer { const oldProps = this._instance.props; if (oldProps !== props) { - if (typeof this._instance.componentWillReceiveProps === 'function') { - // In order to support react-lifecycles-compat polyfilled components, - // Unsafe lifecycles should not be invoked for any component with the new gDSFP. - if (typeof element.type.getDerivedStateFromProps !== 'function') { - this._instance.componentWillReceiveProps(props, context); - } - } + // In order to support react-lifecycles-compat polyfilled components, + // Unsafe lifecycles should not be invoked for components using the new APIs. if ( - typeof this._instance.UNSAFE_componentWillReceiveProps === 'function' && - typeof element.type.getDerivedStateFromProps !== 'function' + typeof element.type.getDerivedStateFromProps !== 'function' && + typeof this._instance.getSnapshotBeforeUpdate !== 'function' ) { - // In order to support react-lifecycles-compat polyfilled components, - // Unsafe lifecycles should not be invoked for any component with the new gDSFP. - this._instance.UNSAFE_componentWillReceiveProps(props, context); + if (typeof this._instance.componentWillReceiveProps === 'function') { + this._instance.componentWillReceiveProps(props, context); + } + if ( + typeof this._instance.UNSAFE_componentWillReceiveProps === 'function' + ) { + this._instance.UNSAFE_componentWillReceiveProps(props, context); + } } this._updateStateFromStaticLifecycle(props); @@ -211,20 +209,18 @@ class ReactShallowRenderer { } if (shouldUpdate) { - if (typeof this._instance.componentWillUpdate === 'function') { - // In order to support react-lifecycles-compat polyfilled components, - // Unsafe lifecycles should not be invoked for any component with the new gDSFP. - if (typeof type.getDerivedStateFromProps !== 'function') { - this._instance.componentWillUpdate(props, state, context); - } - } + // In order to support react-lifecycles-compat polyfilled components, + // Unsafe lifecycles should not be invoked for components using the new APIs. if ( - typeof this._instance.UNSAFE_componentWillUpdate === 'function' && - typeof type.getDerivedStateFromProps !== 'function' + typeof element.type.getDerivedStateFromProps !== 'function' && + typeof this._instance.getSnapshotBeforeUpdate !== 'function' ) { - // In order to support react-lifecycles-compat polyfilled components, - // Unsafe lifecycles should not be invoked for any component with the new gDSFP. - this._instance.UNSAFE_componentWillUpdate(props, state, context); + if (typeof this._instance.componentWillUpdate === 'function') { + this._instance.componentWillUpdate(props, state, context); + } + if (typeof this._instance.UNSAFE_componentWillUpdate === 'function') { + this._instance.UNSAFE_componentWillUpdate(props, state, context); + } } } diff --git a/packages/react-test-renderer/src/__tests__/ReactShallowRenderer-test.js b/packages/react-test-renderer/src/__tests__/ReactShallowRenderer-test.js index 2c6618d28b729..7e60bde00367f 100644 --- a/packages/react-test-renderer/src/__tests__/ReactShallowRenderer-test.js +++ b/packages/react-test-renderer/src/__tests__/ReactShallowRenderer-test.js @@ -128,6 +128,48 @@ describe('ReactShallowRenderer', () => { shallowRenderer.render(); }); + it('should not invoke deprecated lifecycles (cWM/cWRP/cWU) if new getSnapshotBeforeUpdate is present', () => { + class Component extends React.Component { + getSnapshotBeforeUpdate() { + return null; + } + componentWillMount() { + throw Error('unexpected'); + } + componentWillReceiveProps() { + throw Error('unexpected'); + } + componentWillUpdate() { + throw Error('unexpected'); + } + render() { + return null; + } + } + + const shallowRenderer = createRenderer(); + shallowRenderer.render(); + shallowRenderer.render(); + }); + + it('should not call getSnapshotBeforeUpdate or componentDidUpdate when updating since refs wont exist', () => { + class Component extends React.Component { + getSnapshotBeforeUpdate() { + throw Error('unexpected'); + } + componentDidUpdate() { + throw Error('unexpected'); + } + render() { + return null; + } + } + + const shallowRenderer = createRenderer(); + shallowRenderer.render(); + shallowRenderer.render(); + }); + it('should only render 1 level deep', () => { function Parent() { return ( diff --git a/packages/react/src/__tests__/createReactClassIntegration-test.js b/packages/react/src/__tests__/createReactClassIntegration-test.js index eedc2df740438..a1dadaef4b0a2 100644 --- a/packages/react/src/__tests__/createReactClassIntegration-test.js +++ b/packages/react/src/__tests__/createReactClassIntegration-test.js @@ -510,7 +510,7 @@ describe('create-react-class-integration', () => { expect(() => { ReactDOM.render(, document.createElement('div')); }).toWarnDev( - 'Unsafe legacy lifecycles will not be called for components using the new getDerivedStateFromProps() API.\n\n' + + 'Unsafe legacy lifecycles will not be called for components using new component APIs.\n\n' + 'Component uses getDerivedStateFromProps() but also contains the following legacy lifecycles:\n' + ' componentWillMount\n' + ' componentWillReceiveProps\n' + @@ -521,6 +521,40 @@ describe('create-react-class-integration', () => { ReactDOM.render(, document.createElement('div')); }); + it('should not invoke deprecated lifecycles (cWM/cWRP/cWU) if new getSnapshotBeforeUpdate is present', () => { + const Component = createReactClass({ + getSnapshotBeforeUpdate: function() { + return null; + }, + componentWillMount: function() { + throw Error('unexpected'); + }, + componentWillReceiveProps: function() { + throw Error('unexpected'); + }, + componentWillUpdate: function() { + throw Error('unexpected'); + }, + componentDidUpdate: function() {}, + render: function() { + return null; + }, + }); + + expect(() => { + ReactDOM.render(, document.createElement('div')); + }).toWarnDev( + 'Unsafe legacy lifecycles will not be called for components using new component APIs.\n\n' + + 'Component uses getSnapshotBeforeUpdate() but also contains the following legacy lifecycles:\n' + + ' componentWillMount\n' + + ' componentWillReceiveProps\n' + + ' componentWillUpdate\n\n' + + 'The above lifecycles should be removed. Learn more about this warning here:\n' + + 'https://fb.me/react-async-component-lifecycle-hooks', + ); + ReactDOM.render(, document.createElement('div')); + }); + it('should invoke both deprecated and new lifecycles if both are present', () => { const log = [];