From d98a3d92697eeb9d1d5dc3ffd7e4abf7fddf45c9 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 21 Feb 2017 14:29:30 -0800 Subject: [PATCH 1/5] Support assigning directly to this.state inside cWRP We don't explicitly support this but it happens to work in Stack. We'll give it the same semantics as replaceState. --- .../ReactCompositeComponentState-test.js | 26 +++++++++++++++++++ .../shared/fiber/ReactFiberClassComponent.js | 4 +++ 2 files changed, 30 insertions(+) diff --git a/src/renderers/__tests__/ReactCompositeComponentState-test.js b/src/renderers/__tests__/ReactCompositeComponentState-test.js index 90189534384f..2d9351da1ef7 100644 --- a/src/renderers/__tests__/ReactCompositeComponentState-test.js +++ b/src/renderers/__tests__/ReactCompositeComponentState-test.js @@ -414,4 +414,30 @@ describe('ReactCompositeComponent-state', () => { 'scu from a,b to a,b,c', ]); }); + + it('should treat assigning to this.state inside cWRP as a replaceState, with a warning', () => { + let ops = []; + class Test extends React.Component { + state = { step: 1, extra: true }; + componentWillReceiveProps() { + this.setState({ step: 2 }, () => { + // Tests that earlier setState callbacks are not dropped + ops.push(`step: ${this.state.step}, extra: ${!!this.state.extra}`); + }); + // Treat like replaceState + this.state = { step: 3 }; + } + render() { + return null; + } + } + + // Mount + const container = document.createElement('div'); + ReactDOM.render(, container); + // Update + ReactDOM.render(, container); + + expect(ops).toEqual(['step: 3, extra: false']); + }); }); diff --git a/src/renderers/shared/fiber/ReactFiberClassComponent.js b/src/renderers/shared/fiber/ReactFiberClassComponent.js index 87f3426099e4..13b6d7801033 100644 --- a/src/renderers/shared/fiber/ReactFiberClassComponent.js +++ b/src/renderers/shared/fiber/ReactFiberClassComponent.js @@ -408,6 +408,10 @@ module.exports = function( if (oldProps !== newProps || oldContext !== newContext) { if (typeof instance.componentWillReceiveProps === 'function') { instance.componentWillReceiveProps(newProps, newContext); + + if (instance.state !== workInProgress.memoizedState) { + updater.enqueueReplaceState(instance, instance.state, null); + } } } From adc1641fecbf95a2989265b345a43ab374ffcec1 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 21 Feb 2017 14:40:56 -0800 Subject: [PATCH 2/5] Should be able to setState inside cWRP before assigning to this.state ...without dropping the update. This won't work the other way around, if you assign to this.state before calling setState. we'll add a deprecation warning so people stop relying on this pattern. --- .../shared/stack/reconciler/ReactCompositeComponent.js | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js b/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js index f534cfd5c71f..f1cc7086f459 100644 --- a/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js +++ b/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js @@ -850,6 +850,7 @@ var ReactCompositeComponent = { // _pendingStateQueue which will ensure that any state updates gets // immediately reconciled instead of waiting for the next batch. if (willReceive && inst.componentWillReceiveProps) { + const beforeState = inst.state; if (__DEV__) { measureLifeCyclePerf( () => inst.componentWillReceiveProps(nextProps, nextContext), @@ -859,6 +860,11 @@ var ReactCompositeComponent = { } else { inst.componentWillReceiveProps(nextProps, nextContext); } + const afterState = inst.state; + if (beforeState !== afterState) { + inst.state = beforeState; + inst.updater.enqueueReplaceState(inst, afterState); + } } // If updating happens to enqueue any new updates, we shouldn't execute new From d3cbbe3b363e662fb46d97d6f5f87b4010395ba2 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 21 Feb 2017 14:42:12 -0800 Subject: [PATCH 3/5] Add deprecation warning for assigning to this.state inside cWRP We'll remove support for this in a future release, though we'll likely still warn. --- .../__tests__/ReactCompositeComponentState-test.js | 8 ++++++++ src/renderers/shared/fiber/ReactFiberClassComponent.js | 9 +++++++++ .../shared/stack/reconciler/ReactCompositeComponent.js | 9 +++++++++ 3 files changed, 26 insertions(+) diff --git a/src/renderers/__tests__/ReactCompositeComponentState-test.js b/src/renderers/__tests__/ReactCompositeComponentState-test.js index 2d9351da1ef7..fba889a0b178 100644 --- a/src/renderers/__tests__/ReactCompositeComponentState-test.js +++ b/src/renderers/__tests__/ReactCompositeComponentState-test.js @@ -416,6 +416,8 @@ describe('ReactCompositeComponent-state', () => { }); it('should treat assigning to this.state inside cWRP as a replaceState, with a warning', () => { + spyOn(console, 'error'); + let ops = []; class Test extends React.Component { state = { step: 1, extra: true }; @@ -439,5 +441,11 @@ describe('ReactCompositeComponent-state', () => { ReactDOM.render(, container); expect(ops).toEqual(['step: 3, extra: false']); + expect(console.error.calls.count()).toEqual(1); + expect(console.error.calls.argsFor(0)[0]).toEqual( + 'Warning: Test.componentWillReceiveProps(): 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 13b6d7801033..eae8a56a21e1 100644 --- a/src/renderers/shared/fiber/ReactFiberClassComponent.js +++ b/src/renderers/shared/fiber/ReactFiberClassComponent.js @@ -410,6 +410,15 @@ module.exports = function( instance.componentWillReceiveProps(newProps, newContext); 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); } } diff --git a/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js b/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js index f1cc7086f459..9a572a8f6f0b 100644 --- a/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js +++ b/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js @@ -864,6 +864,15 @@ var ReactCompositeComponent = { if (beforeState !== afterState) { inst.state = beforeState; inst.updater.enqueueReplaceState(inst, afterState); + if (__DEV__) { + warning( + false, + '%s.componentWillReceiveProps(): Assigning directly to ' + + 'this.state is deprecated (except inside a component\'s ' + + 'constructor). Use setState instead.', + this.getName() || 'ReactCompositeComponent' + ); + } } } From 28dbbe57349cd79eefc3f59cc3575fdd7b4c6688 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 21 Feb 2017 14:43:01 -0800 Subject: [PATCH 4/5] Run test script --- scripts/fiber/tests-passing.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index 072fe0598897..faa85d2c130b 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -607,6 +607,7 @@ src/renderers/__tests__/ReactCompositeComponentState-test.js * should batch unmounts * 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 src/renderers/__tests__/ReactEmptyComponent-test.js * should not produce child DOM nodes for null and false From d649ebc6e130c1cb5180b61de62ca79b4c01adf9 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 21 Feb 2017 16:47:25 -0800 Subject: [PATCH 5/5] Test should ensure that update is applied before corresponding render Tightens up the test a bit to prevent regressions. --- .../__tests__/ReactCompositeComponentState-test.js | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/renderers/__tests__/ReactCompositeComponentState-test.js b/src/renderers/__tests__/ReactCompositeComponentState-test.js index fba889a0b178..46ec0f1fe452 100644 --- a/src/renderers/__tests__/ReactCompositeComponentState-test.js +++ b/src/renderers/__tests__/ReactCompositeComponentState-test.js @@ -424,12 +424,13 @@ describe('ReactCompositeComponent-state', () => { componentWillReceiveProps() { this.setState({ step: 2 }, () => { // Tests that earlier setState callbacks are not dropped - ops.push(`step: ${this.state.step}, extra: ${!!this.state.extra}`); + 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; } } @@ -440,7 +441,11 @@ describe('ReactCompositeComponent-state', () => { // Update ReactDOM.render(, container); - expect(ops).toEqual(['step: 3, extra: false']); + expect(ops).toEqual([ + 'render -- step: 1, extra: true', + '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.componentWillReceiveProps(): Assigning directly to ' +