From b71ca932d73bbec8dfc856cad8399f82609c8a1c Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Wed, 17 Jan 2018 14:18:35 -0800 Subject: [PATCH] Added getDerivedStateFromProps to ReactFiberClassComponent Also updated class component and lifecyle tests to cover the added functionality. --- .../__tests__/ReactComponentLifeCycle-test.js | 73 ++++++++- .../src/ReactFiberClassComponent.js | 143 ++++++++++++++---- .../ReactCoffeeScriptClass-test.coffee | 49 ++++++ .../react/src/__tests__/ReactES6Class-test.js | 54 +++++++ .../__tests__/ReactTypeScriptClass-test.ts | 54 +++++++ 5 files changed, 340 insertions(+), 33 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactComponentLifeCycle-test.js b/packages/react-dom/src/__tests__/ReactComponentLifeCycle-test.js index d133868d21400..351148aa22587 100644 --- a/packages/react-dom/src/__tests__/ReactComponentLifeCycle-test.js +++ b/packages/react-dom/src/__tests__/ReactComponentLifeCycle-test.js @@ -509,6 +509,10 @@ describe('ReactComponentLifeCycle', () => { }; }; class Outer extends React.Component { + static getDerivedStateFromProps(props, prevState) { + log.push('outer getDerivedStateFromProps'); + return null; + } UNSAFE_componentWillMount = logger('outer componentWillMount'); componentDidMount = logger('outer componentDidMount'); UNSAFE_componentWillReceiveProps = logger( @@ -528,6 +532,10 @@ describe('ReactComponentLifeCycle', () => { } class Inner extends React.Component { + static getDerivedStateFromProps(props, prevState) { + log.push('inner getDerivedStateFromProps'); + return null; + } UNSAFE_componentWillMount = logger('inner componentWillMount'); componentDidMount = logger('inner componentDidMount'); UNSAFE_componentWillReceiveProps = logger( @@ -544,21 +552,33 @@ describe('ReactComponentLifeCycle', () => { const container = document.createElement('div'); log = []; - ReactDOM.render(, container); + expect(() => ReactDOM.render(, container)).toWarnDev([ + 'Warning: Outer: Defines both componentWillReceiveProps() and static ' + + 'getDerivedStateFromProps() methods. ' + + 'We recommend using only getDerivedStateFromProps().', + 'Warning: Inner: Defines both componentWillReceiveProps() and static ' + + 'getDerivedStateFromProps() methods. ' + + 'We recommend using only getDerivedStateFromProps().', + ]); expect(log).toEqual([ + 'outer getDerivedStateFromProps', 'outer componentWillMount', + 'inner getDerivedStateFromProps', 'inner componentWillMount', 'inner componentDidMount', 'outer componentDidMount', ]); + // Dedup warnings log = []; - ReactDOM.render(, container); + ReactDOM.render(, container); expect(log).toEqual([ 'outer componentWillReceiveProps', + 'outer getDerivedStateFromProps', 'outer shouldComponentUpdate', 'outer componentWillUpdate', 'inner componentWillReceiveProps', + 'inner getDerivedStateFromProps', 'inner shouldComponentUpdate', 'inner componentWillUpdate', 'inner componentDidUpdate', @@ -573,6 +593,37 @@ describe('ReactComponentLifeCycle', () => { ]); }); + 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(, container)).toWarnDev([ + 'Warning: MyComponent: componentWillMount() is deprecated and will be ' + + 'removed in the next major version. ' + + 'Please use UNSAFE_componentWillMount() instead.', + ]); + + expect(() => ReactDOM.render(, 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(, container); + ReactDOM.render(, container); + }); + it('calls effects on module-pattern component', function() { const log = []; @@ -623,4 +674,22 @@ describe('ReactComponentLifeCycle', () => { 'ref', ]); }); + + it('should warn if getDerivedStateFromProps returns undefined', () => { + class MyComponent extends React.Component { + static getDerivedStateFromProps() {} + render() { + return null; + } + } + + const div = document.createElement('div'); + expect(() => ReactDOM.render(, div)).toWarnDev( + 'MyComponent.getDerivedStateFromProps(): A valid state object (or null) must ' + + 'be returned. You may have returned undefined.', + ); + + // De-duped + ReactDOM.render(, div); + }); }); diff --git a/packages/react-reconciler/src/ReactFiberClassComponent.js b/packages/react-reconciler/src/ReactFiberClassComponent.js index 9cdb92b8de3e6..0418394d28693 100644 --- a/packages/react-reconciler/src/ReactFiberClassComponent.js +++ b/packages/react-reconciler/src/ReactFiberClassComponent.js @@ -40,12 +40,23 @@ import {hasContextChanged} from './ReactFiberContext'; const fakeInternalInstance = {}; const isArray = Array.isArray; +let didWarnAboutLegacyWillMount; +let didWarnAboutLegacyWillReceiveProps; +let didWarnAboutLegacyWillUpdate; let didWarnAboutStateAssignmentForComponent; +let didWarnAboutUndefinedDerivedState; +let didWarnAboutWillReceivePropsAndDerivedState; let warnOnInvalidCallback; if (__DEV__) { - const didWarnOnInvalidCallback = {}; + didWarnAboutLegacyWillMount = {}; + didWarnAboutLegacyWillReceiveProps = {}; + didWarnAboutLegacyWillUpdate = {}; didWarnAboutStateAssignmentForComponent = {}; + didWarnAboutUndefinedDerivedState = {}; + didWarnAboutWillReceivePropsAndDerivedState = {}; + + const didWarnOnInvalidCallback = {}; warnOnInvalidCallback = function(callback: mixed, callerName: string) { if (callback === null || typeof callback === 'function') { @@ -380,8 +391,17 @@ export default function( const instance = new ctor(props, context); adoptClassInstance(workInProgress, instance); - // TODO (getDerivedStateFromProps) Call Component.getDerivedStateFromProps - // Merge the returned value into instance.state + const partialState = callGetDerivedStateFromProps( + workInProgress, + instance, + props, + ); + if (partialState) { + // Render-phase updates (like this) should not be added to the update queue, + // So that multiple render passes do not enqueue multiple updates. + // Instead, just synchronously merge the returned state into the instance. + instance.state = Object.assign({}, instance.state, partialState); + } // Cache unmasked context so we can avoid recreating masked context unless necessary. // ReactFiberContext usually updates this cache but can't for newly-created instances. @@ -398,12 +418,16 @@ export default function( if (typeof instance.componentWillMount === 'function') { if (__DEV__) { - warning( - false, - '%s: componentWillMount() is deprecated and will be removed in the ' + - 'next major version. Please use UNSAFE_componentWillMount() instead.', - getComponentName(workInProgress), - ); + const componentName = getComponentName(workInProgress) || 'Unknown'; + if (!didWarnAboutLegacyWillMount[componentName]) { + warning( + false, + '%s: componentWillMount() is deprecated and will be removed in the ' + + 'next major version. Please use UNSAFE_componentWillMount() instead.', + componentName, + ); + didWarnAboutLegacyWillMount[componentName] = true; + } } instance.componentWillMount(); } else { @@ -435,12 +459,16 @@ export default function( const oldState = instance.state; if (typeof instance.componentWillReceiveProps === 'function') { if (__DEV__) { - warning( - false, - '%s: componentWillReceiveProps() is deprecated and will be removed in the ' + - 'next major version. Please use UNSAFE_componentWillReceiveProps() instead.', - getComponentName(workInProgress), - ); + const componentName = getComponentName(workInProgress) || 'Unknown'; + if (!didWarnAboutLegacyWillReceiveProps[componentName]) { + warning( + false, + '%s: componentWillReceiveProps() is deprecated and will be removed in the ' + + 'next major version. Please use UNSAFE_componentWillReceiveProps() instead.', + componentName, + ); + didWarnAboutLegacyWillReceiveProps[componentName] = true; + } } startPhaseTimer(workInProgress, 'componentWillReceiveProps'); @@ -457,18 +485,9 @@ export default function( } } - // TODO (getDerivedStateFromProps) If both cWRP and static gDSFP methods exist, warn. - // Call cWRP first then static gDSFP; don't bother trying to sync apply setState() changes. - - // TODO (getDerivedStateFromProps) Call Component.getDerivedStateFromProps - - // TODO (getDerivedStateFromProps) Returned value should not be added to update queue. - // Just synchronously Object.assign it into instance.state - // This should be covered in a test too. - if (instance.state !== oldState) { if (__DEV__) { - const componentName = getComponentName(workInProgress) || 'Component'; + const componentName = getComponentName(workInProgress) || 'Unknown'; if (!didWarnAboutStateAssignmentForComponent[componentName]) { warning( false, @@ -484,6 +503,50 @@ export default function( } } + function callGetDerivedStateFromProps(workInProgress, instance, props) { + const {type} = workInProgress; + + if (typeof type.getDerivedStateFromProps === 'function') { + if (__DEV__) { + if ( + typeof instance.componentWillReceiveProps === 'function' || + typeof instance.UNSAFE_componentWillReceiveProps === 'function' + ) { + const componentName = getComponentName(workInProgress) || 'Unknown'; + if (!didWarnAboutWillReceivePropsAndDerivedState[componentName]) { + warning( + false, + '%s: Defines both componentWillReceiveProps() and static ' + + 'getDerivedStateFromProps() methods. We recommend using ' + + 'only getDerivedStateFromProps().', + componentName, + ); + didWarnAboutWillReceivePropsAndDerivedState[componentName] = true; + } + } + } + + const partialState = type.getDerivedStateFromProps(props, instance.state); + + if (__DEV__) { + if (partialState === undefined) { + const componentName = getComponentName(workInProgress) || 'Unknown'; + if (!didWarnAboutUndefinedDerivedState[componentName]) { + warning( + false, + '%s.getDerivedStateFromProps(): A valid state object (or null) must be returned. ' + + 'You may have returned undefined.', + componentName, + ); + didWarnAboutUndefinedDerivedState[componentName] = componentName; + } + } + } + + return partialState || null; + } + } + // Invokes the mount life-cycles on a previously never rendered instance. function mountClassInstance( workInProgress: Fiber, @@ -675,6 +738,12 @@ export default function( ); } + const partialState = callGetDerivedStateFromProps( + workInProgress, + instance, + newProps, + ); + // Compute the next state using the memoized state and the update queue. const oldState = workInProgress.memoizedState; // TODO: Previous state can be null. @@ -692,6 +761,13 @@ export default function( newState = oldState; } + if (partialState) { + // Render-phase updates (like this) should not be added to the update queue, + // So that multiple render passes do not enqueue multiple updates. + // Instead, just synchronously merge the returned state into the instance. + newState = Object.assign({}, newState, partialState); + } + if ( oldProps === newProps && oldState === newState && @@ -730,12 +806,17 @@ export default function( ) { if (typeof instance.componentWillUpdate === 'function') { if (__DEV__) { - warning( - false, - '%s: componentWillUpdate() is deprecated and will be removed in the ' + - 'next major version. Please use UNSAFE_componentWillUpdate() instead.', - getComponentName(workInProgress), - ); + const componentName = + getComponentName(workInProgress) || 'Component'; + if (!didWarnAboutLegacyWillUpdate[componentName]) { + warning( + false, + '%s: componentWillUpdate() is deprecated and will be removed in the ' + + 'next major version. Please use UNSAFE_componentWillUpdate() instead.', + componentName, + ); + didWarnAboutLegacyWillUpdate[componentName] = true; + } } startPhaseTimer(workInProgress, 'componentWillUpdate'); diff --git a/packages/react/src/__tests__/ReactCoffeeScriptClass-test.coffee b/packages/react/src/__tests__/ReactCoffeeScriptClass-test.coffee index a3bd6e8779e41..ef3a1eeecd0cd 100644 --- a/packages/react/src/__tests__/ReactCoffeeScriptClass-test.coffee +++ b/packages/react/src/__tests__/ReactCoffeeScriptClass-test.coffee @@ -98,6 +98,55 @@ describe 'ReactCoffeeScriptClass', -> test React.createElement(Foo), 'SPAN', 'bar' undefined + it 'sets initial state with value returned by static getDerivedStateFromProps', -> + class Foo extends React.Component + render: -> + div + className: "#{@state.foo} #{@state.bar}" + Foo.getDerivedStateFromProps = (nextProps, prevState) -> + { + foo: nextProps.foo + bar: 'bar' + } + test React.createElement(Foo, foo: 'foo'), 'DIV', 'foo bar' + undefined + + it 'updates initial state with values returned by static getDerivedStateFromProps', -> + class Foo extends React.Component + constructor: (props, context) -> + super props, context + @state = + foo: 'foo' + bar: 'bar' + render: -> + div + className: "#{@state.foo} #{@state.bar}" + Foo.getDerivedStateFromProps = (nextProps, prevState) -> + { + foo: "not-#{prevState.foo}" + } + test React.createElement(Foo), 'DIV', 'not-foo bar' + undefined + + it 'renders updated state with values returned by static getDerivedStateFromProps', -> + class Foo extends React.Component + constructor: (props, context) -> + super props, context + @state = + value: 'initial' + render: -> + div + className: @state.value + Foo.getDerivedStateFromProps = (nextProps, prevState) -> + if nextProps.update + return { + value: 'updated' + } + return null + test React.createElement(Foo, update: false), 'DIV', 'initial' + test React.createElement(Foo, update: true), 'DIV', 'updated' + undefined + it 'renders based on context in the constructor', -> class Foo extends React.Component @contextTypes: diff --git a/packages/react/src/__tests__/ReactES6Class-test.js b/packages/react/src/__tests__/ReactES6Class-test.js index b7ef26504a4ff..401c3a4e5ec32 100644 --- a/packages/react/src/__tests__/ReactES6Class-test.js +++ b/packages/react/src/__tests__/ReactES6Class-test.js @@ -109,6 +109,60 @@ describe('ReactES6Class', () => { test(, 'SPAN', 'bar'); }); + it('sets initial state with value returned by static getDerivedStateFromProps', () => { + class Foo extends React.Component { + static getDerivedStateFromProps(nextProps, prevState) { + return { + foo: nextProps.foo, + bar: 'bar', + }; + } + render() { + return
; + } + } + test(, 'DIV', 'foo bar'); + }); + + it('updates initial state with values returned by static getDerivedStateFromProps', () => { + class Foo extends React.Component { + state = { + foo: 'foo', + bar: 'bar', + }; + static getDerivedStateFromProps(nextProps, prevState) { + return { + foo: `not-${prevState.foo}`, + }; + } + render() { + return
; + } + } + test(, 'DIV', 'not-foo bar'); + }); + + it('renders updated state with values returned by static getDerivedStateFromProps', () => { + class Foo extends React.Component { + state = { + value: 'initial', + }; + static getDerivedStateFromProps(nextProps, prevState) { + if (nextProps.update) { + return { + value: 'updated', + }; + } + return null; + } + render() { + return
; + } + } + test(, 'DIV', 'initial'); + test(, 'DIV', 'updated'); + }); + it('renders based on context in the constructor', () => { class Foo extends React.Component { constructor(props, context) { diff --git a/packages/react/src/__tests__/ReactTypeScriptClass-test.ts b/packages/react/src/__tests__/ReactTypeScriptClass-test.ts index 3dce5bcd4bde6..fdb4f494ed9ff 100644 --- a/packages/react/src/__tests__/ReactTypeScriptClass-test.ts +++ b/packages/react/src/__tests__/ReactTypeScriptClass-test.ts @@ -344,6 +344,60 @@ describe('ReactTypeScriptClass', function() { test(React.createElement(StateBasedOnProps), 'SPAN', 'bar'); }); + it('sets initial state with value returned by static getDerivedStateFromProps', function() { + class Foo extends React.Component { + static getDerivedStateFromProps(nextProps, prevState) { + return { + foo: nextProps.foo, + bar: 'bar', + }; + } + render() { + return React.createElement('div', {className: `${this.state.foo} ${this.state.bar}`}); + } + } + test(React.createElement(Foo, {foo: "foo"}), 'DIV', 'foo bar'); + }); + + it('updates initial state with values returned by static getDerivedStateFromProps', function() { + class Foo extends React.Component { + state = { + foo: 'foo', + bar: 'bar', + }; + static getDerivedStateFromProps(nextProps, prevState) { + return { + foo: `not-${prevState.foo}`, + }; + } + render() { + return React.createElement('div', {className: `${this.state.foo} ${this.state.bar}`}); + } + } + test(React.createElement(Foo), 'DIV', 'not-foo bar'); + }); + + it('renders updated state with values returned by static getDerivedStateFromProps', function() { + class Foo extends React.Component { + state = { + value: 'initial', + }; + static getDerivedStateFromProps(nextProps, prevState) { + if (nextProps.update) { + return { + value: 'updated', + }; + } + return null; + } + render() { + return React.createElement('div', {className: this.state.value}); + } + } + test(React.createElement(Foo, {update:false}), 'DIV', 'initial'); + test(React.createElement(Foo, {update:true}), 'DIV', 'updated'); + }); + it('renders based on context in the constructor', function() { test(React.createElement(ProvideChildContextTypes), 'SPAN', 'foo'); });