Don't recreate instance when resuming a class component's initial mount
Recreating the class instance causes refs (and other callbacks) to close over stale instances. Instead, re-use the previous instance. componentWillMount is called again. We also call componentWillReceiveProps, to ensure that state derived from props remains in sync.
Andrew Clark committed
May 4, 2017 at 15:38 UTC
12326665900d173bf616fde5bfdbc5f1b2ce66dc
3 files changed
+155
-39
scripts/fiber/tests-passing.txt
+1
@@ -1675,6 +1675,7 @@ src/renderers/shared/fiber/__tests__/ReactIncremental-test.js
1675
* can resume work in a subtree even when a parent bails out
1676
* can resume work in a bailed subtree within one pass
1677
* can resume mounting a class component
1678
+* reuses the same instance when resuming a class instance
1679
* can reuse work done after being preempted
1680
* can reuse work that began but did not complete, after being preempted
1681
* can reuse work if shouldComponentUpdate is false, after being preempted
src/renderers/shared/fiber/ReactFiberClassComponent.js
+75
-36
@@ -365,6 +365,36 @@ module.exports = function(
365
}
366
}
367
368
+ function callComponentWillReceiveProps(
369
+ workInProgress,
370
+ instance,
371
+ newProps,
372
+ newContext,
373
+ ) {
374
+ if (typeof instance.componentWillReceiveProps === 'function') {
375
+ if (__DEV__) {
376
+ startPhaseTimer(workInProgress, 'componentWillReceiveProps');
377
+ }
378
+ instance.componentWillReceiveProps(newProps, newContext);
379
+ if (__DEV__) {
380
+ stopPhaseTimer();
381
+ }
382
+
383
+ if (instance.state !== workInProgress.memoizedState) {
384
+ if (__DEV__) {
385
+ warning(
386
+ false,
387
+ '%s.componentWillReceiveProps(): Assigning directly to ' +
388
+ "this.state is deprecated (except inside a component's " +
389
+ 'constructor). Use setState instead.',
390
+ getComponentName(workInProgress),
391
+ );
392
+ }
393
+ updater.enqueueReplaceState(instance, instance.state, null);
394
+ }
395
+ }
396
+ }
397
+
398
// Called on a preexisting class instance. Returns false if a resumed render
399
// could be reused.
400
function resumeMountClassInstance(
@@ -389,6 +419,17 @@ module.exports = function(
419
const newUnmaskedContext = getUnmaskedContext(workInProgress);
420
const newContext = getMaskedContext(workInProgress, newUnmaskedContext);
421
422
+ const oldContext = instance.context;
423
+ const oldProps = workInProgress.memoizedProps;
424
+ if (oldProps !== newProps || oldContext !== newContext) {
425
+ callComponentWillReceiveProps(
426
+ workInProgress,
427
+ instance,
428
+ newProps,
429
+ newContext,
430
+ );
431
+ }
432
+
433
// TODO: Should we deal with a setState that happened after the last
434
// componentWillMount and before this componentWillMount? Probably
435
// unsupported anyway.
@@ -411,31 +452,42 @@ module.exports = function(
452
return false;
453
}
454
414
- // If we didn't bail out we need to construct a new instance. We don't
415
- // want to reuse one that failed to fully mount.
416
- const newInstance = constructClassInstance(workInProgress, newProps);
417
- newInstance.props = newProps;
418
- newInstance.state = newState = newInstance.state || null;
419
- newInstance.context = newContext;
455
+ // componentWillMount may have called setState. Process the update queue.
456
+ let newUpdateQueue = workInProgress.updateQueue;
457
+ if (newUpdateQueue !== null) {
458
+ newState = beginUpdateQueue(
459
+ workInProgress,
460
+ newUpdateQueue,
461
+ instance,
462
+ newState,
463
+ newProps,
464
+ priorityLevel,
465
+ );
466
+ }
467
421
- if (typeof newInstance.componentWillMount === 'function') {
468
+ // Update the input pointers now so that they are correct when we call
469
+ // componentWillMount
470
+ instance.props = newProps;
471
+ instance.state = newState;
472
+ instance.context = newContext;
473
+
474
+ if (typeof instance.componentWillMount === 'function') {
475
if (__DEV__) {
476
startPhaseTimer(workInProgress, 'componentWillMount');
477
}
425
- newInstance.componentWillMount();
478
+ instance.componentWillMount();
479
if (__DEV__) {
480
stopPhaseTimer();
481
}
482
}
430
- // If we had additional state updates, process them now.
431
- // They may be from componentWillMount() or from error boundary's setState()
432
- // during initial mounting.
433
- const newUpdateQueue = workInProgress.updateQueue;
483
+
484
+ // componentWillMount may have called setState. Process the update queue.
485
+ newUpdateQueue = workInProgress.updateQueue;
486
if (newUpdateQueue !== null) {
435
- newInstance.state = beginUpdateQueue(
487
+ newState = beginUpdateQueue(
488
workInProgress,
489
newUpdateQueue,
438
- newInstance,
490
+ instance,
491
newState,
492
newProps,
493
priorityLevel,
@@ -444,6 +496,9 @@ module.exports = function(
496
if (typeof instance.componentDidMount === 'function') {
497
workInProgress.effectTag |= Update;
498
}
499
+
500
+ instance.state = newState;
501
+
502
return true;
503
}
504
@@ -477,28 +532,12 @@ module.exports = function(
532
// during componentDidUpdate we pass the "current" props.
533
534
if (oldProps !== newProps || oldContext !== newContext) {
480
- if (typeof instance.componentWillReceiveProps === 'function') {
481
- if (__DEV__) {
482
- startPhaseTimer(workInProgress, 'componentWillReceiveProps');
483
- }
484
- instance.componentWillReceiveProps(newProps, newContext);
485
- if (__DEV__) {
486
- stopPhaseTimer();
487
- }
488
-
489
- if (instance.state !== workInProgress.memoizedState) {
490
- if (__DEV__) {
491
- warning(
492
- false,
493
- '%s.componentWillReceiveProps(): Assigning directly to ' +
494
- "this.state is deprecated (except inside a component's " +
495
- 'constructor). Use setState instead.',
496
- getComponentName(workInProgress),
497
- );
498
- }
499
- updater.enqueueReplaceState(instance, instance.state, null);
500
- }
501
- }
535
+ callComponentWillReceiveProps(
536
+ workInProgress,
537
+ instance,
538
+ newProps,
539
+ newContext,
540
+ );
541
}
542
543
// Compute the next state using the memoized state and the update queue.
src/renderers/shared/fiber/__tests__/ReactIncremental-test.js
+79
-3
@@ -490,7 +490,76 @@ describe('ReactIncremental', () => {
490
491
ops = [];
492
ReactNoop.flush();
493
- expect(ops).toEqual(['Foo constructor: foo', 'Foo', 'Bar']);
493
+ expect(ops).toEqual(['Foo', 'Bar']);
494
+ });
495
+
496
+ it('reuses the same instance when resuming a class instance', () => {
497
+ let ops = [];
498
+ let foo;
499
+ class Parent extends React.Component {
500
+ shouldComponentUpdate() {
501
+ return false;
502
+ }
503
+ render() {
504
+ return <Foo prop={this.props.prop} />;
505
+ }
506
+ }
507
+
508
+ let constructorCount = 0;
509
+ class Foo extends React.Component {
510
+ constructor(props) {
511
+ super(props);
512
+ // Test based on a www bug where props was null on resume
513
+ ops.push('constructor: ' + props.prop);
514
+ constructorCount++;
515
+ }
516
+ componentWillMount() {
517
+ ops.push('componentWillMount: ' + this.props.prop);
518
+ }
519
+ componentWillReceiveProps() {
520
+ ops.push('componentWillReceiveProps: ' + this.props.prop);
521
+ }
522
+ componentDidMount() {
523
+ ops.push('componentDidMount: ' + this.props.prop);
524
+ }
525
+ componentWillUpdate() {
526
+ ops.push('componentWillUpdate: ' + this.props.prop);
527
+ }
528
+ componentDidUpdate() {
529
+ ops.push('componentDidUpdate: ' + this.props.prop);
530
+ }
531
+ render() {
532
+ foo = this;
533
+ ops.push('render: ' + this.props.prop);
534
+ return <Bar />;
535
+ }
536
+ }
537
+
538
+ function Bar() {
539
+ ops.push('Foo did complete');
540
+ return <div />;
541
+ }
542
+
543
+ ReactNoop.render(<Parent prop="foo" />);
544
+ ReactNoop.flushDeferredPri(25);
545
+ expect(ops).toEqual([
546
+ 'constructor: foo',
547
+ 'componentWillMount: foo',
548
+ 'render: foo',
549
+ 'Foo did complete',
550
+ ]);
551
+
552
+ foo.setState({value: 'bar'});
553
+
554
+ ops = [];
555
+ ReactNoop.flush();
556
+ expect(constructorCount).toEqual(1);
557
+ expect(ops).toEqual([
558
+ 'componentWillMount: foo',
559
+ 'render: foo',
560
+ 'Foo did complete',
561
+ 'componentDidMount: foo',
562
+ ]);
563
});
564
565
it('can reuse work done after being preempted', () => {
@@ -1030,7 +1099,7 @@ describe('ReactIncremental', () => {
1099
expect(ops).toEqual(['Foo', 'Bar:B2', 'Bar:D']);
1100
1101
// We expect each rerender to correspond to a new instance.
1033
- expect(instances.size).toBe(5);
1102
+ expect(instances.size).toBe(4);
1103
});
1104
1105
it('gets new props when setting state on a partly updated component', () => {
@@ -1094,6 +1163,12 @@ describe('ReactIncremental', () => {
1163
1164
class LifeCycle extends React.Component {
1165
state = {x: this.props.x};
1166
+ componentWillReceiveProps(nextProps) {
1167
+ ops.push(
1168
+ 'componentWillReceiveProps:' + this.state.x + '-' + nextProps.x,
1169
+ );
1170
+ this.setState({x: nextProps.x});
1171
+ }
1172
componentWillMount() {
1173
ops.push('componentWillMount:' + this.state.x + '-' + this.props.x);
1174
}
@@ -1132,6 +1207,7 @@ describe('ReactIncremental', () => {
1207
1208
expect(ops).toEqual([
1209
'App',
1210
+ 'componentWillReceiveProps:0-1',
1211
'componentWillMount:1-1',
1212
'Trail',
1213
'componentDidMount:1-1',
@@ -1179,7 +1255,7 @@ describe('ReactIncremental', () => {
1255
1256
expect(ops).toEqual([
1257
'App',
1182
- 'componentWillMount:1(ctor)',
1258
+ 'componentWillMount:0(willMount)',
1259
'render:1(willMount)',
1260
'componentDidMount:1(willMount)',
1261
]);