Improve not-yet-mounted setState warning #12531
Conversation
| '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.', |
There was a problem hiding this comment.
Even though this one is for forceUpdate I figured we should push people to use this.state = ... and setState here anyway.
There was a problem hiding this comment.
Yeah, no strong feelings about this I guess.
| @@ -225,6 +225,86 @@ describe('ReactCompositeComponent', () => { | |||
| expect(inputProps.prop).not.toBeDefined(); | |||
| }); | |||
|
|
|||
| it('should warn about `forceUpdate` on not-yet-mounted components', () => { | |||
There was a problem hiding this comment.
Note these are not new warnings. Just new tests for an existing warning.
| callerName, | ||
| "Can't call %s on a component that is not yet mounted. " + | ||
| 'This is a no-op, but it might indicate a bug in your application. ' + | ||
| 'To fix, assign the initial state in the %s constructor. ' + |
There was a problem hiding this comment.
nit: Might read nicer with "\n\n" between these two sections?
| '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. " + |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Since we don't want to encourage those methods (and will start warning for them in the future) I wouldn't worry about them.
| '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.', |
There was a problem hiding this comment.
Yeah, no strong feelings about this I guess.
|
@sophiebits Yes, it's just for constructor. The renderer will later override the |
| '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. " + | ||
| 'This is a no-op, but it might indicate a bug in your application.\n\n' + |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I think I see what you mean. If this only affects constructor we might as well not talk about this.
* Tweak not-yet-mounted setState warning * Add \n\n
This is similar to #12347, but for when you call
setStateorforceUpdatetoo early. I tweaked the wording to have more actionable suggestions.