From 15e3dffb4c9ca9b9466f4ef1a6b8b2293d41e9d6 Mon Sep 17 00:00:00 2001
From: Dustan Kasten <dustan.kasten@gmail.com>
Date: Thu, 29 Mar 2018 14:16:02 -0400
Subject: [PATCH] Don't bail out on referential equality of Consumer's
 props.children function (#12470)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* Test case for React Context bailing out unexpectedly

* This is 💯% definitely not the correct fix at all

* Revert "This is 💯% definitely not the correct fix at all"

This reverts commit 8686c0f6bdc1cba3056fb2212f3f7740c749d33a.

* Formatting + minor tweaks to the test

* Don't bail out on consumer child equality

* Tweak the comment

* Pretty lint

* Silly Dan
---
 .../src/ReactFiberBeginWork.js                |   6 +-
 .../ReactNewContext-test.internal.js          | 112 ++++++++++++++----
 2 files changed, 95 insertions(+), 23 deletions(-)

diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js
index f3f9681332dcf..7ca1aa6f25574 100644
--- a/packages/react-reconciler/src/ReactFiberBeginWork.js
+++ b/packages/react-reconciler/src/ReactFiberBeginWork.js
@@ -994,10 +994,10 @@ export default function<T, P, I, TI, HI, PI, C, CC, CX, PL>(
         changedBits,
         renderExpirationTime,
       );
-    } else if (oldProps !== null && oldProps.children === newProps.children) {
-      // No change. Bailout early if children are the same.
-      return bailoutOnAlreadyFinishedWork(current, workInProgress);
     }
+    // There is no bailout on `children` equality because we expect people
+    // to often pass a bound method as a child, but it may reference
+    // `this.state` or `this.props` (and thus needs to re-render on `setState`).
 
     const render = newProps.children;
 
diff --git a/packages/react-reconciler/src/__tests__/ReactNewContext-test.internal.js b/packages/react-reconciler/src/__tests__/ReactNewContext-test.internal.js
index 4b3880fd626b3..c0aadacd1c030 100644
--- a/packages/react-reconciler/src/__tests__/ReactNewContext-test.internal.js
+++ b/packages/react-reconciler/src/__tests__/ReactNewContext-test.internal.js
@@ -727,39 +727,111 @@ describe('ReactNewContext', () => {
     expect(ReactNoop.getChildren()).toEqual([span('Child')]);
   });
 
-  it('consumer bails out if children and value are unchanged (like sCU)', () => {
+  it('consumer bails out if value is unchanged and something above bailed out', () => {
     const Context = React.createContext(0);
 
-    function Child() {
-      ReactNoop.yield('Child');
-      return <span prop="Child" />;
-    }
-
-    function renderConsumer(context) {
-      return <Child context={context} />;
+    function renderChildValue(value) {
+      ReactNoop.yield('Consumer');
+      return <span prop={value} />;
     }
 
-    function App(props) {
-      ReactNoop.yield('App');
+    function ChildWithInlineRenderCallback() {
+      ReactNoop.yield('ChildWithInlineRenderCallback');
+      // Note: we are intentionally passing an inline arrow. Don't refactor.
       return (
-        <Context.Provider value={props.value}>
-          <Context.Consumer>{renderConsumer}</Context.Consumer>
-        </Context.Provider>
+        <Context.Consumer>{value => renderChildValue(value)}</Context.Consumer>
       );
     }
 
-    // Initial mount
-    ReactNoop.render(<App value={1} />);
-    expect(ReactNoop.flush()).toEqual(['App', 'Child']);
-    expect(ReactNoop.getChildren()).toEqual([span('Child')]);
+    function ChildWithCachedRenderCallback() {
+      ReactNoop.yield('ChildWithCachedRenderCallback');
+      return <Context.Consumer>{renderChildValue}</Context.Consumer>;
+    }
 
-    // Update
+    class PureIndirection extends React.PureComponent {
+      render() {
+        ReactNoop.yield('PureIndirection');
+        return (
+          <React.Fragment>
+            <ChildWithInlineRenderCallback />
+            <ChildWithCachedRenderCallback />
+          </React.Fragment>
+        );
+      }
+    }
+
+    class App extends React.Component {
+      render() {
+        ReactNoop.yield('App');
+        return (
+          <Context.Provider value={this.props.value}>
+            <PureIndirection />
+          </Context.Provider>
+        );
+      }
+    }
+
+    // Initial mount
     ReactNoop.render(<App value={1} />);
     expect(ReactNoop.flush()).toEqual([
       'App',
-      // Child does not re-render
+      'PureIndirection',
+      'ChildWithInlineRenderCallback',
+      'Consumer',
+      'ChildWithCachedRenderCallback',
+      'Consumer',
     ]);
-    expect(ReactNoop.getChildren()).toEqual([span('Child')]);
+    expect(ReactNoop.getChildren()).toEqual([span(1), span(1)]);
+
+    // Update (bailout)
+    ReactNoop.render(<App value={1} />);
+    expect(ReactNoop.flush()).toEqual(['App']);
+    expect(ReactNoop.getChildren()).toEqual([span(1), span(1)]);
+
+    // Update (no bailout)
+    ReactNoop.render(<App value={2} />);
+    expect(ReactNoop.flush()).toEqual(['App', 'Consumer', 'Consumer']);
+    expect(ReactNoop.getChildren()).toEqual([span(2), span(2)]);
+  });
+
+  // Context consumer bails out on propagating "deep" updates when `value` hasn't changed.
+  // However, it doesn't bail out from rendering if the component above it re-rendered anyway.
+  // If we bailed out on referential equality, it would be confusing that you
+  // can call this.setState(), but an autobound render callback "blocked" the update.
+  // https://github.com/facebook/react/pull/12470#issuecomment-376917711
+  it('consumer does not bail out if there were no bailouts above it', () => {
+    const Context = React.createContext(0);
+
+    class App extends React.Component {
+      state = {
+        text: 'hello',
+      };
+
+      renderConsumer = context => {
+        ReactNoop.yield('App#renderConsumer');
+        return <span prop={this.state.text} />;
+      };
+
+      render() {
+        ReactNoop.yield('App');
+        return (
+          <Context.Provider value={this.props.value}>
+            <Context.Consumer>{this.renderConsumer}</Context.Consumer>
+          </Context.Provider>
+        );
+      }
+    }
+
+    // Initial mount
+    let inst;
+    ReactNoop.render(<App value={1} ref={ref => (inst = ref)} />);
+    expect(ReactNoop.flush()).toEqual(['App', 'App#renderConsumer']);
+    expect(ReactNoop.getChildren()).toEqual([span('hello')]);
+
+    // Update
+    inst.setState({text: 'goodbye'});
+    expect(ReactNoop.flush()).toEqual(['App', 'App#renderConsumer']);
+    expect(ReactNoop.getChildren()).toEqual([span('goodbye')]);
   });
 
   // This is a regression case for https://github.com/facebook/react/issues/12389.