From a361a50ed23f718a67d85f24104af705a7dfc64b Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Mon, 31 Jul 2017 16:41:58 -0700 Subject: [PATCH 1/6] Refactored fiber context to fix a nested updates bug --- .../shared/fiber/ReactFiberBeginWork.js | 8 +- .../shared/fiber/ReactFiberContext.js | 33 ++- .../fiber/__tests__/ReactIncremental-test.js | 217 ++++++++++++++++++ 3 files changed, 245 insertions(+), 13 deletions(-) diff --git a/src/renderers/shared/fiber/ReactFiberBeginWork.js b/src/renderers/shared/fiber/ReactFiberBeginWork.js index fe42ad435aa9..87cd7acd2300 100644 --- a/src/renderers/shared/fiber/ReactFiberBeginWork.js +++ b/src/renderers/shared/fiber/ReactFiberBeginWork.js @@ -277,6 +277,11 @@ module.exports = function( markRef(current, workInProgress); if (!shouldUpdate) { + // Context providers should defer to sCU for rendering + if (hasContext) { + invalidateContextProvider(workInProgress, false); + } + return bailoutOnAlreadyFinishedWork(current, workInProgress); } @@ -302,8 +307,9 @@ module.exports = function( // The context might have changed so we need to recalculate it. if (hasContext) { - invalidateContextProvider(workInProgress); + invalidateContextProvider(workInProgress, true); } + return workInProgress.child; } diff --git a/src/renderers/shared/fiber/ReactFiberContext.js b/src/renderers/shared/fiber/ReactFiberContext.js index 7a5f8eca203e..004a1de02f32 100644 --- a/src/renderers/shared/fiber/ReactFiberContext.js +++ b/src/renderers/shared/fiber/ReactFiberContext.js @@ -234,14 +234,22 @@ exports.pushContextProvider = function(workInProgress: Fiber): boolean { emptyObject; // Remember the parent context so we can merge with it later. + // Inherit the parent's did-perform-work value to avoid inadvertantly blocking updates. previousContext = contextStackCursor.current; push(contextStackCursor, memoizedMergedChildContext, workInProgress); - push(didPerformWorkStackCursor, false, workInProgress); + push( + didPerformWorkStackCursor, + didPerformWorkStackCursor.current, + workInProgress, + ); return true; }; -exports.invalidateContextProvider = function(workInProgress: Fiber): void { +exports.invalidateContextProvider = function( + workInProgress: Fiber, + didChange: boolean, +): void { const instance = workInProgress.stateNode; invariant( instance, @@ -250,20 +258,21 @@ exports.invalidateContextProvider = function(workInProgress: Fiber): void { ); // Merge parent and own context. - const mergedContext = processChildContext( - workInProgress, - previousContext, - true, - ); - instance.__reactInternalMemoizedMergedChildContext = mergedContext; + // Skip this if we're not updating due to sCU. + let mergedContext; + if (didChange) { + mergedContext = processChildContext(workInProgress, previousContext, true); + instance.__reactInternalMemoizedMergedChildContext = mergedContext; + } // Replace the old (or empty) context with the new one. // It is important to unwind the context in the reverse order. pop(didPerformWorkStackCursor, workInProgress); - pop(contextStackCursor, workInProgress); - // Now push the new context and mark that it has changed. - push(contextStackCursor, mergedContext, workInProgress); - push(didPerformWorkStackCursor, true, workInProgress); + if (didChange) { + pop(contextStackCursor, workInProgress); + push(contextStackCursor, mergedContext, workInProgress); + } + push(didPerformWorkStackCursor, didChange, workInProgress); }; exports.resetContext = function(): void { diff --git a/src/renderers/shared/fiber/__tests__/ReactIncremental-test.js b/src/renderers/shared/fiber/__tests__/ReactIncremental-test.js index 63935f5ca481..ccd324073d0d 100644 --- a/src/renderers/shared/fiber/__tests__/ReactIncremental-test.js +++ b/src/renderers/shared/fiber/__tests__/ReactIncremental-test.js @@ -12,6 +12,8 @@ 'use strict'; var React; +var ReactDOM; +var ReactDOMFeatureFlags; var ReactNoop; var ReactFeatureFlags; var PropTypes; @@ -20,6 +22,8 @@ describe('ReactIncremental', () => { beforeEach(() => { jest.resetModules(); React = require('react'); + ReactDOM = require('react-dom'); + ReactDOMFeatureFlags = require('ReactDOMFeatureFlags'); ReactNoop = require('react-noop-renderer'); PropTypes = require('prop-types'); @@ -2395,4 +2399,217 @@ describe('ReactIncremental', () => { expect(cduNextProps).toEqual([{children: 'B'}]); }, ); + + // TODO bvaughn Convert new tests to use ReactNoop? + // TODO bvaughn Better annotate with comments? + // TODO bvaughn Remove any tests that overlap with above? + it('should update descendants with new context values', () => { + class TopContextProvider extends React.Component { + static childContextTypes = { + count: PropTypes.number, + }; + state = {count: 0}; + getChildContext = () => ({ + count: this.state.count, + }); + render = () => this.props.children; + updateCount = () => + this.setState(state => ({ + count: state.count + 1, + })); + } + + class Middle extends React.Component { + render = () => this.props.children; + } + + class Child extends React.Component { + static contextTypes = { + count: PropTypes.number, + }; + render = () =>
count:{this.context.count}
; + } + + const container = document.createElement('div'); + const grandparent = ReactDOM.render( + , + container, + ); + + expect(container.textContent).toBe('count:0'); + grandparent.updateCount(); + expect(container.textContent).toBe('count:1'); + }); + + it('should update descendants with multiple context-providing anscestors with new context values', () => { + class TopContextProvider extends React.Component { + static childContextTypes = { + count: PropTypes.number, + }; + state = {count: 0}; + getChildContext = () => ({ + count: this.state.count, + }); + render = () => this.props.children; + updateCount = () => + this.setState(state => ({ + count: state.count + 1, + })); + } + + class MiddleContextProvider extends React.Component { + static childContextTypes = { + name: PropTypes.string, + }; + getChildContext = () => ({ + name: 'brian', + }); + render = () => this.props.children; + } + + class Child extends React.Component { + static contextTypes = { + count: PropTypes.number, + }; + render = () =>
count:{this.context.count}
; + } + + const container = document.createElement('div'); + const grandparent = ReactDOM.render( + + + + + , + container, + ); + + expect(container.textContent).toBe('count:0'); + grandparent.updateCount(); + expect(container.textContent).toBe('count:1'); + }); + + it('should not update descendants with new context values if shouldComponentUpdate returns false', () => { + class TopContextProvider extends React.Component { + static childContextTypes = { + count: PropTypes.number, + }; + state = {count: 0}; + getChildContext = () => ({ + count: this.state.count, + }); + render = () => this.props.children; + updateCount = () => + this.setState(state => ({ + count: state.count + 1, + })); + } + + class MiddleScu extends React.Component { + shouldComponentUpdate() { + return false; + } + render = () => this.props.children; + } + + class MiddleContextProvider extends React.Component { + static childContextTypes = { + name: PropTypes.string, + }; + getChildContext = () => ({ + name: 'brian', + }); + render = () => this.props.children; + } + + class Child extends React.Component { + static contextTypes = { + count: PropTypes.number, + }; + render = () =>
count:{this.context.count}
; + } + + const container = document.createElement('div'); + const grandparent = ReactDOM.render( + + + + + , + container, + ); + + expect(container.textContent).toBe('count:0'); + grandparent.updateCount(); + expect(container.textContent).toBe('count:0'); + }); + + it('should update descendants with new context values if setState() is called in the middle of the tree', () => { + class TopContextProvider extends React.Component { + static childContextTypes = { + count: PropTypes.number, + }; + state = {count: 0}; + getChildContext = () => ({ + count: this.state.count, + }); + render = () => this.props.children; + updateCount = () => + this.setState(state => ({ + count: state.count + 1, + })); + } + + class MiddleScu extends React.Component { + shouldComponentUpdate() { + return false; + } + render = () => this.props.children; + } + + let middle, name = 'brian'; + class MiddleContextProvider extends React.Component { + static childContextTypes = { + name: PropTypes.string, + }; + getChildContext = () => ({ + name, + }); + componentDidMount = () => { + middle = this; + }; + render = () => this.props.children; + } + + class Child extends React.Component { + static contextTypes = { + count: PropTypes.number, + name: PropTypes.string, + }; + render = () => ( +
count:{this.context.count}, name:{this.context.name}
+ ); + } + + const container = document.createElement('div'); + const grandparent = ReactDOM.render( + + + + + , + container, + ); + + expect(container.textContent).toBe('count:0, name:brian'); + grandparent.updateCount(); + expect(container.textContent).toBe('count:0, name:brian'); + name = 'not brian'; + middle.forceUpdate(); + if (ReactDOMFeatureFlags.useFiber) { + expect(container.textContent).toBe('count:1, name:not brian'); + } else { + expect(container.textContent).toBe('count:0, name:not brian'); + } + }); }); From e113731de6c4c27a6e81e33c38025e2a931adaa0 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Mon, 31 Jul 2017 16:49:25 -0700 Subject: [PATCH 2/6] Refactored ReactIncremental-test to use ReactNoop instead of ReactDOM renderer --- .../fiber/__tests__/ReactIncremental-test.js | 140 +++++++++++------- 1 file changed, 90 insertions(+), 50 deletions(-) diff --git a/src/renderers/shared/fiber/__tests__/ReactIncremental-test.js b/src/renderers/shared/fiber/__tests__/ReactIncremental-test.js index ccd324073d0d..e89ac73ef0be 100644 --- a/src/renderers/shared/fiber/__tests__/ReactIncremental-test.js +++ b/src/renderers/shared/fiber/__tests__/ReactIncremental-test.js @@ -12,8 +12,6 @@ 'use strict'; var React; -var ReactDOM; -var ReactDOMFeatureFlags; var ReactNoop; var ReactFeatureFlags; var PropTypes; @@ -22,8 +20,6 @@ describe('ReactIncremental', () => { beforeEach(() => { jest.resetModules(); React = require('react'); - ReactDOM = require('react-dom'); - ReactDOMFeatureFlags = require('ReactDOMFeatureFlags'); ReactNoop = require('react-noop-renderer'); PropTypes = require('prop-types'); @@ -2404,11 +2400,18 @@ describe('ReactIncremental', () => { // TODO bvaughn Better annotate with comments? // TODO bvaughn Remove any tests that overlap with above? it('should update descendants with new context values', () => { + let rendered = []; + let instance; + class TopContextProvider extends React.Component { static childContextTypes = { count: PropTypes.number, }; - state = {count: 0}; + constructor() { + super(); + this.state = {count: 0}; + instance = this; + } getChildContext = () => ({ count: this.state.count, }); @@ -2427,26 +2430,36 @@ describe('ReactIncremental', () => { static contextTypes = { count: PropTypes.number, }; - render = () =>
count:{this.context.count}
; + render = () => { + rendered.push(`count:${this.context.count}`); + return null; + }; } - const container = document.createElement('div'); - const grandparent = ReactDOM.render( + ReactNoop.render( , - container, ); - expect(container.textContent).toBe('count:0'); - grandparent.updateCount(); - expect(container.textContent).toBe('count:1'); + ReactNoop.flush(); + expect(rendered).toEqual(['count:0']); + instance.updateCount(); + ReactNoop.flush(); + expect(rendered).toEqual(['count:0', 'count:1']); }); it('should update descendants with multiple context-providing anscestors with new context values', () => { + let rendered = []; + let instance; + class TopContextProvider extends React.Component { static childContextTypes = { count: PropTypes.number, }; - state = {count: 0}; + constructor() { + super(); + this.state = {count: 0}; + instance = this; + } getChildContext = () => ({ count: this.state.count, }); @@ -2471,30 +2484,40 @@ describe('ReactIncremental', () => { static contextTypes = { count: PropTypes.number, }; - render = () =>
count:{this.context.count}
; + render = () => { + rendered.push(`count:${this.context.count}`); + return null; + }; } - const container = document.createElement('div'); - const grandparent = ReactDOM.render( + ReactNoop.render( , - container, ); - expect(container.textContent).toBe('count:0'); - grandparent.updateCount(); - expect(container.textContent).toBe('count:1'); + ReactNoop.flush(); + expect(rendered).toEqual(['count:0']); + instance.updateCount(); + ReactNoop.flush(); + expect(rendered).toEqual(['count:0', 'count:1']); }); it('should not update descendants with new context values if shouldComponentUpdate returns false', () => { + let rendered = []; + let instance; + class TopContextProvider extends React.Component { static childContextTypes = { count: PropTypes.number, }; - state = {count: 0}; + constructor() { + super(); + this.state = {count: 0}; + instance = this; + } getChildContext = () => ({ count: this.state.count, }); @@ -2526,30 +2549,41 @@ describe('ReactIncremental', () => { static contextTypes = { count: PropTypes.number, }; - render = () =>
count:{this.context.count}
; + render = () => { + rendered.push(`count:${this.context.count}`); + return null; + }; } - const container = document.createElement('div'); - const grandparent = ReactDOM.render( + ReactNoop.render( , - container, ); - expect(container.textContent).toBe('count:0'); - grandparent.updateCount(); - expect(container.textContent).toBe('count:0'); + ReactNoop.flush(); + expect(rendered).toEqual(['count:0']); + instance.updateCount(); + ReactNoop.flush(); + expect(rendered).toEqual(['count:0']); }); it('should update descendants with new context values if setState() is called in the middle of the tree', () => { + let rendered = []; + let middleInstance; + let topInstance; + class TopContextProvider extends React.Component { static childContextTypes = { count: PropTypes.number, }; - state = {count: 0}; + constructor() { + super(); + this.state = {count: 0}; + topInstance = this; + } getChildContext = () => ({ count: this.state.count, }); @@ -2567,16 +2601,20 @@ describe('ReactIncremental', () => { render = () => this.props.children; } - let middle, name = 'brian'; class MiddleContextProvider extends React.Component { static childContextTypes = { name: PropTypes.string, }; + constructor() { + super(); + this.state = {name: 'brian'}; + middleInstance = this; + } getChildContext = () => ({ - name, + name: this.state.name, }); - componentDidMount = () => { - middle = this; + updateName = name => { + this.setState({name}); }; render = () => this.props.children; } @@ -2586,30 +2624,32 @@ describe('ReactIncremental', () => { count: PropTypes.number, name: PropTypes.string, }; - render = () => ( -
count:{this.context.count}, name:{this.context.name}
- ); + render = () => { + rendered.push(`count:${this.context.count}, name:${this.context.name}`); + return null; + }; } - const container = document.createElement('div'); - const grandparent = ReactDOM.render( + ReactNoop.render( - + + + , - container, ); - expect(container.textContent).toBe('count:0, name:brian'); - grandparent.updateCount(); - expect(container.textContent).toBe('count:0, name:brian'); - name = 'not brian'; - middle.forceUpdate(); - if (ReactDOMFeatureFlags.useFiber) { - expect(container.textContent).toBe('count:1, name:not brian'); - } else { - expect(container.textContent).toBe('count:0, name:not brian'); - } + ReactNoop.flush(); + expect(rendered).toEqual(['count:0, name:brian']); + topInstance.updateCount(); + ReactNoop.flush(); + expect(rendered).toEqual(['count:0, name:brian']); + middleInstance.updateName('not brian'); + ReactNoop.flush(); + expect(rendered).toEqual([ + 'count:0, name:brian', + 'count:1, name:not brian', + ]); }); }); From 97c3de66e3f716088ecd15b26e6d9fff243aaca6 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Mon, 31 Jul 2017 16:51:40 -0700 Subject: [PATCH 3/6] Added a to prevent an unnecessary Flow warning --- src/renderers/shared/fiber/ReactFiberContext.js | 1 + 1 file changed, 1 insertion(+) diff --git a/src/renderers/shared/fiber/ReactFiberContext.js b/src/renderers/shared/fiber/ReactFiberContext.js index 004a1de02f32..b6ede62d16ec 100644 --- a/src/renderers/shared/fiber/ReactFiberContext.js +++ b/src/renderers/shared/fiber/ReactFiberContext.js @@ -270,6 +270,7 @@ exports.invalidateContextProvider = function( pop(didPerformWorkStackCursor, workInProgress); if (didChange) { pop(contextStackCursor, workInProgress); + // $FlowFixMe - We know that this is always an object when didChange is true. push(contextStackCursor, mergedContext, workInProgress); } push(didPerformWorkStackCursor, didChange, workInProgress); From 14e3af9aeb647f468e4c12f985389787e3df9f4b Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Mon, 31 Jul 2017 17:01:57 -0700 Subject: [PATCH 4/6] Tidying up comments --- src/renderers/shared/fiber/ReactFiberContext.js | 1 + .../shared/fiber/__tests__/ReactIncremental-test.js | 7 ++----- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/src/renderers/shared/fiber/ReactFiberContext.js b/src/renderers/shared/fiber/ReactFiberContext.js index b6ede62d16ec..a67cd5b10633 100644 --- a/src/renderers/shared/fiber/ReactFiberContext.js +++ b/src/renderers/shared/fiber/ReactFiberContext.js @@ -259,6 +259,7 @@ exports.invalidateContextProvider = function( // Merge parent and own context. // Skip this if we're not updating due to sCU. + // This avoids unnecessarily recomputing memoized values. let mergedContext; if (didChange) { mergedContext = processChildContext(workInProgress, previousContext, true); diff --git a/src/renderers/shared/fiber/__tests__/ReactIncremental-test.js b/src/renderers/shared/fiber/__tests__/ReactIncremental-test.js index e89ac73ef0be..96b5d34c6605 100644 --- a/src/renderers/shared/fiber/__tests__/ReactIncremental-test.js +++ b/src/renderers/shared/fiber/__tests__/ReactIncremental-test.js @@ -2396,10 +2396,7 @@ describe('ReactIncremental', () => { }, ); - // TODO bvaughn Convert new tests to use ReactNoop? - // TODO bvaughn Better annotate with comments? - // TODO bvaughn Remove any tests that overlap with above? - it('should update descendants with new context values', () => { + it('updates descendants with new context values', () => { let rendered = []; let instance; @@ -2447,7 +2444,7 @@ describe('ReactIncremental', () => { expect(rendered).toEqual(['count:0', 'count:1']); }); - it('should update descendants with multiple context-providing anscestors with new context values', () => { + it('updates descendants with multiple context-providing anscestors with new context values', () => { let rendered = []; let instance; From 101c78c5b355f3f248b3d83ad5cb93430734fa17 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Mon, 31 Jul 2017 17:54:53 -0700 Subject: [PATCH 5/6] Fixed typo in test name --- src/renderers/shared/fiber/__tests__/ReactIncremental-test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/renderers/shared/fiber/__tests__/ReactIncremental-test.js b/src/renderers/shared/fiber/__tests__/ReactIncremental-test.js index 96b5d34c6605..e1a8dfc54cf1 100644 --- a/src/renderers/shared/fiber/__tests__/ReactIncremental-test.js +++ b/src/renderers/shared/fiber/__tests__/ReactIncremental-test.js @@ -2444,7 +2444,7 @@ describe('ReactIncremental', () => { expect(rendered).toEqual(['count:0', 'count:1']); }); - it('updates descendants with multiple context-providing anscestors with new context values', () => { + it('updates descendants with multiple context-providing ancestors with new context values', () => { let rendered = []; let instance; From 252e8f1ec1630edc53691970e53c971bef305a28 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Mon, 31 Jul 2017 21:06:40 -0700 Subject: [PATCH 6/6] Combined conditional branches in ReactFiberContext to be more efficient --- .../shared/fiber/ReactFiberContext.js | 28 +++++++++++-------- 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/src/renderers/shared/fiber/ReactFiberContext.js b/src/renderers/shared/fiber/ReactFiberContext.js index a67cd5b10633..3b9b7ed9d431 100644 --- a/src/renderers/shared/fiber/ReactFiberContext.js +++ b/src/renderers/shared/fiber/ReactFiberContext.js @@ -257,24 +257,28 @@ exports.invalidateContextProvider = function( 'This error is likely caused by a bug in React. Please file an issue.', ); - // Merge parent and own context. - // Skip this if we're not updating due to sCU. - // This avoids unnecessarily recomputing memoized values. - let mergedContext; if (didChange) { - mergedContext = processChildContext(workInProgress, previousContext, true); + // Merge parent and own context. + // Skip this if we're not updating due to sCU. + // This avoids unnecessarily recomputing memoized values. + const mergedContext = processChildContext( + workInProgress, + previousContext, + true, + ); instance.__reactInternalMemoizedMergedChildContext = mergedContext; - } - // Replace the old (or empty) context with the new one. - // It is important to unwind the context in the reverse order. - pop(didPerformWorkStackCursor, workInProgress); - if (didChange) { + // Replace the old (or empty) context with the new one. + // It is important to unwind the context in the reverse order. + pop(didPerformWorkStackCursor, workInProgress); pop(contextStackCursor, workInProgress); - // $FlowFixMe - We know that this is always an object when didChange is true. + // Now push the new context and mark that it has changed. push(contextStackCursor, mergedContext, workInProgress); + push(didPerformWorkStackCursor, didChange, workInProgress); + } else { + pop(didPerformWorkStackCursor, workInProgress); + push(didPerformWorkStackCursor, didChange, workInProgress); } - push(didPerformWorkStackCursor, didChange, workInProgress); }; exports.resetContext = function(): void {