Warn about reassigning this.props (#14277)
* Warn about reassigning this.props * Improve the warning * Don't show the spammy bug warning if we suspect it's a component bug
Dan Abramov committed
Nov 20, 2018 at 16:40 UTC
a9fdf8a32650d6238a6d9b055f3ac68e88090cec
3 files changed
+92
-21
packages/react-dom/src/__tests__/ReactCompositeComponent-test.js
+19
@@ -1743,6 +1743,25 @@ describe('ReactCompositeComponent', () => {
1743
);
1744
});
1745
1746
+ it('should warn about reassigning this.props while rendering', () => {
1747
+ class Bad extends React.Component {
1748
+ componentDidMount() {}
1749
+ componentDidUpdate() {}
1750
+ render() {
1751
+ this.props = {...this.props};
1752
+ return null;
1753
+ }
1754
+ }
1755
+
1756
+ const container = document.createElement('div');
1757
+ expect(() => {
1758
+ ReactDOM.render(<Bad />, container);
1759
+ }).toWarnDev(
1760
+ 'It looks like Bad is reassigning its own `this.props` while rendering. ' +
1761
+ 'This is not supported and can lead to confusing bugs.',
1762
+ );
1763
+ });
1764
+
1765
it('should return error if render is not defined', () => {
1766
class RenderTestUndefinedRender extends React.Component {}
1767
packages/react-reconciler/src/ReactFiberBeginWork.js
+16
-1
@@ -130,12 +130,14 @@ let didWarnAboutBadClass;
130
let didWarnAboutContextTypeOnFunctionComponent;
131
let didWarnAboutGetDerivedStateOnFunctionComponent;
132
let didWarnAboutFunctionRefs;
133
+export let didWarnAboutReassigningProps;
134
135
if (__DEV__) {
136
didWarnAboutBadClass = {};
137
didWarnAboutContextTypeOnFunctionComponent = {};
138
didWarnAboutGetDerivedStateOnFunctionComponent = {};
139
didWarnAboutFunctionRefs = {};
140
+ didWarnAboutReassigningProps = false;
141
}
142
143
export function reconcileChildren(
@@ -495,7 +497,7 @@ function updateClassComponent(
497
renderExpirationTime,
498
);
499
}
498
- return finishClassComponent(
500
+ const nextUnitOfWork = finishClassComponent(
501
current,
502
workInProgress,
503
Component,
@@ -503,6 +505,19 @@ function updateClassComponent(
505
hasContext,
506
renderExpirationTime,
507
);
508
+ if (__DEV__) {
509
+ let inst = workInProgress.stateNode;
510
+ if (inst.props !== nextProps) {
511
+ warning(
512
+ didWarnAboutReassigningProps,
513
+ 'It looks like %s is reassigning its own `this.props` while rendering. ' +
514
+ 'This is not supported and can lead to confusing bugs.',
515
+ getComponentName(workInProgress.type) || 'a component',
516
+ );
517
+ didWarnAboutReassigningProps = true;
518
+ }
519
+ }
520
+ return nextUnitOfWork;
521
}
522
523
function finishClassComponent(
packages/react-reconciler/src/ReactFiberCommitWork.js
+57
-20
@@ -99,6 +99,7 @@ import {
99
UnmountPassive,
100
MountPassive,
101
} from './ReactHookEffectTags';
102
+import {didWarnAboutReassigningProps} from './ReactFiberBeginWork';
103
104
let didWarnAboutUndefinedSnapshotBeforeUpdate: Set<mixed> | null = null;
105
if (__DEV__) {
@@ -234,18 +235,27 @@ function commitBeforeMutationLifeCycles(
235
// but instead we rely on them being set during last render.
236
// TODO: revisit this when we implement resuming.
237
if (__DEV__) {
237
- if (finishedWork.type === finishedWork.elementType) {
238
+ if (
239
+ finishedWork.type === finishedWork.elementType &&
240
+ !didWarnAboutReassigningProps
241
+ ) {
242
warning(
243
instance.props === finishedWork.memoizedProps,
240
- 'Expected instance props to match memoized props before ' +
241
- 'getSnapshotBeforeUpdate. This is likely due to a bug in React. ' +
244
+ 'Expected %s props to match memoized props before ' +
245
+ 'getSnapshotBeforeUpdate. ' +
246
+ 'This might either be because of a bug in React, or because ' +
247
+ 'a component reassigns its own `this.props`. ' +
248
'Please file an issue.',
249
+ getComponentName(finishedWork.type) || 'instance',
250
);
251
warning(
252
instance.state === finishedWork.memoizedState,
246
- 'Expected instance state to match memoized state before ' +
247
- 'getSnapshotBeforeUpdate. This is likely due to a bug in React. ' +
253
+ 'Expected %s state to match memoized state before ' +
254
+ 'getSnapshotBeforeUpdate. ' +
255
+ 'This might either be because of a bug in React, or because ' +
256
+ 'a component reassigns its own `this.props`. ' +
257
'Please file an issue.',
258
+ getComponentName(finishedWork.type) || 'instance',
259
);
260
}
261
}
@@ -370,18 +380,27 @@ function commitLifeCycles(
380
// but instead we rely on them being set during last render.
381
// TODO: revisit this when we implement resuming.
382
if (__DEV__) {
373
- if (finishedWork.type === finishedWork.elementType) {
383
+ if (
384
+ finishedWork.type === finishedWork.elementType &&
385
+ !didWarnAboutReassigningProps
386
+ ) {
387
warning(
388
instance.props === finishedWork.memoizedProps,
376
- 'Expected instance props to match memoized props before ' +
377
- 'componentDidMount. This is likely due to a bug in React. ' +
389
+ 'Expected %s props to match memoized props before ' +
390
+ 'componentDidMount. ' +
391
+ 'This might either be because of a bug in React, or because ' +
392
+ 'a component reassigns its own `this.props`. ' +
393
'Please file an issue.',
394
+ getComponentName(finishedWork.type) || 'instance',
395
);
396
warning(
397
instance.state === finishedWork.memoizedState,
382
- 'Expected instance state to match memoized state before ' +
383
- 'componentDidMount. This is likely due to a bug in React. ' +
398
+ 'Expected %s state to match memoized state before ' +
399
+ 'componentDidMount. ' +
400
+ 'This might either be because of a bug in React, or because ' +
401
+ 'a component reassigns its own `this.props`. ' +
402
'Please file an issue.',
403
+ getComponentName(finishedWork.type) || 'instance',
404
);
405
}
406
}
@@ -398,18 +417,27 @@ function commitLifeCycles(
417
// but instead we rely on them being set during last render.
418
// TODO: revisit this when we implement resuming.
419
if (__DEV__) {
401
- if (finishedWork.type === finishedWork.elementType) {
420
+ if (
421
+ finishedWork.type === finishedWork.elementType &&
422
+ !didWarnAboutReassigningProps
423
+ ) {
424
warning(
425
instance.props === finishedWork.memoizedProps,
404
- 'Expected instance props to match memoized props before ' +
405
- 'componentDidUpdate. This is likely due to a bug in React. ' +
426
+ 'Expected %s props to match memoized props before ' +
427
+ 'componentDidUpdate. ' +
428
+ 'This might either be because of a bug in React, or because ' +
429
+ 'a component reassigns its own `this.props`. ' +
430
'Please file an issue.',
431
+ getComponentName(finishedWork.type) || 'instance',
432
);
433
warning(
434
instance.state === finishedWork.memoizedState,
410
- 'Expected instance state to match memoized state before ' +
411
- 'componentDidUpdate. This is likely due to a bug in React. ' +
435
+ 'Expected %s state to match memoized state before ' +
436
+ 'componentDidUpdate. ' +
437
+ 'This might either be because of a bug in React, or because ' +
438
+ 'a component reassigns its own `this.props`. ' +
439
'Please file an issue.',
440
+ getComponentName(finishedWork.type) || 'instance',
441
);
442
}
443
}
@@ -424,18 +452,27 @@ function commitLifeCycles(
452
const updateQueue = finishedWork.updateQueue;
453
if (updateQueue !== null) {
454
if (__DEV__) {
427
- if (finishedWork.type === finishedWork.elementType) {
455
+ if (
456
+ finishedWork.type === finishedWork.elementType &&
457
+ !didWarnAboutReassigningProps
458
+ ) {
459
warning(
460
instance.props === finishedWork.memoizedProps,
430
- 'Expected instance props to match memoized props before ' +
431
- 'processing the update queue. This is likely due to a bug in React. ' +
461
+ 'Expected %s props to match memoized props before ' +
462
+ 'processing the update queue. ' +
463
+ 'This might either be because of a bug in React, or because ' +
464
+ 'a component reassigns its own `this.props`. ' +
465
'Please file an issue.',
466
+ getComponentName(finishedWork.type) || 'instance',
467
);
468
warning(
469
instance.state === finishedWork.memoizedState,
436
- 'Expected instance state to match memoized state before ' +
437
- 'processing the update queue. This is likely due to a bug in React. ' +
470
+ 'Expected %s state to match memoized state before ' +
471
+ 'processing the update queue. ' +
472
+ 'This might either be because of a bug in React, or because ' +
473
+ 'a component reassigns its own `this.props`. ' +
474
'Please file an issue.',
475
+ getComponentName(finishedWork.type) || 'instance',
476
);
477
}
478
}