Context improvements (#10334)

Refactored fiber context to fix a nested updates bug

Author: bvaughn

src/renderers/shared/fiber/ReactFiberBeginWork.js

@@ -277,6 +277,11 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(
     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<T, P, I, TI, PI, C, CX, PL>(
 
     // The context might have changed so we need to recalculate it.
     if (hasContext) {
-      invalidateContextProvider(workInProgress);
+      invalidateContextProvider(workInProgress, true);
     }
+
     return workInProgress.child;
   }
 

src/renderers/shared/fiber/ReactFiberContext.js

@@ -234,36 +234,51 @@ 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,
     'Expected to have an instance by this point. ' +
       'This error is likely caused by a bug in React. Please file an issue.',
   );
 
-  // Merge parent and own context.
-  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);
-  pop(contextStackCursor, workInProgress);
-  // Now push the new context and mark that it has changed.
-  push(contextStackCursor, mergedContext, workInProgress);
-  push(didPerformWorkStackCursor, true, workInProgress);
+  if (didChange) {
+    // 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);
+    pop(contextStackCursor, workInProgress);
+    // 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);
+  }
 };
 
 exports.resetContext = function(): void {

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

@@ -2395,4 +2395,258 @@ describe('ReactIncremental', () => {
       expect(cduNextProps).toEqual([{children: 'B'}]);
     },
   );
+
+  it('updates descendants with new context values', () => {
+    let rendered = [];
+    let instance;
+
+    class TopContextProvider extends React.Component {
+      static childContextTypes = {
+        count: PropTypes.number,
+      };
+      constructor() {
+        super();
+        this.state = {count: 0};
+        instance = this;
+      }
+      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 = () => {
+        rendered.push(`count:${this.context.count}`);
+        return null;
+      };
+    }
+
+    ReactNoop.render(
+      <TopContextProvider><Middle><Child /></Middle></TopContextProvider>,
+    );
+
+    ReactNoop.flush();
+    expect(rendered).toEqual(['count:0']);
+    instance.updateCount();
+    ReactNoop.flush();
+    expect(rendered).toEqual(['count:0', 'count:1']);
+  });
+
+  it('updates descendants with multiple context-providing ancestors with new context values', () => {
+    let rendered = [];
+    let instance;
+
+    class TopContextProvider extends React.Component {
+      static childContextTypes = {
+        count: PropTypes.number,
+      };
+      constructor() {
+        super();
+        this.state = {count: 0};
+        instance = this;
+      }
+      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 = () => {
+        rendered.push(`count:${this.context.count}`);
+        return null;
+      };
+    }
+
+    ReactNoop.render(
+      <TopContextProvider>
+        <MiddleContextProvider>
+          <Child />
+        </MiddleContextProvider>
+      </TopContextProvider>,
+    );
+
+    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,
+      };
+      constructor() {
+        super();
+        this.state = {count: 0};
+        instance = this;
+      }
+      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 = () => {
+        rendered.push(`count:${this.context.count}`);
+        return null;
+      };
+    }
+
+    ReactNoop.render(
+      <TopContextProvider>
+        <MiddleScu>
+          <MiddleContextProvider><Child /></MiddleContextProvider>
+        </MiddleScu>
+      </TopContextProvider>,
+    );
+
+    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,
+      };
+      constructor() {
+        super();
+        this.state = {count: 0};
+        topInstance = this;
+      }
+      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,
+      };
+      constructor() {
+        super();
+        this.state = {name: 'brian'};
+        middleInstance = this;
+      }
+      getChildContext = () => ({
+        name: this.state.name,
+      });
+      updateName = name => {
+        this.setState({name});
+      };
+      render = () => this.props.children;
+    }
+
+    class Child extends React.Component {
+      static contextTypes = {
+        count: PropTypes.number,
+        name: PropTypes.string,
+      };
+      render = () => {
+        rendered.push(`count:${this.context.count}, name:${this.context.name}`);
+        return null;
+      };
+    }
+
+    ReactNoop.render(
+      <TopContextProvider>
+        <MiddleScu>
+          <MiddleContextProvider>
+            <Child />
+          </MiddleContextProvider>
+        </MiddleScu>
+      </TopContextProvider>,
+    );
+
+    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',
+    ]);
+  });
 });