From 1bcf7d2a896b6e7cc1f72879a4ddf8f75215bc77 Mon Sep 17 00:00:00 2001
From: Andrew Clark <git@andrewclark.io>
Date: Tue, 7 Apr 2020 18:53:31 -0700
Subject: [PATCH] Bugfix: Render phase update leads to dropped work

Render phase updates should not affect the `fiber.expirationTime` field.
We don't have to set anything on the fiber because we're going to
process the render phase update immediately.

We also shouldn't reset the `expirationTime` field in between render
passes because it represents the remaining work left in the update
queues. During the re-render, the updates that were skipped in the
original pass are not processed again.

I think my original motivation for using this field for render phase
updates was so I didn't have to add another module level variable.
---
 .../react-reconciler/src/ReactFiberHooks.js   | 18 ++++++----
 ...eactHooksWithNoopRenderer-test.internal.js | 35 +++++++++++++++++++
 2 files changed, 46 insertions(+), 7 deletions(-)

diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js
index 81514012002e..d23df4be9515 100644
--- a/packages/react-reconciler/src/ReactFiberHooks.js
+++ b/packages/react-reconciler/src/ReactFiberHooks.js
@@ -230,6 +230,11 @@ let workInProgressHook: Hook | null = null;
 // finished evaluating this component. This is an optimization so we know
 // whether we need to clear render phase updates after a throw.
 let didScheduleRenderPhaseUpdate: boolean = false;
+// Where an update was scheduled only during the current render pass. This
+// gets reset after each attempt.
+// TODO: Maybe there's some way to consolidate this with
+// `didScheduleRenderPhaseUpdate`. Or with `numberOfReRenders`.
+let didScheduleRenderPhaseUpdateDuringThisPass: boolean = false;
 
 const RE_RENDER_LIMIT = 25;
 
@@ -455,13 +460,12 @@ export function renderWithHooks<Props, SecondArg>(
   let children = Component(props, secondArg);
 
   // Check if there was a render phase update
-  if (workInProgress.expirationTime === renderExpirationTime) {
+  if (didScheduleRenderPhaseUpdateDuringThisPass) {
     // Keep rendering in a loop for as long as render phase updates continue to
     // be scheduled. Use a counter to prevent infinite loops.
     let numberOfReRenders: number = 0;
     do {
-      workInProgress.expirationTime = NoWork;
-
+      didScheduleRenderPhaseUpdateDuringThisPass = false;
       invariant(
         numberOfReRenders < RE_RENDER_LIMIT,
         'Too many re-renders. React limits the number of renders to prevent ' +
@@ -491,7 +495,7 @@ export function renderWithHooks<Props, SecondArg>(
         : HooksDispatcherOnRerender;
 
       children = Component(props, secondArg);
-    } while (workInProgress.expirationTime === renderExpirationTime);
+    } while (didScheduleRenderPhaseUpdateDuringThisPass);
   }
 
   // We can assume the previous dispatcher is always this one, since we set it
@@ -564,6 +568,7 @@ export function resetHooksAfterThrow(): void {
       }
       hook = hook.next;
     }
+    didScheduleRenderPhaseUpdate = false;
   }
 
   renderExpirationTime = NoWork;
@@ -581,7 +586,7 @@ export function resetHooksAfterThrow(): void {
     isUpdatingOpaqueValueInRenderPhase = false;
   }
 
-  didScheduleRenderPhaseUpdate = false;
+  didScheduleRenderPhaseUpdateDuringThisPass = false;
 }
 
 function mountWorkInProgressHook(): Hook {
@@ -1726,9 +1731,8 @@ function dispatchAction<S, A>(
     // This is a render phase update. Stash it in a lazily-created map of
     // queue -> linked list of updates. After this render pass, we'll restart
     // and apply the stashed updates on top of the work-in-progress hook.
-    didScheduleRenderPhaseUpdate = true;
+    didScheduleRenderPhaseUpdateDuringThisPass = didScheduleRenderPhaseUpdate = true;
     update.expirationTime = renderExpirationTime;
-    currentlyRenderingFiber.expirationTime = renderExpirationTime;
   } else {
     if (
       fiber.expirationTime === NoWork &&
diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js
index 383208275b3f..ecc37d2c2d02 100644
--- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js
+++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js
@@ -750,6 +750,41 @@ describe('ReactHooksWithNoopRenderer', () => {
       expect(root).toMatchRenderedOutput(<span prop="B:0" />);
     });
 
+    it('regression: render phase updates cause lower pri work to be dropped', async () => {
+      let setRow;
+      function ScrollView() {
+        const [row, _setRow] = useState(10);
+        setRow = _setRow;
+
+        const [scrollDirection, setScrollDirection] = useState('Up');
+        const [prevRow, setPrevRow] = useState(null);
+
+        if (prevRow !== row) {
+          setScrollDirection(prevRow !== null && row > prevRow ? 'Down' : 'Up');
+          setPrevRow(row);
+        }
+
+        return <Text text={scrollDirection} />;
+      }
+
+      const root = ReactNoop.createRoot();
+
+      await act(async () => {
+        root.render(<ScrollView row={10} />);
+      });
+      expect(Scheduler).toHaveYielded(['Up']);
+      expect(root).toMatchRenderedOutput(<span prop="Up" />);
+
+      await act(async () => {
+        ReactNoop.discreteUpdates(() => {
+          setRow(5);
+        });
+        setRow(20);
+      });
+      expect(Scheduler).toHaveYielded(['Up', 'Down']);
+      expect(root).toMatchRenderedOutput(<span prop="Down" />);
+    });
+
     // TODO: This should probably warn
     it.experimental('calling startTransition inside render phase', async () => {
       let startTransition;