Skip to content

Commit 2b1a0e3

Browse files
committed
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.
1 parent 824e991 commit 2b1a0e3

File tree

3 files changed

+155
-39
lines changed

3 files changed

+155
-39
lines changed

scripts/fiber/tests-passing.txt

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1675,6 +1675,7 @@ src/renderers/shared/fiber/__tests__/ReactIncremental-test.js
16751675
* can resume work in a subtree even when a parent bails out
16761676
* can resume work in a bailed subtree within one pass
16771677
* can resume mounting a class component
1678+
* reuses the same instance when resuming a class instance
16781679
* can reuse work done after being preempted
16791680
* can reuse work that began but did not complete, after being preempted
16801681
* can reuse work if shouldComponentUpdate is false, after being preempted

src/renderers/shared/fiber/ReactFiberClassComponent.js

Lines changed: 75 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -365,6 +365,36 @@ module.exports = function(
365365
}
366366
}
367367

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+
368398
// Called on a preexisting class instance. Returns false if a resumed render
369399
// could be reused.
370400
function resumeMountClassInstance(
@@ -389,6 +419,17 @@ module.exports = function(
389419
const newUnmaskedContext = getUnmaskedContext(workInProgress);
390420
const newContext = getMaskedContext(workInProgress, newUnmaskedContext);
391421

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+
392433
// TODO: Should we deal with a setState that happened after the last
393434
// componentWillMount and before this componentWillMount? Probably
394435
// unsupported anyway.
@@ -411,31 +452,42 @@ module.exports = function(
411452
return false;
412453
}
413454

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+
}
420467

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') {
422475
if (__DEV__) {
423476
startPhaseTimer(workInProgress, 'componentWillMount');
424477
}
425-
newInstance.componentWillMount();
478+
instance.componentWillMount();
426479
if (__DEV__) {
427480
stopPhaseTimer();
428481
}
429482
}
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;
434486
if (newUpdateQueue !== null) {
435-
newInstance.state = beginUpdateQueue(
487+
newState = beginUpdateQueue(
436488
workInProgress,
437489
newUpdateQueue,
438-
newInstance,
490+
instance,
439491
newState,
440492
newProps,
441493
priorityLevel,
@@ -444,6 +496,9 @@ module.exports = function(
444496
if (typeof instance.componentDidMount === 'function') {
445497
workInProgress.effectTag |= Update;
446498
}
499+
500+
instance.state = newState;
501+
447502
return true;
448503
}
449504

@@ -477,28 +532,12 @@ module.exports = function(
477532
// during componentDidUpdate we pass the "current" props.
478533

479534
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+
);
502541
}
503542

504543
// Compute the next state using the memoized state and the update queue.

src/renderers/shared/fiber/__tests__/ReactIncremental-test.js

Lines changed: 79 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -490,7 +490,76 @@ describe('ReactIncremental', () => {
490490

491491
ops = [];
492492
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+
]);
494563
});
495564

496565
it('can reuse work done after being preempted', () => {
@@ -1030,7 +1099,7 @@ describe('ReactIncremental', () => {
10301099
expect(ops).toEqual(['Foo', 'Bar:B2', 'Bar:D']);
10311100

10321101
// We expect each rerender to correspond to a new instance.
1033-
expect(instances.size).toBe(5);
1102+
expect(instances.size).toBe(4);
10341103
});
10351104

10361105
it('gets new props when setting state on a partly updated component', () => {
@@ -1094,6 +1163,12 @@ describe('ReactIncremental', () => {
10941163

10951164
class LifeCycle extends React.Component {
10961165
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+
}
10971172
componentWillMount() {
10981173
ops.push('componentWillMount:' + this.state.x + '-' + this.props.x);
10991174
}
@@ -1132,6 +1207,7 @@ describe('ReactIncremental', () => {
11321207

11331208
expect(ops).toEqual([
11341209
'App',
1210+
'componentWillReceiveProps:0-1',
11351211
'componentWillMount:1-1',
11361212
'Trail',
11371213
'componentDidMount:1-1',
@@ -1179,7 +1255,7 @@ describe('ReactIncremental', () => {
11791255

11801256
expect(ops).toEqual([
11811257
'App',
1182-
'componentWillMount:1(ctor)',
1258+
'componentWillMount:0(willMount)',
11831259
'render:1(willMount)',
11841260
'componentDidMount:1(willMount)',
11851261
]);

0 commit comments

Comments
 (0)