diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index 01cba63cf12..36d020d40ab 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -506,6 +506,7 @@ src/renderers/__tests__/ReactCompositeComponentState-test.js * should update state when called from child cWRP * should merge state when sCU returns false * should treat assigning to this.state inside cWRP as a replaceState, with a warning +* should treat assigning to this.state inside cWM as a replaceState, with a warning src/renderers/__tests__/ReactEmptyComponent-test.js * should not produce child DOM nodes for null and false @@ -1675,6 +1676,7 @@ src/renderers/shared/fiber/__tests__/ReactIncremental-test.js * can resume work in a subtree even when a parent bails out * can resume work in a bailed subtree within one pass * can resume mounting a class component +* reuses the same instance when resuming a class instance * can reuse work done after being preempted * can reuse work that began but did not complete, after being preempted * can reuse work if shouldComponentUpdate is false, after being preempted diff --git a/src/renderers/__tests__/ReactCompositeComponentState-test.js b/src/renderers/__tests__/ReactCompositeComponentState-test.js index 118220bf5cc..21b29c04357 100644 --- a/src/renderers/__tests__/ReactCompositeComponentState-test.js +++ b/src/renderers/__tests__/ReactCompositeComponentState-test.js @@ -447,4 +447,44 @@ describe('ReactCompositeComponent-state', () => { 'Use setState instead.', ); }); + + it('should treat assigning to this.state inside cWM as a replaceState, with a warning', () => { + spyOn(console, 'error'); + + let ops = []; + class Test extends React.Component { + state = {step: 1, extra: true}; + componentWillMount() { + this.setState({step: 2}, () => { + // Tests that earlier setState callbacks are not dropped + ops.push( + `callback -- step: ${this.state.step}, extra: ${!!this.state.extra}`, + ); + }); + // Treat like replaceState + this.state = {step: 3}; + } + render() { + ops.push( + `render -- step: ${this.state.step}, extra: ${!!this.state.extra}`, + ); + return null; + } + } + + // Mount + const container = document.createElement('div'); + ReactDOM.render(, container); + + expect(ops).toEqual([ + 'render -- step: 3, extra: false', + 'callback -- step: 3, extra: false', + ]); + expect(console.error.calls.count()).toEqual(1); + expect(console.error.calls.argsFor(0)[0]).toEqual( + 'Warning: Test.componentWillMount(): Assigning directly to ' + + "this.state is deprecated (except inside a component's constructor). " + + 'Use setState instead.', + ); + }); }); diff --git a/src/renderers/shared/fiber/ReactFiberClassComponent.js b/src/renderers/shared/fiber/ReactFiberClassComponent.js index ba6425308e9..a1320b98666 100644 --- a/src/renderers/shared/fiber/ReactFiberClassComponent.js +++ b/src/renderers/shared/fiber/ReactFiberClassComponent.js @@ -304,6 +304,59 @@ module.exports = function( return instance; } + function callComponentWillMount(workInProgress, instance) { + if (__DEV__) { + startPhaseTimer(workInProgress, 'componentWillMount'); + } + const oldState = instance.state; + instance.componentWillMount(); + if (__DEV__) { + stopPhaseTimer(); + } + + if (oldState !== instance.state) { + if (__DEV__) { + warning( + false, + '%s.componentWillMount(): Assigning directly to this.state is ' + + "deprecated (except inside a component's " + + 'constructor). Use setState instead.', + getComponentName(workInProgress), + ); + } + updater.enqueueReplaceState(instance, instance.state, null); + } + } + + function callComponentWillReceiveProps( + workInProgress, + instance, + newProps, + newContext, + ) { + if (__DEV__) { + startPhaseTimer(workInProgress, 'componentWillReceiveProps'); + } + const oldState = instance.state; + instance.componentWillReceiveProps(newProps, newContext); + if (__DEV__) { + stopPhaseTimer(); + } + + if (instance.state !== oldState) { + if (__DEV__) { + warning( + false, + '%s.componentWillReceiveProps(): Assigning directly to ' + + "this.state is deprecated (except inside a component's " + + 'constructor). Use setState instead.', + getComponentName(workInProgress), + ); + } + updater.enqueueReplaceState(instance, instance.state, null); + } + } + // Invokes the mount life-cycles on a previously never rendered instance. function mountClassInstance( workInProgress: Fiber, @@ -339,13 +392,7 @@ module.exports = function( } if (typeof instance.componentWillMount === 'function') { - if (__DEV__) { - startPhaseTimer(workInProgress, 'componentWillMount'); - } - instance.componentWillMount(); - if (__DEV__) { - stopPhaseTimer(); - } + callComponentWillMount(workInProgress, instance); // If we had additional state updates during this life-cycle, let's // process them now. const updateQueue = workInProgress.updateQueue; @@ -389,6 +436,34 @@ module.exports = function( const newUnmaskedContext = getUnmaskedContext(workInProgress); const newContext = getMaskedContext(workInProgress, newUnmaskedContext); + const oldContext = instance.context; + const oldProps = workInProgress.memoizedProps; + + if ( + typeof instance.componentWillReceiveProps === 'function' && + (oldProps !== newProps || oldContext !== newContext) + ) { + callComponentWillReceiveProps( + workInProgress, + instance, + newProps, + newContext, + ); + } + + // Process the update queue before calling shouldComponentUpdate + const updateQueue = workInProgress.updateQueue; + if (updateQueue !== null) { + newState = beginUpdateQueue( + workInProgress, + updateQueue, + instance, + newState, + newProps, + priorityLevel, + ); + } + // TODO: Should we deal with a setState that happened after the last // componentWillMount and before this componentWillMount? Probably // unsupported anyway. @@ -411,39 +486,34 @@ module.exports = function( return false; } - // If we didn't bail out we need to construct a new instance. We don't - // want to reuse one that failed to fully mount. - const newInstance = constructClassInstance(workInProgress, newProps); - newInstance.props = newProps; - newInstance.state = newState = newInstance.state || null; - newInstance.context = newContext; + // Update the input pointers now so that they are correct when we call + // componentWillMount + instance.props = newProps; + instance.state = newState; + instance.context = newContext; - if (typeof newInstance.componentWillMount === 'function') { - if (__DEV__) { - startPhaseTimer(workInProgress, 'componentWillMount'); - } - newInstance.componentWillMount(); - if (__DEV__) { - stopPhaseTimer(); + if (typeof instance.componentWillMount === 'function') { + callComponentWillMount(workInProgress, instance); + // componentWillMount may have called setState. Process the update queue. + const newUpdateQueue = workInProgress.updateQueue; + if (newUpdateQueue !== null) { + newState = beginUpdateQueue( + workInProgress, + updateQueue, + instance, + newState, + newProps, + priorityLevel, + ); } } - // If we had additional state updates, process them now. - // They may be from componentWillMount() or from error boundary's setState() - // during initial mounting. - const newUpdateQueue = workInProgress.updateQueue; - if (newUpdateQueue !== null) { - newInstance.state = beginUpdateQueue( - workInProgress, - newUpdateQueue, - newInstance, - newState, - newProps, - priorityLevel, - ); - } + if (typeof instance.componentDidMount === 'function') { workInProgress.effectTag |= Update; } + + instance.state = newState; + return true; } @@ -476,29 +546,16 @@ module.exports = function( // ever the previously attempted to render - not the "current". However, // during componentDidUpdate we pass the "current" props. - if (oldProps !== newProps || oldContext !== newContext) { - if (typeof instance.componentWillReceiveProps === 'function') { - if (__DEV__) { - startPhaseTimer(workInProgress, 'componentWillReceiveProps'); - } - instance.componentWillReceiveProps(newProps, newContext); - if (__DEV__) { - stopPhaseTimer(); - } - - if (instance.state !== workInProgress.memoizedState) { - if (__DEV__) { - warning( - false, - '%s.componentWillReceiveProps(): Assigning directly to ' + - "this.state is deprecated (except inside a component's " + - 'constructor). Use setState instead.', - getComponentName(workInProgress), - ); - } - updater.enqueueReplaceState(instance, instance.state, null); - } - } + if ( + typeof instance.componentWillReceiveProps === 'function' && + (oldProps !== newProps || oldContext !== newContext) + ) { + callComponentWillReceiveProps( + workInProgress, + instance, + newProps, + newContext, + ); } // Compute the next state using the memoized state and the update queue. diff --git a/src/renderers/shared/fiber/__tests__/ReactIncremental-test.js b/src/renderers/shared/fiber/__tests__/ReactIncremental-test.js index d4eeb883779..acfea6ac166 100644 --- a/src/renderers/shared/fiber/__tests__/ReactIncremental-test.js +++ b/src/renderers/shared/fiber/__tests__/ReactIncremental-test.js @@ -490,7 +490,76 @@ describe('ReactIncremental', () => { ops = []; ReactNoop.flush(); - expect(ops).toEqual(['Foo constructor: foo', 'Foo', 'Bar']); + expect(ops).toEqual(['Foo', 'Bar']); + }); + + it('reuses the same instance when resuming a class instance', () => { + let ops = []; + let foo; + class Parent extends React.Component { + shouldComponentUpdate() { + return false; + } + render() { + return ; + } + } + + let constructorCount = 0; + class Foo extends React.Component { + constructor(props) { + super(props); + // Test based on a www bug where props was null on resume + ops.push('constructor: ' + props.prop); + constructorCount++; + } + componentWillMount() { + ops.push('componentWillMount: ' + this.props.prop); + } + componentWillReceiveProps() { + ops.push('componentWillReceiveProps: ' + this.props.prop); + } + componentDidMount() { + ops.push('componentDidMount: ' + this.props.prop); + } + componentWillUpdate() { + ops.push('componentWillUpdate: ' + this.props.prop); + } + componentDidUpdate() { + ops.push('componentDidUpdate: ' + this.props.prop); + } + render() { + foo = this; + ops.push('render: ' + this.props.prop); + return ; + } + } + + function Bar() { + ops.push('Foo did complete'); + return
; + } + + ReactNoop.render(); + ReactNoop.flushDeferredPri(25); + expect(ops).toEqual([ + 'constructor: foo', + 'componentWillMount: foo', + 'render: foo', + 'Foo did complete', + ]); + + foo.setState({value: 'bar'}); + + ops = []; + ReactNoop.flush(); + expect(constructorCount).toEqual(1); + expect(ops).toEqual([ + 'componentWillMount: foo', + 'render: foo', + 'Foo did complete', + 'componentDidMount: foo', + ]); }); it('can reuse work done after being preempted', () => { @@ -1030,7 +1099,7 @@ describe('ReactIncremental', () => { expect(ops).toEqual(['Foo', 'Bar:B2', 'Bar:D']); // We expect each rerender to correspond to a new instance. - expect(instances.size).toBe(5); + expect(instances.size).toBe(4); }); it('gets new props when setting state on a partly updated component', () => { @@ -1094,6 +1163,12 @@ describe('ReactIncremental', () => { class LifeCycle extends React.Component { state = {x: this.props.x}; + componentWillReceiveProps(nextProps) { + ops.push( + 'componentWillReceiveProps:' + this.state.x + '-' + nextProps.x, + ); + this.setState({x: nextProps.x}); + } componentWillMount() { ops.push('componentWillMount:' + this.state.x + '-' + this.props.x); } @@ -1132,6 +1207,7 @@ describe('ReactIncremental', () => { expect(ops).toEqual([ 'App', + 'componentWillReceiveProps:0-1', 'componentWillMount:1-1', 'Trail', 'componentDidMount:1-1', @@ -1179,7 +1255,7 @@ describe('ReactIncremental', () => { expect(ops).toEqual([ 'App', - 'componentWillMount:1(ctor)', + 'componentWillMount:0(willMount)', 'render:1(willMount)', 'componentDidMount:1(willMount)', ]);