Skip to content
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

Improve not-yet-mounted setState warning #12531

Merged
merged 2 commits into from
Apr 3, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions packages/react-dom/src/__tests__/ReactComponentLifeCycle-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -214,10 +214,12 @@ describe('ReactComponentLifeCycle', () => {
expect(() => {
ReactTestUtils.renderIntoDocument(<StatefulComponent />);
}).toWarnDev(
'Warning: setState(...): Can only update a mounted or ' +
'mounting component. This usually means you called setState() on an ' +
'unmounted component. This is a no-op.\n\nPlease check the code for the ' +
'StatefulComponent component.',
"Warning: Can't call setState on a component that is not yet mounted. " +
'This is a no-op, but it might indicate a bug in your application.\n\n' +
'To fix, assign the initial state in the StatefulComponent constructor. ' +
'If the state needs to reflect an external data source, ' +
'you may also add a componentDidMount lifecycle hook to StatefulComponent ' +
'and call setState there if the external data has changed.',
);

// Check deduplication; (no extra warnings should be logged).
Expand Down
52 changes: 52 additions & 0 deletions packages/react-dom/src/__tests__/ReactCompositeComponent-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,58 @@ describe('ReactCompositeComponent', () => {
expect(inputProps.prop).not.toBeDefined();
});

it('should warn about `forceUpdate` on not-yet-mounted components', () => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note these are not new warnings. Just new tests for an existing warning.

class MyComponent extends React.Component {
constructor(props) {
super(props);
this.forceUpdate();
}
render() {
return <div />;
}
}

const container = document.createElement('div');
expect(() => ReactDOM.render(<MyComponent />, container)).toWarnDev(
"Warning: Can't call forceUpdate on a component that is not yet mounted. " +
'This is a no-op, but it might indicate a bug in your application.\n\n' +
'To fix, assign the initial state in the MyComponent constructor. ' +
'If the state needs to reflect an external data source, ' +
'you may also add a componentDidMount lifecycle hook to MyComponent ' +
'and call setState there if the external data has changed.',
);

// No additional warning should be recorded
const container2 = document.createElement('div');
ReactDOM.render(<MyComponent />, container2);
});

it('should warn about `setState` on not-yet-mounted components', () => {
class MyComponent extends React.Component {
constructor(props) {
super(props);
this.setState();
}
render() {
return <div />;
}
}

const container = document.createElement('div');
expect(() => ReactDOM.render(<MyComponent />, container)).toWarnDev(
"Warning: Can't call setState on a component that is not yet mounted. " +
'This is a no-op, but it might indicate a bug in your application.\n\n' +
'To fix, assign the initial state in the MyComponent constructor. ' +
'If the state needs to reflect an external data source, ' +
'you may also add a componentDidMount lifecycle hook to MyComponent ' +
'and call setState there if the external data has changed.',
);

// No additional warning should be recorded
const container2 = document.createElement('div');
ReactDOM.render(<MyComponent />, container2);
});

it('should warn about `forceUpdate` on unmounted components', () => {
const container = document.createElement('div');
document.body.appendChild(container);
Expand Down
11 changes: 7 additions & 4 deletions packages/react/src/ReactNoopUpdateQueue.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,15 @@ function warnNoop(publicInstance, callerName) {
}
warning(
false,
'%s(...): Can only update a mounted or mounting component. ' +
'This usually means you called %s() on an unmounted component. ' +
'This is a no-op.\n\nPlease check the code for the %s component.',
callerName,
"Can't call %s on a component that is not yet mounted. " +
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, I like this message better- but this first sentence seems less accurate and maybe confusing, since you can call setState from componentWillMount/UNSAFE_componentWillMount.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we don't want to encourage those methods (and will start warning for them in the future) I wouldn't worry about them.

'This is a no-op, but it might indicate a bug in your application.\n\n' +
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't call %s on a component before mounting it. This is a no-op, but it likely indicates a bug in your application. Instead, assign to `this.state` directly or define a `state = {};` class property with the desired state.

^ Maybe? It looks a little wordy.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean you want to drop the second part? I thought it's kind of important because these issues will get more prominent with async.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I see what you mean. If this only affects constructor we might as well not talk about this.

'To fix, assign the initial state in the %s constructor. ' +
'If the state needs to reflect an external data source, ' +
'you may also add a componentDidMount lifecycle hook to %s ' +
'and call setState there if the external data has changed.',
callerName,
componentName,
componentName,
);
didWarnStateUpdateForUnmountedComponent[warningKey] = true;
}
Expand Down