Skip to content

Commit

Permalink
Explicitly warn about uninitialized state before calling getDerivedSt…
Browse files Browse the repository at this point in the history
…ateFromProps.

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.
  • Loading branch information
bvaughn committed Jan 18, 2018
1 parent 8d67e27 commit 53770c3
Show file tree
Hide file tree
Showing 11 changed files with 246 additions and 24 deletions.
23 changes: 23 additions & 0 deletions packages/react-dom/src/__tests__/ReactComponentLifeCycle-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -509,6 +509,7 @@ describe('ReactComponentLifeCycle', () => {
};
};
class Outer extends React.Component {
state = {};
static getDerivedStateFromProps(props, prevState) {
log.push('outer getDerivedStateFromProps');
return null;
Expand All @@ -532,6 +533,7 @@ describe('ReactComponentLifeCycle', () => {
}

class Inner extends React.Component {
state = {};
static getDerivedStateFromProps(props, prevState) {
log.push('inner getDerivedStateFromProps');
return null;
Expand Down Expand Up @@ -646,6 +648,7 @@ describe('ReactComponentLifeCycle', () => {

it('should warn if getDerivedStateFromProps returns undefined', () => {
class MyComponent extends React.Component {
state = {};
static getDerivedStateFromProps() {}
render() {
return null;
Expand All @@ -661,4 +664,24 @@ describe('ReactComponentLifeCycle', () => {
// De-duped
ReactDOM.render(<MyComponent />, 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(<MyComponent />, div)).toWarnDev(
'MyComponent: Did not properly initialize state during construction. ' +
'Expected state to be an object, but it was undefined.',
);

// De-duped
ReactDOM.render(<MyComponent />, div);
});
});
21 changes: 21 additions & 0 deletions packages/react-dom/src/__tests__/ReactDOMServerLifecycles-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ describe('ReactDOMServerLifecycles', () => {
}

class Child extends React.Component {
state = {};
static getDerivedStateFromProps() {
return {
qux: 'qux',
Expand All @@ -119,6 +120,7 @@ describe('ReactDOMServerLifecycles', () => {

it('should warn if getDerivedStateFromProps returns undefined', () => {
class Component extends React.Component {
state = {};
static getDerivedStateFromProps() {}
render() {
return null;
Expand All @@ -133,4 +135,23 @@ describe('ReactDOMServerLifecycles', () => {
// De-duped
ReactDOMServer.renderToString(<Component />);
});

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(<Component />)).toWarnDev(
'Component: Did not properly initialize state during construction. ' +
'Expected state to be an object, but it was undefined.',
);

// De-duped
ReactDOMServer.renderToString(<Component />);
});
});
18 changes: 17 additions & 1 deletion packages/react-dom/src/server/ReactPartialRenderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,7 @@ const didWarnAboutNoopUpdateForComponent = {};
const didWarnAboutBadClass = {};
const didWarnAboutDeprecatedWillMount = {};
const didWarnAboutUndefinedDerivedState = {};
const didWarnAboutUninitializedState = {};
const valuePropNames = ['value', 'defaultValue'];
const newlineEatingTags = {
listing: true,
Expand Down Expand Up @@ -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,
Expand All @@ -435,7 +452,6 @@ function resolve(
if (__DEV__) {
if (partialState === undefined) {
const componentName = getComponentName(Component) || 'Unknown';

if (!didWarnAboutUndefinedDerivedState[componentName]) {
warning(
false,
Expand Down
54 changes: 39 additions & 15 deletions packages/react-reconciler/src/ReactFiberClassComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ let didWarnAboutLegacyWillReceiveProps;
let didWarnAboutLegacyWillUpdate;
let didWarnAboutStateAssignmentForComponent;
let didWarnAboutUndefinedDerivedState;
let didWarnAboutUninitializedState;
let didWarnAboutWillReceivePropsAndDerivedState;
let warnOnInvalidCallback;

Expand All @@ -57,6 +58,7 @@ if (__DEV__) {
}
didWarnAboutStateAssignmentForComponent = {};
didWarnAboutUndefinedDerivedState = {};
didWarnAboutUninitializedState = {};
didWarnAboutWillReceivePropsAndDerivedState = {};

const didWarnOnInvalidCallback = {};
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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') {
Expand All @@ -550,7 +576,7 @@ export default function(
const partialState = type.getDerivedStateFromProps.call(
null,
props,
state,
workInProgress.memoizedState,
);

if (__DEV__) {
Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -769,7 +794,6 @@ export default function(
workInProgress,
instance,
newProps,
workInProgress.memoizedState,
);
}

Expand All @@ -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);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1422,6 +1422,7 @@ describe('ReactIncremental', () => {
let instance;

class LifeCycle extends React.Component {
state = {};
static getDerivedStateFromProps(props, prevState) {
ops.push('getDerivedStateFromProps');
return {foo: 'foo'};
Expand Down
24 changes: 24 additions & 0 deletions packages/react-test-renderer/src/ReactShallowRenderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ let didWarnAboutLegacyWillMount;
let didWarnAboutLegacyWillReceiveProps;
let didWarnAboutLegacyWillUpdate;
let didWarnAboutUndefinedDerivedState;
let didWarnAboutUninitializedState;
let didWarnAboutWillReceivePropsAndDerivedState;

if (__DEV__) {
Expand All @@ -29,6 +30,7 @@ if (__DEV__) {
didWarnAboutLegacyWillUpdate = {};
}
didWarnAboutUndefinedDerivedState = {};
didWarnAboutUninitializedState = {};
didWarnAboutWillReceivePropsAndDerivedState = {};
}

Expand Down Expand Up @@ -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')) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand All @@ -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');
Expand Down Expand Up @@ -1095,6 +1098,7 @@ describe('ReactShallowRenderer', () => {

it('should warn if getDerivedStateFromProps returns undefined', () => {
class Component extends React.Component {
state = {};
static getDerivedStateFromProps() {}
render() {
return null;
Expand All @@ -1110,4 +1114,24 @@ describe('ReactShallowRenderer', () => {
// De-duped
shallowRenderer.render(<Component />);
});

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(<Component />)).toWarnDev(
'Component: Did not properly initialize state during construction. ' +
'Expected state to be an object, but it was undefined.',
);

// De-duped
shallowRenderer.render(<Component />);
});
});
18 changes: 18 additions & 0 deletions packages/react/src/__tests__/ReactCoffeeScriptClass-test.coffee
Original file line number Diff line number Diff line change
Expand Up @@ -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}"
Expand All @@ -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) ->
Expand Down
Loading

0 comments on commit 53770c3

Please sign in to comment.