Warn when calling setState & other methods at wrong time
Currently we use an invariant to prevent this code pattern. That is really aggressive for something that doesn't actually put React in a bad state. This diff replaces invariants with warnings and makes those code paths no-ops.
Paul O’Shannessy committed
Feb 9, 2015 at 17:04 UTC
5967915ec4783f083250d796e2cf62ac64bab086
3 files changed
+98
-56
src/core/ReactUpdateQueue.js
+53
-22
@@ -19,6 +19,7 @@ var ReactUpdates = require('ReactUpdates');
19
20
var assign = require('Object.assign');
21
var invariant = require('invariant');
22
+var warning = require('warning');
23
24
function enqueueUpdate(internalInstance) {
25
if (internalInstance !== ReactLifeCycle.currentlyMountingInstance) {
@@ -40,20 +41,27 @@ function getInternalInstanceReadyForUpdate(publicInstance, callerName) {
41
);
42
43
var internalInstance = ReactInstanceMap.get(publicInstance);
43
- invariant(
44
- internalInstance,
45
- '%s(...): Can only update a mounted or mounting component. ' +
46
- 'This usually means you called %s() on an unmounted ' +
47
- 'component.',
48
- callerName,
49
- callerName
50
- );
51
- invariant(
52
- internalInstance !== ReactLifeCycle.currentlyUnmountingInstance,
53
- '%s(...): Cannot call %s() on an unmounting component.',
54
- callerName,
55
- callerName
56
- );
44
+ if (!internalInstance) {
45
+ if (__DEV__) {
46
+ // Only warn when we have a callerName. Otherwise we should be silent.
47
+ // We're probably calling from enqueueCallback. We don't want to warn
48
+ // there because we already warned for the corresponding lifecycle method.
49
+ warning(
50
+ !callerName,
51
+ '%s(...): Can only update a mounted or mounting component. ' +
52
+ 'This usually means you called %s() on an unmounted ' +
53
+ 'component. This is a no-op.',
54
+ callerName,
55
+ callerName
56
+ );
57
+ }
58
+ return null;
59
+ }
60
+
61
+ if (internalInstance === ReactLifeCycle.currentlyUnmountingInstance) {
62
+ return null;
63
+ }
64
+
65
return internalInstance;
66
}
67
@@ -78,15 +86,18 @@ var ReactUpdateQueue = {
86
'`setState`, `replaceState`, or `forceUpdate` with a callback that ' +
87
'isn\'t callable.'
88
);
81
- var internalInstance = ReactInstanceMap.get(publicInstance);
82
- invariant(
83
- internalInstance,
84
- 'Cannot enqueue a callback on an instance that is unmounted.'
85
- );
86
- if (internalInstance === ReactLifeCycle.currentlyMountingInstance) {
87
- // Ignore callbacks in componentWillMount. See enqueueUpdate.
88
- return;
89
+ var internalInstance = getInternalInstanceReadyForUpdate(publicInstance);
90
+
91
+ // Previously we would throw an error if we didn't have an internal
92
+ // instance. Since we want to make it a no-op instead, we mirror the same
93
+ // behavior we have in other enqueue* methods.
94
+ // We also need to ignore callbacks in componentWillMount. See
95
+ // enqueueUpdates.
96
+ if (!internalInstance ||
97
+ internalInstance === ReactLifeCycle.currentlyMountingInstance) {
98
+ return null;
99
}
100
+
101
if (internalInstance._pendingCallbacks) {
102
internalInstance._pendingCallbacks.push(callback);
103
} else {
@@ -133,6 +144,10 @@ var ReactUpdateQueue = {
144
'forceUpdate'
145
);
146
147
+ if (!internalInstance) {
148
+ return;
149
+ }
150
+
151
internalInstance._pendingForceUpdate = true;
152
153
enqueueUpdate(internalInstance);
@@ -155,6 +170,10 @@ var ReactUpdateQueue = {
170
'replaceState'
171
);
172
173
+ if (!internalInstance) {
174
+ return;
175
+ }
176
+
177
internalInstance._pendingStateQueue = [completeState];
178
internalInstance._pendingReplaceState = true;
179
@@ -177,6 +196,10 @@ var ReactUpdateQueue = {
196
'setState'
197
);
198
199
+ if (!internalInstance) {
200
+ return;
201
+ }
202
+
203
var queue =
204
internalInstance._pendingStateQueue ||
205
(internalInstance._pendingStateQueue = []);
@@ -198,6 +221,10 @@ var ReactUpdateQueue = {
221
'setProps'
222
);
223
224
+ if (!internalInstance) {
225
+ return;
226
+ }
227
+
228
invariant(
229
internalInstance._isTopLevel,
230
'setProps(...): You called `setProps` on a ' +
@@ -233,6 +260,10 @@ var ReactUpdateQueue = {
260
'replaceProps'
261
);
262
263
+ if (!internalInstance) {
264
+ return;
265
+ }
266
+
267
invariant(
268
internalInstance._isTopLevel,
269
'replaceProps(...): You called `replaceProps` on a ' +
src/core/__tests__/ReactComponentLifeCycle-test.js
+6
-5
@@ -239,6 +239,7 @@ describe('ReactComponentLifeCycle', function() {
239
});
240
241
it('should not allow update state inside of getInitialState', function() {
242
+ spyOn(console, 'warn');
243
var StatefulComponent = React.createClass({
244
getInitialState: function() {
245
this.setState({stateField: 'something'});
@@ -251,12 +252,12 @@ describe('ReactComponentLifeCycle', function() {
252
);
253
}
254
});
254
- expect(function() {
255
- ReactTestUtils.renderIntoDocument(<StatefulComponent />);
256
- }).toThrow(
257
- 'Invariant Violation: setState(...): Can only update a mounted or ' +
255
+ ReactTestUtils.renderIntoDocument(<StatefulComponent />);
256
+ expect(console.warn.calls.length).toBe(1);
257
+ expect(console.warn.argsForCall[0][0]).toBe(
258
+ 'Warning: setState(...): Can only update a mounted or ' +
259
'mounting component. This usually means you called setState() on an ' +
259
- 'unmounted component.'
260
+ 'unmounted component. This is a no-op.'
261
);
262
});
263
src/core/__tests__/ReactCompositeComponent-test.js
+39
-29
@@ -262,7 +262,7 @@ describe('ReactCompositeComponent', function() {
262
expect(inputProps.prop).not.toBeDefined();
263
});
264
265
- it('should not allow `forceUpdate` on unmounted components', function() {
265
+ it('should warn about `forceUpdate` on unmounted components', function() {
266
var container = document.createElement('div');
267
document.documentElement.appendChild(container);
268
@@ -276,21 +276,22 @@ describe('ReactCompositeComponent', function() {
276
expect(instance.forceUpdate).not.toBeDefined();
277
278
instance = React.render(instance, container);
279
- expect(function() {
280
- instance.forceUpdate();
281
- }).not.toThrow();
279
+ instance.forceUpdate();
280
+
281
+ expect(console.warn.calls.length).toBe(0);
282
283
React.unmountComponentAtNode(container);
284
- expect(function() {
285
- instance.forceUpdate();
286
- }).toThrow(
287
- 'Invariant Violation: forceUpdate(...): Can only update a mounted or ' +
284
+
285
+ instance.forceUpdate();
286
+ expect(console.warn.calls.length).toBe(1);
287
+ expect(console.warn.argsForCall[0][0]).toBe(
288
+ 'Warning: forceUpdate(...): Can only update a mounted or ' +
289
'mounting component. This usually means you called forceUpdate() on ' +
289
- 'an unmounted component.'
290
+ 'an unmounted component. This is a no-op.'
291
);
292
});
293
293
- it('should not allow `setState` on unmounted components', function() {
294
+ it('should warn about `setState` on unmounted components', function() {
295
var container = document.createElement('div');
296
document.documentElement.appendChild(container);
297
@@ -307,21 +308,23 @@ describe('ReactCompositeComponent', function() {
308
expect(instance.setState).not.toBeDefined();
309
310
instance = React.render(instance, container);
310
- expect(function() {
311
- instance.setState({ value: 1 });
312
- }).not.toThrow();
311
+ instance.setState({ value: 1 });
312
+
313
+ expect(console.warn.calls.length).toBe(0);
314
315
React.unmountComponentAtNode(container);
315
- expect(function() {
316
- instance.setState({ value: 2 });
317
- }).toThrow(
318
- 'Invariant Violation: setState(...): Can only update a mounted or ' +
316
+ instance.setState({ value: 2 });
317
+ expect(console.warn.calls.length).toBe(1);
318
+ expect(console.warn.argsForCall[0][0]).toBe(
319
+ 'Warning: setState(...): Can only update a mounted or ' +
320
'mounting component. This usually means you called setState() on an ' +
320
- 'unmounted component.'
321
+ 'unmounted component. This is a no-op.'
322
);
323
});
324
324
- it('should not allow `setState` on unmounting components', function() {
325
+ it('should silently allow `setState`, not call cb on unmounting components',
326
+ function() {
327
+ var cbCalled = false;
328
var container = document.createElement('div');
329
document.documentElement.appendChild(container);
330
@@ -330,10 +333,11 @@ describe('ReactCompositeComponent', function() {
333
return { value: 0 };
334
},
335
componentWillUnmount: function() {
333
- expect(() => this.setState({ value: 2 })).toThrow(
334
- 'Invariant Violation: setState(...): Cannot call setState() on an ' +
335
- 'unmounting component.'
336
- );
336
+ expect(() => {
337
+ this.setState({ value: 2 }, function() {
338
+ cbCalled = true;
339
+ })
340
+ }).not.toThrow();
341
},
342
render: function() {
343
return <div />;
@@ -341,11 +345,13 @@ describe('ReactCompositeComponent', function() {
345
});
346
347
var instance = React.render(<Component />, container);
344
- expect(function() {
345
- instance.setState({ value: 1 });
346
- }).not.toThrow();
348
+
349
+ instance.setState({ value: 1 });
350
+ expect(console.warn.calls.length).toBe(0);
351
352
React.unmountComponentAtNode(container);
353
+ expect(console.warn.calls.length).toBe(0);
354
+ expect(cbCalled).toBe(false);
355
});
356
357
it('should not allow `setProps` on unmounted components', function() {
@@ -365,14 +371,18 @@ describe('ReactCompositeComponent', function() {
371
expect(function() {
372
instance.setProps({ value: 1 });
373
}).not.toThrow();
374
+ expect(console.warn.calls.length).toBe(0);
375
376
React.unmountComponentAtNode(container);
377
expect(function() {
378
instance.setProps({ value: 2 });
372
- }).toThrow(
373
- 'Invariant Violation: setProps(...): Can only update a mounted or ' +
379
+ }).not.toThrow();
380
+
381
+ expect(console.warn.calls.length).toBe(1);
382
+ expect(console.warn.argsForCall[0][0]).toBe(
383
+ 'Warning: setProps(...): Can only update a mounted or ' +
384
'mounting component. This usually means you called setProps() on an ' +
375
- 'unmounted component.'
385
+ 'unmounted component. This is a no-op.'
386
);
387
});
388