From b7edb4b9c45a3c34c9b2c29be046fdc8b2ba5673 Mon Sep 17 00:00:00 2001
From: Andrew Clark <acdlite@fb.com>
Date: Thu, 16 Nov 2017 15:00:22 -0800
Subject: [PATCH] Updates at the same priority should not interrupt current
 render

When we're rendering work at a specific level, and a higher priority
update comes in, we interrupt the current work and restart at the
higher priority. The rationale is that the high priority update is
likely cheaper to render that the lower one, so it's usually worth
throwing out the current work to get the high pri update on the screen
as soon as possible.

Currently, we also interrupt the current work if an update of *equal*
priority is scheduled. The rationale here is less clear: the only reason
to do this is if both updates are expected to flush at the same time,
to prevent tearing. But this usually isn't the case. Separate setStates
are usually distinct updates that can be flushed separately, especially
if the components that are being updated are in separate subtrees.

An exception is in Flux-like systems where multiple setStates are the
result of a single conceptual update/event/dispatch. We can add an
explicit API for batching in the future; in fact, we'd likely need one
anyway to account for expiration accidentally causing consecutive
updates to fall into separate buckets.
---
 packages/react-noop-renderer/src/ReactNoop.js |  6 +-
 .../src/ReactFiberScheduler.js                |  2 +-
 .../src/__tests__/ReactIncremental-test.js    | 67 +++++++++++++++++++
 3 files changed, 73 insertions(+), 2 deletions(-)

diff --git a/packages/react-noop-renderer/src/ReactNoop.js b/packages/react-noop-renderer/src/ReactNoop.js
index a25821006c5f..f7a895ff9bb1 100644
--- a/packages/react-noop-renderer/src/ReactNoop.js
+++ b/packages/react-noop-renderer/src/ReactNoop.js
@@ -455,7 +455,11 @@ var ReactNoop = {
 
   unbatchedUpdates: NoopRenderer.unbatchedUpdates,
 
-  flushSync: NoopRenderer.flushSync,
+  flushSync(fn: () => mixed) {
+    yieldedValues = [];
+    NoopRenderer.flushSync(fn);
+    return yieldedValues;
+  },
 
   // Logs the current state of the tree.
   dumpTree(rootID: string = DEFAULT_ROOT_ID) {
diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js
index 5d85c0ad179d..5f47c83c1ea8 100644
--- a/packages/react-reconciler/src/ReactFiberScheduler.js
+++ b/packages/react-reconciler/src/ReactFiberScheduler.js
@@ -1202,7 +1202,7 @@ export default function<T, P, I, TI, HI, PI, C, CC, CX, PL>(
           if (
             !isWorking &&
             root === nextRoot &&
-            expirationTime <= nextRenderExpirationTime
+            expirationTime < nextRenderExpirationTime
           ) {
             // Restart the root from the top.
             if (nextUnitOfWork !== null) {
diff --git a/packages/react-reconciler/src/__tests__/ReactIncremental-test.js b/packages/react-reconciler/src/__tests__/ReactIncremental-test.js
index 7979db6cece5..5bb476554336 100644
--- a/packages/react-reconciler/src/__tests__/ReactIncremental-test.js
+++ b/packages/react-reconciler/src/__tests__/ReactIncremental-test.js
@@ -168,6 +168,7 @@ describe('ReactIncremental', () => {
     ops = [];
 
     // This will abort the previous work and restart
+    ReactNoop.flushSync(() => ReactNoop.render(null));
     ReactNoop.render(<Foo text="baz" />);
 
     // Flush part of the new work
@@ -221,6 +222,7 @@ describe('ReactIncremental', () => {
     expect(ops).toEqual(['setState1']);
 
     // This will abort the previous work and restart
+    ReactNoop.flushSync(() => ReactNoop.render(<Foo />));
     inst.setState(
       () => {
         ops.push('setState2');
@@ -1877,6 +1879,8 @@ describe('ReactIncremental', () => {
     );
     ReactNoop.flush();
     expect(ops).toEqual([
+      'ShowLocale {"locale":"sv"}',
+      'ShowBoth {"locale":"sv"}',
       'Intl {}',
       'ShowLocale {"locale":"en"}',
       'Router {}',
@@ -2648,4 +2652,67 @@ describe('ReactIncremental', () => {
       'count:1, name:not brian',
     ]);
   });
+
+  it('does not interrupt for update at same priority', () => {
+    function Parent(props) {
+      ReactNoop.yield('Parent: ' + props.step);
+      return <Child step={props.step} />;
+    }
+
+    function Child(props) {
+      ReactNoop.yield('Child: ' + props.step);
+      return null;
+    }
+
+    ReactNoop.render(<Parent step={1} />);
+    ReactNoop.flushThrough(['Parent: 1']);
+
+    // Interrupt at same priority
+    ReactNoop.render(<Parent step={2} />);
+
+    expect(ReactNoop.flush()).toEqual(['Child: 1', 'Parent: 2', 'Child: 2']);
+  });
+
+  it('does not interrupt for update at lower priority', () => {
+    function Parent(props) {
+      ReactNoop.yield('Parent: ' + props.step);
+      return <Child step={props.step} />;
+    }
+
+    function Child(props) {
+      ReactNoop.yield('Child: ' + props.step);
+      return null;
+    }
+
+    ReactNoop.render(<Parent step={1} />);
+    ReactNoop.flushThrough(['Parent: 1']);
+
+    // Interrupt at lower priority
+    ReactNoop.expire(2000);
+    ReactNoop.render(<Parent step={2} />);
+
+    expect(ReactNoop.flush()).toEqual(['Child: 1', 'Parent: 2', 'Child: 2']);
+  });
+
+  it('does interrupt for update at higher priority', () => {
+    function Parent(props) {
+      ReactNoop.yield('Parent: ' + props.step);
+      return <Child step={props.step} />;
+    }
+
+    function Child(props) {
+      ReactNoop.yield('Child: ' + props.step);
+      return null;
+    }
+
+    ReactNoop.render(<Parent step={1} />);
+    ReactNoop.flushThrough(['Parent: 1']);
+
+    // Interrupt at higher priority
+    expect(
+      ReactNoop.flushSync(() => ReactNoop.render(<Parent step={2} />)),
+    ).toEqual(['Parent: 2', 'Child: 2']);
+
+    expect(ReactNoop.flush()).toEqual([]);
+  });
 });