From 339d6cb32bc80dd8ed9d500c634a47fa0dafc8d1 Mon Sep 17 00:00:00 2001
From: Andrew Clark <acdlite@me.com>
Date: Thu, 12 Oct 2017 21:05:58 -0700
Subject: [PATCH] Sync setStates inside willUpdate and didUpdate should flush
 at same time (#11212)

In sync mode, we downgrade sync priority work to task work when we're in
the commit phase, but not in the render phase. That means if you
schedule updates in both phases, the render phase update will flush
first, and the commit phase update will flush after that. What should
really happen is that both updates flush at the same time.

To solve this, updates in the commit phase are now given sync priority.
The distinction between task and sync really only exists to account for
a historical quirk in the behavior of top-level mounts. (Refer to the
test case titled "initial mount is sync inside batchedUpdates".)

Ideally, there would only be one priority for both sync and task. This
gets us closer to that model, while still accounting for
top-level mounts.
---
 src/renderers/__tests__/ReactUpdates-test.js  | 102 ++++++++++++++++++
 .../shared/fiber/ReactFiberScheduler.js       |   7 +-
 2 files changed, 107 insertions(+), 2 deletions(-)

diff --git a/src/renderers/__tests__/ReactUpdates-test.js b/src/renderers/__tests__/ReactUpdates-test.js
index fdfb92c532..f3cc2dd5ca 100644
--- a/src/renderers/__tests__/ReactUpdates-test.js
+++ b/src/renderers/__tests__/ReactUpdates-test.js
@@ -1072,6 +1072,108 @@ describe('ReactUpdates', () => {
     expect(ops).toEqual(['Hello', '']);
   });
 
+  it(
+    'in sync mode, updates in componentWillUpdate and componentDidUpdate ' +
+      'should both flush in the immediately subsequent commit',
+    () => {
+      let ops = [];
+      class Foo extends React.Component {
+        state = {a: false, b: false};
+        componentWillUpdate(_, nextState) {
+          if (!nextState.a) {
+            this.setState({a: true});
+          }
+        }
+        componentDidUpdate() {
+          ops.push('Foo updated');
+          if (!this.state.b) {
+            this.setState({b: true});
+          }
+        }
+        render() {
+          ops.push(`a: ${this.state.a}, b: ${this.state.b}`);
+          return null;
+        }
+      }
+
+      const container = document.createElement('div');
+      // Mount
+      ReactDOM.render(<Foo />, container);
+      // Root update
+      ReactDOM.render(<Foo />, container);
+      expect(ops).toEqual([
+        // Mount
+        'a: false, b: false',
+        // Root update
+        'a: false, b: false',
+        'Foo updated',
+        // Subsequent update (both a and b should have flushed)
+        'a: true, b: true',
+        'Foo updated',
+        // There should not be any additional updates
+      ]);
+    },
+  );
+
+  it(
+    'in sync mode, updates in componentWillUpdate and componentDidUpdate ' +
+      '(on a sibling) should both flush in the immediately subsequent commit',
+    () => {
+      let ops = [];
+      class Foo extends React.Component {
+        state = {a: false};
+        componentWillUpdate(_, nextState) {
+          if (!nextState.a) {
+            this.setState({a: true});
+          }
+        }
+        componentDidUpdate() {
+          ops.push('Foo updated');
+        }
+        render() {
+          ops.push(`a: ${this.state.a}`);
+          return null;
+        }
+      }
+
+      class Bar extends React.Component {
+        state = {b: false};
+        componentDidUpdate() {
+          ops.push('Bar updated');
+          if (!this.state.b) {
+            this.setState({b: true});
+          }
+        }
+        render() {
+          ops.push(`b: ${this.state.b}`);
+          return null;
+        }
+      }
+
+      const container = document.createElement('div');
+      // Mount
+      ReactDOM.render(<div><Foo /><Bar /></div>, container);
+      // Root update
+      ReactDOM.render(<div><Foo /><Bar /></div>, container);
+      expect(ops).toEqual([
+        // Mount
+        'a: false',
+        'b: false',
+        // Root update
+        'a: false',
+        'b: false',
+        'Foo updated',
+        'Bar updated',
+        // Subsequent update (both a and b should have flushed)
+        'a: true',
+        'b: true',
+        'Foo updated',
+        'Bar updated',
+        // There should not be any additional updates
+      ]);
+    },
+  );
+
   it('does not re-render if state update is null', () => {
     let container = document.createElement('div');
 
diff --git a/src/renderers/shared/fiber/ReactFiberScheduler.js b/src/renderers/shared/fiber/ReactFiberScheduler.js
index 479c730b49..bf026e60f4 100644
--- a/src/renderers/shared/fiber/ReactFiberScheduler.js
+++ b/src/renderers/shared/fiber/ReactFiberScheduler.js
@@ -1494,7 +1494,7 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(
       if (isCommitting) {
         // Updates that occur during the commit phase should have task priority
         // by default.
-        expirationTime = Task;
+        expirationTime = Sync;
       } else {
         // Updates during the render phase should expire at the same time as
         // the work that is being rendered.
@@ -1517,7 +1517,10 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(
       }
     }
 
-    if (expirationTime === Sync && (isCommitting || isBatchingUpdates)) {
+    if (
+      expirationTime === Sync &&
+      (isBatchingUpdates || (isUnbatchingUpdates && isCommitting))
+    ) {
       // If we're in a batch, or in the commit phase, downgrade sync to task
       return Task;
     }