From c840fa4f0d782257867fe1f1296b819cbc9b5c5d Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Tue, 3 Jan 2017 08:45:46 -0800 Subject: [PATCH 1/3] Pass prevContext param to componentDidUpdate This makes use of an expando property (__reactInternalPrevContext) on the stateNode (instance). A property on the instance was used instead of a Fiber attribute because: 1) Conditional fields on fibers would break monomorphism. 2) Adding to all fibers would bloat types that don't use context. --- scripts/fiber/tests-failing.txt | 2 +- scripts/fiber/tests-passing.txt | 1 + src/renderers/shared/fiber/ReactFiberCommitWork.js | 9 ++++++++- 3 files changed, 10 insertions(+), 2 deletions(-) diff --git a/scripts/fiber/tests-failing.txt b/scripts/fiber/tests-failing.txt index 70fbfb03293b2..29c0e41ab795b 100644 --- a/scripts/fiber/tests-failing.txt +++ b/scripts/fiber/tests-failing.txt @@ -38,4 +38,4 @@ src/renderers/dom/shared/__tests__/ReactRenderDocument-test.js * should be able to mount into document * should give helpful errors on state desync * should throw on full document render w/ no markup -* supports findDOMNode on full-page components +* supports findDOMNode on full-page components \ No newline at end of file diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index c5ff7cee6acb5..b17befaafe40d 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -159,6 +159,7 @@ src/isomorphic/children/__tests__/sliceChildren-test.js src/isomorphic/classic/__tests__/ReactContextValidator-test.js * should filter out context not in contextTypes * should pass next context to lifecycles +* should pass previous context to lifecycles * should check context types * should check child context types * should warn (but not error) if getChildContext method is missing diff --git a/src/renderers/shared/fiber/ReactFiberCommitWork.js b/src/renderers/shared/fiber/ReactFiberCommitWork.js index b9bc6d6fb47e5..8340beea65c0c 100644 --- a/src/renderers/shared/fiber/ReactFiberCommitWork.js +++ b/src/renderers/shared/fiber/ReactFiberCommitWork.js @@ -442,13 +442,20 @@ module.exports = function( if (typeof instance.componentDidUpdate === 'function') { const prevProps = current.memoizedProps; const prevState = current.memoizedState; - instance.componentDidUpdate(prevProps, prevState); + const prevContext = instance.__reactInternalPrevContext; + instance.componentDidUpdate(prevProps, prevState, prevContext); } } } if ((finishedWork.effectTag & Callback) && finishedWork.updateQueue !== null) { commitCallbacks(finishedWork, finishedWork.updateQueue, instance); } + + // Store updated context for prevContext param in next call to componentDidUpdate(). + // Use an expando property on instance rather than Fiber because: + // 1) Conditional fields on fibers would break monomorphism. + // 2) Adding to all fibers would bloat types that don't use context. + instance.__reactInternalPrevContext = instance.context; return; } case HostRoot: { From cdb2aa07d6a3bb77aeedfad30330131ed521e883 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Tue, 28 Feb 2017 16:48:37 -0800 Subject: [PATCH 2/3] Deprecated prevContext parameter to componentDidUpdate This affects Stack and Fiber. I've updated impacted tests as well. This may not be the direction we choose to go either. More discussion is still needed about the future of context... --- scripts/fiber/tests-failing.txt | 5 +---- scripts/fiber/tests-passing.txt | 2 +- .../__tests__/ReactContextValidator-test.js | 18 +++++------------- .../__tests__/ReactCompositeComponent-test.js | 14 -------------- .../shared/fiber/ReactFiberCommitWork.js | 9 +-------- .../reconciler/ReactCompositeComponent.js | 6 ++---- 6 files changed, 10 insertions(+), 44 deletions(-) diff --git a/scripts/fiber/tests-failing.txt b/scripts/fiber/tests-failing.txt index 29c0e41ab795b..51105e16bff20 100644 --- a/scripts/fiber/tests-failing.txt +++ b/scripts/fiber/tests-failing.txt @@ -1,6 +1,3 @@ -src/isomorphic/classic/__tests__/ReactContextValidator-test.js -* should pass previous context to lifecycles - src/renderers/__tests__/ReactComponentTreeHook-test.js * can be retrieved by ID @@ -38,4 +35,4 @@ src/renderers/dom/shared/__tests__/ReactRenderDocument-test.js * should be able to mount into document * should give helpful errors on state desync * should throw on full document render w/ no markup -* supports findDOMNode on full-page components \ No newline at end of file +* supports findDOMNode on full-page components diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index b17befaafe40d..fcf1f1de335ac 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -159,7 +159,7 @@ src/isomorphic/children/__tests__/sliceChildren-test.js src/isomorphic/classic/__tests__/ReactContextValidator-test.js * should filter out context not in contextTypes * should pass next context to lifecycles -* should pass previous context to lifecycles +* should not pass previous context to lifecycles * should check context types * should check child context types * should warn (but not error) if getChildContext method is missing diff --git a/src/isomorphic/classic/__tests__/ReactContextValidator-test.js b/src/isomorphic/classic/__tests__/ReactContextValidator-test.js index 4eab5185cb998..1040bcc7029eb 100644 --- a/src/isomorphic/classic/__tests__/ReactContextValidator-test.js +++ b/src/isomorphic/classic/__tests__/ReactContextValidator-test.js @@ -125,22 +125,16 @@ describe('ReactContextValidator', () => { expect(actualComponentWillUpdate).toEqual({foo: 'def'}); }); - it('should pass previous context to lifecycles', () => { + it('should not pass previous context to lifecycles', () => { var actualComponentDidUpdate; var Parent = React.createClass({ childContextTypes: { foo: React.PropTypes.string.isRequired, - bar: React.PropTypes.string.isRequired, }, - getChildContext: function() { - return { - foo: this.props.foo, - bar: 'bar', - }; + return { foo: this.props.foo }; }, - render: function() { return ; }, @@ -150,11 +144,9 @@ describe('ReactContextValidator', () => { contextTypes: { foo: React.PropTypes.string, }, - - componentDidUpdate: function(prevProps, prevState, prevContext) { - actualComponentDidUpdate = prevContext; + componentDidUpdate: function(...args) { + actualComponentDidUpdate = args; }, - render: function() { return
; }, @@ -163,7 +155,7 @@ describe('ReactContextValidator', () => { var container = document.createElement('div'); ReactDOM.render(, container); ReactDOM.render(, container); - expect(actualComponentDidUpdate).toEqual({foo: 'abc'}); + expect(actualComponentDidUpdate).toHaveLength(2); }); it('should check context types', () => { diff --git a/src/renderers/__tests__/ReactCompositeComponent-test.js b/src/renderers/__tests__/ReactCompositeComponent-test.js index 6346ff739dc69..55a70fa53d35c 100644 --- a/src/renderers/__tests__/ReactCompositeComponent-test.js +++ b/src/renderers/__tests__/ReactCompositeComponent-test.js @@ -831,13 +831,6 @@ describe('ReactCompositeComponent', () => { expect('foo' in nextContext).toBe(true); } - componentDidUpdate(prevProps, prevState, prevContext) { - if (!ReactDOMFeatureFlags.useFiber) { - // Fiber does not pass the previous context. - expect('foo' in prevContext).toBe(true); - } - } - shouldComponentUpdate(nextProps, nextState, nextContext) { expect('foo' in nextContext).toBe(true); return true; @@ -853,13 +846,6 @@ describe('ReactCompositeComponent', () => { expect('foo' in nextContext).toBe(false); } - componentDidUpdate(prevProps, prevState, prevContext) { - if (!ReactDOMFeatureFlags.useFiber) { - // Fiber does not pass the previous context. - expect('foo' in prevContext).toBe(false); - } - } - shouldComponentUpdate(nextProps, nextState, nextContext) { expect('foo' in nextContext).toBe(false); return true; diff --git a/src/renderers/shared/fiber/ReactFiberCommitWork.js b/src/renderers/shared/fiber/ReactFiberCommitWork.js index 8340beea65c0c..b9bc6d6fb47e5 100644 --- a/src/renderers/shared/fiber/ReactFiberCommitWork.js +++ b/src/renderers/shared/fiber/ReactFiberCommitWork.js @@ -442,20 +442,13 @@ module.exports = function( if (typeof instance.componentDidUpdate === 'function') { const prevProps = current.memoizedProps; const prevState = current.memoizedState; - const prevContext = instance.__reactInternalPrevContext; - instance.componentDidUpdate(prevProps, prevState, prevContext); + instance.componentDidUpdate(prevProps, prevState); } } } if ((finishedWork.effectTag & Callback) && finishedWork.updateQueue !== null) { commitCallbacks(finishedWork, finishedWork.updateQueue, instance); } - - // Store updated context for prevContext param in next call to componentDidUpdate(). - // Use an expando property on instance rather than Fiber because: - // 1) Conditional fields on fibers would break monomorphism. - // 2) Adding to all fibers would bloat types that don't use context. - instance.__reactInternalPrevContext = instance.context; return; } case HostRoot: { diff --git a/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js b/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js index a1434d7b886f4..557b1311d6151 100644 --- a/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js +++ b/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js @@ -1005,11 +1005,9 @@ var ReactCompositeComponent = { var hasComponentDidUpdate = Boolean(inst.componentDidUpdate); var prevProps; var prevState; - var prevContext; if (hasComponentDidUpdate) { prevProps = inst.props; prevState = inst.state; - prevContext = inst.context; } if (inst.componentWillUpdate) { @@ -1040,14 +1038,14 @@ var ReactCompositeComponent = { if (__DEV__) { transaction.getReactMountReady().enqueue(() => { measureLifeCyclePerf( - inst.componentDidUpdate.bind(inst, prevProps, prevState, prevContext), + inst.componentDidUpdate.bind(inst, prevProps, prevState), this._debugID, 'componentDidUpdate' ); }); } else { transaction.getReactMountReady().enqueue( - inst.componentDidUpdate.bind(inst, prevProps, prevState, prevContext), + inst.componentDidUpdate.bind(inst, prevProps, prevState), inst ); } From d0fffd7e67e089ee4778be4a04dfbba408a74869 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Fri, 21 Apr 2017 11:22:36 -0700 Subject: [PATCH 3/3] Removed unused ReactDOMFeatureFlags variable --- src/renderers/__tests__/ReactCompositeComponent-test.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/renderers/__tests__/ReactCompositeComponent-test.js b/src/renderers/__tests__/ReactCompositeComponent-test.js index bfc945d9bd3d9..543ca7bda991d 100644 --- a/src/renderers/__tests__/ReactCompositeComponent-test.js +++ b/src/renderers/__tests__/ReactCompositeComponent-test.js @@ -15,7 +15,6 @@ var ChildUpdates; var MorphingComponent; var React; var ReactDOM; -var ReactDOMFeatureFlags; var ReactDOMServer; var ReactCurrentOwner; var ReactPropTypes; @@ -28,7 +27,6 @@ describe('ReactCompositeComponent', () => { jest.resetModules(); React = require('react'); ReactDOM = require('react-dom'); - ReactDOMFeatureFlags = require('ReactDOMFeatureFlags'); ReactDOMServer = require('react-dom/server'); ReactCurrentOwner = require('ReactCurrentOwner'); ReactPropTypes = require('ReactPropTypes');