From 53770c335c294a58be701149fde85555e7b3db97 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Thu, 18 Jan 2018 14:39:55 -0800 Subject: [PATCH] Explicitly warn about uninitialized state before calling getDerivedStateFromProps. And added some new tests for this change. Also: * Improved a couple of falsy null/undefined checks to more explicitly check for null or undefined. * Made some small tweaks to ReactFiberClassComponent WRT when and how it reads instance.state and sets to null. --- .../__tests__/ReactComponentLifeCycle-test.js | 23 ++++++++ .../ReactDOMServerLifecycles-test.js | 21 ++++++++ .../src/server/ReactPartialRenderer.js | 18 ++++++- .../src/ReactFiberClassComponent.js | 54 +++++++++++++------ .../src/__tests__/ReactIncremental-test.js | 1 + .../src/ReactShallowRenderer.js | 24 +++++++++ .../__tests__/ReactShallowRenderer-test.js | 24 +++++++++ .../ReactCoffeeScriptClass-test.coffee | 18 +++++++ .../react/src/__tests__/ReactES6Class-test.js | 19 +++++++ .../__tests__/ReactTypeScriptClass-test.ts | 24 +++++++++ .../createReactClassIntegration-test.js | 44 ++++++++++++--- 11 files changed, 246 insertions(+), 24 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactComponentLifeCycle-test.js b/packages/react-dom/src/__tests__/ReactComponentLifeCycle-test.js index 87e7f1a9db42a..81591d82fb0ef 100644 --- a/packages/react-dom/src/__tests__/ReactComponentLifeCycle-test.js +++ b/packages/react-dom/src/__tests__/ReactComponentLifeCycle-test.js @@ -509,6 +509,7 @@ describe('ReactComponentLifeCycle', () => { }; }; class Outer extends React.Component { + state = {}; static getDerivedStateFromProps(props, prevState) { log.push('outer getDerivedStateFromProps'); return null; @@ -532,6 +533,7 @@ describe('ReactComponentLifeCycle', () => { } class Inner extends React.Component { + state = {}; static getDerivedStateFromProps(props, prevState) { log.push('inner getDerivedStateFromProps'); return null; @@ -646,6 +648,7 @@ describe('ReactComponentLifeCycle', () => { it('should warn if getDerivedStateFromProps returns undefined', () => { class MyComponent extends React.Component { + state = {}; static getDerivedStateFromProps() {} render() { return null; @@ -661,4 +664,24 @@ describe('ReactComponentLifeCycle', () => { // De-duped ReactDOM.render(, div); }); + + it('should warn if state is not initialized before getDerivedStateFromProps', () => { + class MyComponent extends React.Component { + static getDerivedStateFromProps() { + return null; + } + render() { + return null; + } + } + + const div = document.createElement('div'); + expect(() => ReactDOM.render(, div)).toWarnDev( + 'MyComponent: Did not properly initialize state during construction. ' + + 'Expected state to be an object, but it was undefined.', + ); + + // De-duped + ReactDOM.render(, div); + }); }); diff --git a/packages/react-dom/src/__tests__/ReactDOMServerLifecycles-test.js b/packages/react-dom/src/__tests__/ReactDOMServerLifecycles-test.js index 452b42f9852c9..3de800d0ace3c 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerLifecycles-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerLifecycles-test.js @@ -101,6 +101,7 @@ describe('ReactDOMServerLifecycles', () => { } class Child extends React.Component { + state = {}; static getDerivedStateFromProps() { return { qux: 'qux', @@ -119,6 +120,7 @@ describe('ReactDOMServerLifecycles', () => { it('should warn if getDerivedStateFromProps returns undefined', () => { class Component extends React.Component { + state = {}; static getDerivedStateFromProps() {} render() { return null; @@ -133,4 +135,23 @@ describe('ReactDOMServerLifecycles', () => { // De-duped ReactDOMServer.renderToString(); }); + + it('should warn if state is not initialized before getDerivedStateFromProps', () => { + class Component extends React.Component { + static getDerivedStateFromProps() { + return null; + } + render() { + return null; + } + } + + expect(() => ReactDOMServer.renderToString()).toWarnDev( + 'Component: Did not properly initialize state during construction. ' + + 'Expected state to be an object, but it was undefined.', + ); + + // De-duped + ReactDOMServer.renderToString(); + }); }); diff --git a/packages/react-dom/src/server/ReactPartialRenderer.js b/packages/react-dom/src/server/ReactPartialRenderer.js index 90961dd612395..58f7859765d5a 100644 --- a/packages/react-dom/src/server/ReactPartialRenderer.js +++ b/packages/react-dom/src/server/ReactPartialRenderer.js @@ -126,6 +126,7 @@ const didWarnAboutNoopUpdateForComponent = {}; const didWarnAboutBadClass = {}; const didWarnAboutDeprecatedWillMount = {}; const didWarnAboutUndefinedDerivedState = {}; +const didWarnAboutUninitializedState = {}; const valuePropNames = ['value', 'defaultValue']; const newlineEatingTags = { listing: true, @@ -426,6 +427,22 @@ function resolve( inst = new Component(element.props, publicContext, updater); if (typeof Component.getDerivedStateFromProps === 'function') { + if (__DEV__) { + if (inst.state === null || inst.state === undefined) { + const componentName = getComponentName(Component) || 'Unknown'; + if (!didWarnAboutUninitializedState[componentName]) { + warning( + false, + '%s: Did not properly initialize state during construction. ' + + 'Expected state to be an object, but it was %s.', + componentName, + inst.state === null ? 'null' : 'undefined', + ); + didWarnAboutUninitializedState[componentName] = true; + } + } + } + partialState = Component.getDerivedStateFromProps.call( null, element.props, @@ -435,7 +452,6 @@ function resolve( if (__DEV__) { if (partialState === undefined) { const componentName = getComponentName(Component) || 'Unknown'; - if (!didWarnAboutUndefinedDerivedState[componentName]) { warning( false, diff --git a/packages/react-reconciler/src/ReactFiberClassComponent.js b/packages/react-reconciler/src/ReactFiberClassComponent.js index 054b1a1914062..59274ec524389 100644 --- a/packages/react-reconciler/src/ReactFiberClassComponent.js +++ b/packages/react-reconciler/src/ReactFiberClassComponent.js @@ -46,6 +46,7 @@ let didWarnAboutLegacyWillReceiveProps; let didWarnAboutLegacyWillUpdate; let didWarnAboutStateAssignmentForComponent; let didWarnAboutUndefinedDerivedState; +let didWarnAboutUninitializedState; let didWarnAboutWillReceivePropsAndDerivedState; let warnOnInvalidCallback; @@ -57,6 +58,7 @@ if (__DEV__) { } didWarnAboutStateAssignmentForComponent = {}; didWarnAboutUndefinedDerivedState = {}; + didWarnAboutUninitializedState = {}; didWarnAboutWillReceivePropsAndDerivedState = {}; const didWarnOnInvalidCallback = {}; @@ -400,19 +402,48 @@ export default function( ? getMaskedContext(workInProgress, unmaskedContext) : emptyObject; const instance = new ctor(props, context); + const state = + instance.state !== null && instance.state !== undefined + ? instance.state + : null; adoptClassInstance(workInProgress, instance); + if (__DEV__) { + if ( + typeof ctor.getDerivedStateFromProps === 'function' && + state === null + ) { + const componentName = getComponentName(workInProgress) || 'Unknown'; + 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; + } + } + } + + workInProgress.memoizedState = state; + const partialState = callGetDerivedStateFromProps( workInProgress, instance, props, - instance.state, ); - if (partialState != null) { + + if (partialState !== null && partialState !== undefined) { // 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); + workInProgress.memoizedState = Object.assign( + {}, + workInProgress.memoizedState, + partialState, + ); } // Cache unmasked context so we can avoid recreating masked context unless necessary. @@ -519,12 +550,7 @@ export default function( } } - function callGetDerivedStateFromProps( - workInProgress, - instance, - props, - state, - ) { + function callGetDerivedStateFromProps(workInProgress, instance, props) { const {type} = workInProgress; if (typeof type.getDerivedStateFromProps === 'function') { @@ -550,7 +576,7 @@ export default function( const partialState = type.getDerivedStateFromProps.call( null, props, - state, + workInProgress.memoizedState, ); if (__DEV__) { @@ -584,12 +610,11 @@ export default function( } const instance = workInProgress.stateNode; - const state = instance.state || null; const props = workInProgress.pendingProps; const unmaskedContext = getUnmaskedContext(workInProgress); instance.props = props; - instance.state = workInProgress.memoizedState = state; + instance.state = workInProgress.memoizedState; instance.refs = emptyObject; instance.context = getMaskedContext(workInProgress, unmaskedContext); @@ -769,7 +794,6 @@ export default function( workInProgress, instance, newProps, - workInProgress.memoizedState, ); } @@ -790,12 +814,12 @@ export default function( newState = oldState; } - if (partialState != null) { + if (partialState !== null && partialState !== undefined) { // 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 = - newState == null + newState === null || newState === undefined ? partialState : Object.assign({}, newState, partialState); } diff --git a/packages/react-reconciler/src/__tests__/ReactIncremental-test.js b/packages/react-reconciler/src/__tests__/ReactIncremental-test.js index d9243ac2cf600..2400f1b500cb5 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncremental-test.js +++ b/packages/react-reconciler/src/__tests__/ReactIncremental-test.js @@ -1422,6 +1422,7 @@ describe('ReactIncremental', () => { let instance; class LifeCycle extends React.Component { + state = {}; static getDerivedStateFromProps(props, prevState) { ops.push('getDerivedStateFromProps'); return {foo: 'foo'}; diff --git a/packages/react-test-renderer/src/ReactShallowRenderer.js b/packages/react-test-renderer/src/ReactShallowRenderer.js index 2ece1bc261cf7..ff80a1ffa06a2 100644 --- a/packages/react-test-renderer/src/ReactShallowRenderer.js +++ b/packages/react-test-renderer/src/ReactShallowRenderer.js @@ -20,6 +20,7 @@ let didWarnAboutLegacyWillMount; let didWarnAboutLegacyWillReceiveProps; let didWarnAboutLegacyWillUpdate; let didWarnAboutUndefinedDerivedState; +let didWarnAboutUninitializedState; let didWarnAboutWillReceivePropsAndDerivedState; if (__DEV__) { @@ -29,6 +30,7 @@ if (__DEV__) { didWarnAboutLegacyWillUpdate = {}; } didWarnAboutUndefinedDerivedState = {}; + didWarnAboutUninitializedState = {}; didWarnAboutWillReceivePropsAndDerivedState = {}; } @@ -100,6 +102,28 @@ class ReactShallowRenderer { this._updater, ); + if (__DEV__) { + if (typeof element.type.getDerivedStateFromProps === 'function') { + if ( + this._instance.state === null || + this._instance.state === undefined + ) { + const componentName = + getName(element.type, this._instance) || 'Unknown'; + if (!didWarnAboutUninitializedState[componentName]) { + warning( + false, + '%s: Did not properly initialize state during construction. ' + + 'Expected state to be an object, but it was %s.', + componentName, + this._instance.state === null ? 'null' : 'undefined', + ); + didWarnAboutUninitializedState[componentName] = true; + } + } + } + } + this._updateStateFromStaticLifecycle(element.props); if (element.type.hasOwnProperty('contextTypes')) { diff --git a/packages/react-test-renderer/src/__tests__/ReactShallowRenderer-test.js b/packages/react-test-renderer/src/__tests__/ReactShallowRenderer-test.js index e363eca6936e1..02532ec6fb799 100644 --- a/packages/react-test-renderer/src/__tests__/ReactShallowRenderer-test.js +++ b/packages/react-test-renderer/src/__tests__/ReactShallowRenderer-test.js @@ -16,6 +16,8 @@ let React; describe('ReactShallowRenderer', () => { beforeEach(() => { + jest.resetModules(); + createRenderer = require('react-test-renderer/shallow').createRenderer; PropTypes = require('prop-types'); React = require('react'); @@ -26,6 +28,7 @@ describe('ReactShallowRenderer', () => { const logger = message => () => logs.push(message) || true; class SomeComponent extends React.Component { + state = {}; static getDerivedStateFromProps = logger('getDerivedStateFromProps'); UNSAFE_componentWillMount = logger('componentWillMount'); componentDidMount = logger('componentDidMount'); @@ -1095,6 +1098,7 @@ describe('ReactShallowRenderer', () => { it('should warn if getDerivedStateFromProps returns undefined', () => { class Component extends React.Component { + state = {}; static getDerivedStateFromProps() {} render() { return null; @@ -1110,4 +1114,24 @@ describe('ReactShallowRenderer', () => { // De-duped shallowRenderer.render(); }); + + it('should warn if state not initialized before getDerivedStateFromProps', () => { + class Component extends React.Component { + static getDerivedStateFromProps() { + return null; + } + render() { + return null; + } + } + + const shallowRenderer = createRenderer(); + expect(() => shallowRenderer.render()).toWarnDev( + 'Component: Did not properly initialize state during construction. ' + + 'Expected state to be an object, but it was undefined.', + ); + + // De-duped + shallowRenderer.render(); + }); }); diff --git a/packages/react/src/__tests__/ReactCoffeeScriptClass-test.coffee b/packages/react/src/__tests__/ReactCoffeeScriptClass-test.coffee index 3bfe8a87fde34..45c9b6ba6a98c 100644 --- a/packages/react/src/__tests__/ReactCoffeeScriptClass-test.coffee +++ b/packages/react/src/__tests__/ReactCoffeeScriptClass-test.coffee @@ -100,6 +100,9 @@ describe 'ReactCoffeeScriptClass', -> it 'sets initial state with value returned by static getDerivedStateFromProps', -> class Foo extends React.Component + constructor: (props) -> + super props + @state = foo: null render: -> div className: "#{@state.foo} #{@state.bar}" @@ -111,6 +114,21 @@ describe 'ReactCoffeeScriptClass', -> test React.createElement(Foo, foo: 'foo'), 'DIV', 'foo bar' undefined + it 'warns if state not initialized before static getDerivedStateFromProps', -> + class Foo extends React.Component + render: -> + div + className: "#{@state.foo} #{@state.bar}" + Foo.getDerivedStateFromProps = (nextProps, prevState) -> + { + foo: nextProps.foo + bar: 'bar' + } + expect(-> + ReactDOM.render(React.createElement(Foo, foo: 'foo'), container) + ).toWarnDev 'Foo: Did not properly initialize state during construction. Expected state to be an object, but it was undefined.' + undefined + it 'updates initial state with values returned by static getDerivedStateFromProps', -> class Foo extends React.Component constructor: (props, context) -> diff --git a/packages/react/src/__tests__/ReactES6Class-test.js b/packages/react/src/__tests__/ReactES6Class-test.js index 634326235db8b..e65ce4e6b5788 100644 --- a/packages/react/src/__tests__/ReactES6Class-test.js +++ b/packages/react/src/__tests__/ReactES6Class-test.js @@ -111,6 +111,7 @@ describe('ReactES6Class', () => { it('sets initial state with value returned by static getDerivedStateFromProps', () => { class Foo extends React.Component { + state = {}; static getDerivedStateFromProps(nextProps, prevState) { return { foo: nextProps.foo, @@ -124,6 +125,24 @@ describe('ReactES6Class', () => { test(, 'DIV', 'foo bar'); }); + it('warns if state not initialized before static getDerivedStateFromProps', () => { + class Foo extends React.Component { + static getDerivedStateFromProps(nextProps, prevState) { + return { + foo: nextProps.foo, + bar: 'bar', + }; + } + render() { + return
; + } + } + expect(() => ReactDOM.render(, container)).toWarnDev( + 'Foo: Did not properly initialize state during construction. ' + + 'Expected state to be an object, but it was undefined.', + ); + }); + it('updates initial state with values returned by static getDerivedStateFromProps', () => { class Foo extends React.Component { state = { diff --git a/packages/react/src/__tests__/ReactTypeScriptClass-test.ts b/packages/react/src/__tests__/ReactTypeScriptClass-test.ts index 6fd47da5de4c3..45757c98f6ff5 100644 --- a/packages/react/src/__tests__/ReactTypeScriptClass-test.ts +++ b/packages/react/src/__tests__/ReactTypeScriptClass-test.ts @@ -356,6 +356,10 @@ describe('ReactTypeScriptClass', function() { it('sets initial state with value returned by static getDerivedStateFromProps', function() { class Foo extends React.Component { + state = { + foo: null, + bar: null, + }; static getDerivedStateFromProps(nextProps, prevState) { return { foo: nextProps.foo, @@ -369,6 +373,26 @@ describe('ReactTypeScriptClass', function() { test(React.createElement(Foo, {foo: "foo"}), 'DIV', 'foo bar'); }); + it('warns if state not initialized before 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}`}); + } + } + expect(function() { + ReactDOM.render(React.createElement(Foo, {foo: "foo"}), container); + }).toWarnDev( + 'Foo: Did not properly initialize state during construction. ' + + 'Expected state to be an object, but it was undefined.' + ); + }); + it('updates initial state with values returned by static getDerivedStateFromProps', function() { class Foo extends React.Component { state = { diff --git a/packages/react/src/__tests__/createReactClassIntegration-test.js b/packages/react/src/__tests__/createReactClassIntegration-test.js index ee092b8855ca7..aa46acf4fc476 100644 --- a/packages/react/src/__tests__/createReactClassIntegration-test.js +++ b/packages/react/src/__tests__/createReactClassIntegration-test.js @@ -17,6 +17,7 @@ let createReactClass; describe('create-react-class-integration', () => { beforeEach(() => { + jest.resetModules(); PropTypes = require('prop-types'); React = require('react'); ReactDOM = require('react-dom'); @@ -269,13 +270,16 @@ describe('create-react-class-integration', () => { it('should work with getDerivedStateFromProps() return values', () => { const Component = createReactClass({ + getInitialState() { + return {}; + }, render: function() { return ; }, }); - Component.getDerivedStateFromProps = () => ({ - occupation: 'clown', - }); + Component.getDerivedStateFromProps = () => { + return {occupation: 'clown'}; + }; let instance = ; instance = ReactTestUtils.renderIntoDocument(instance); expect(instance.state.occupation).toEqual('clown'); @@ -378,19 +382,23 @@ describe('create-react-class-integration', () => { it('getDerivedStateFromProps updates state when props change', () => { const Component = createReactClass({ + getInitialState() { + return { + count: 1, + }; + }, render() { return
count:{this.state.count}
; }, }); - Component.getDerivedStateFromProps = (nextProps, prevState) => - prevState === null - ? {count: 1} - : {count: prevState.count + nextProps.incrementBy}; + Component.getDerivedStateFromProps = (nextProps, prevState) => ({ + count: prevState.count + nextProps.incrementBy, + }); const container = document.createElement('div'); const instance = ReactDOM.render(
- +
, container, ); @@ -413,6 +421,10 @@ describe('create-react-class-integration', () => { }, }, + getInitialState() { + return {}; + }, + render: function() { instance = this; return null; @@ -421,4 +433,20 @@ describe('create-react-class-integration', () => { ReactDOM.render(, document.createElement('div')); expect(instance.state.foo).toBe('bar'); }); + + it('should warn if state is not properly initialized before getDerivedStateFromProps', () => { + const Component = createReactClass({ + statics: { + getDerivedStateFromProps: function() { + return null; + }, + }, + render: function() { + return null; + }, + }); + expect(() => + ReactDOM.render(, document.createElement('div')), + ).toWarnDev('Did not properly initialize state during construction.'); + }); });