From 3ff151daddb04ffaefe256d3cd5ccefd96f623ce Mon Sep 17 00:00:00 2001
From: Andrew Clark <git@andrewclark.io>
Date: Tue, 7 Apr 2020 16:40:41 -0700
Subject: [PATCH 1/2] Add another test for #18515 using pings

Adds a regression test for the same underlying bug as #18515 but using
pings.

Test already passes, but I confirmed it fails if you revert the fix
in #18515.
---
 ...tSuspenseWithNoopRenderer-test.internal.js | 113 ++++++++++++++++++
 1 file changed, 113 insertions(+)

diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js
index 966fc653ca127..d27b422f46c58 100644
--- a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js
+++ b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js
@@ -3645,4 +3645,117 @@ describe('ReactSuspenseWithNoopRenderer', () => {
       </>,
     );
   });
+
+  it('regression: ping at high priority causes update to be dropped', async () => {
+    const {useState, useTransition} = React;
+
+    let setTextA;
+    function A() {
+      const [textA, _setTextA] = useState('A');
+      setTextA = _setTextA;
+      return (
+        <Suspense fallback={<Text text="Loading..." />}>
+          <AsyncText text={textA} />
+        </Suspense>
+      );
+    }
+
+    let setTextB;
+    let startTransition;
+    function B() {
+      const [textB, _setTextB] = useState('B');
+      const [_startTransition] = useTransition({timeoutMs: 10000});
+      startTransition = _startTransition;
+      setTextB = _setTextB;
+      return (
+        <Suspense fallback={<Text text="Loading..." />}>
+          <AsyncText text={textB} />
+        </Suspense>
+      );
+    }
+
+    function App() {
+      return (
+        <>
+          <A />
+          <B />
+        </>
+      );
+    }
+
+    const root = ReactNoop.createRoot();
+    await ReactNoop.act(async () => {
+      await resolveText('A');
+      await resolveText('B');
+      root.render(<App />);
+    });
+    expect(Scheduler).toHaveYielded(['A', 'B']);
+    expect(root).toMatchRenderedOutput(
+      <>
+        <span prop="A" />
+        <span prop="B" />
+      </>,
+    );
+
+    await ReactNoop.act(async () => {
+      // Triggers suspense at normal pri
+      setTextA('A1');
+      // Triggers in an unrelated tree at a different pri
+      startTransition(() => {
+        // Update A again so that it doesn't suspend on A1. That way we can ping
+        // the A1 update without also pinging this one. This is a workaround
+        // because there's currently no way to render at a lower priority (B2)
+        // without including all updates at higher priority (A1).
+        setTextA('A2');
+        setTextB('B2');
+      });
+    });
+    expect(Scheduler).toHaveYielded([
+      'B',
+      'Suspend! [A1]',
+      'Loading...',
+
+      'Suspend! [A2]',
+      'Loading...',
+      'Suspend! [B2]',
+      'Loading...',
+    ]);
+    expect(root).toMatchRenderedOutput(
+      <>
+        <span prop="A" />
+        <span prop="B" />
+      </>,
+    );
+
+    await ReactNoop.act(async () => {
+      resolveText('A1');
+    });
+    expect(Scheduler).toHaveYielded([
+      'Promise resolved [A1]',
+      'A1',
+      'Suspend! [A2]',
+      'Loading...',
+      'Suspend! [B2]',
+      'Loading...',
+    ]);
+    expect(root).toMatchRenderedOutput(
+      <>
+        <span prop="A1" />
+        <span prop="B" />
+      </>,
+    );
+
+    // Commit the placeholder
+    Scheduler.unstable_advanceTime(20000);
+    await advanceTimers(20000);
+
+    expect(root).toMatchRenderedOutput(
+      <>
+        <span hidden={true} prop="A1" />
+        <span prop="Loading..." />
+        <span hidden={true} prop="B" />
+        <span prop="Loading..." />
+      </>,
+    );
+  });
 });

From 942d5557e227afb272c95511b881c106f6a0166e Mon Sep 17 00:00:00 2001
From: Andrew Clark <git@andrewclark.io>
Date: Tue, 7 Apr 2020 15:19:02 -0700
Subject: [PATCH 2/2] Set nextPendingLevel after commit, too

---
 .../src/ReactFiberWorkLoop.js                 | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js
index a83f9a6f25056..24420f9ebebbf 100644
--- a/packages/react-reconciler/src/ReactFiberWorkLoop.js
+++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js
@@ -679,9 +679,10 @@ function performConcurrentWorkOnRoot(root, didTimeout) {
 
     // We now have a consistent tree. The next step is either to commit it,
     // or, if something suspended, wait to commit it after a timeout.
-    const finishedWork: Fiber = ((root.finishedWork =
-      root.current.alternate): any);
+    const finishedWork: Fiber = (root.current.alternate: any);
+    root.finishedWork = finishedWork;
     root.finishedExpirationTime = expirationTime;
+    root.nextKnownPendingLevel = getRemainingExpirationTime(finishedWork);
     finishConcurrentRender(root, finishedWork, exitStatus, expirationTime);
   }
 
@@ -717,9 +718,6 @@ function finishConcurrentRender(
     case RootSuspended: {
       markRootSuspendedAtTime(root, expirationTime);
       const lastSuspendedTime = root.lastSuspendedTime;
-      if (expirationTime === lastSuspendedTime) {
-        root.nextKnownPendingLevel = getRemainingExpirationTime(finishedWork);
-      }
 
       // We have an acceptable loading state. We need to figure out if we
       // should immediately commit it or wait a bit.
@@ -792,9 +790,6 @@ function finishConcurrentRender(
     case RootSuspendedWithDelay: {
       markRootSuspendedAtTime(root, expirationTime);
       const lastSuspendedTime = root.lastSuspendedTime;
-      if (expirationTime === lastSuspendedTime) {
-        root.nextKnownPendingLevel = getRemainingExpirationTime(finishedWork);
-      }
 
       if (
         // do not delay if we're inside an act() scope
@@ -979,9 +974,10 @@ function performSyncWorkOnRoot(root) {
 
   // We now have a consistent tree. Because this is a sync render, we
   // will commit it even if something suspended.
-  root.finishedWork = (root.current.alternate: any);
+  const finishedWork: Fiber = (root.current.alternate: any);
+  root.finishedWork = finishedWork;
   root.finishedExpirationTime = expirationTime;
-
+  root.nextKnownPendingLevel = getRemainingExpirationTime(finishedWork);
   commitRoot(root);
 
   // Before exiting, make sure there's a callback scheduled for the next
@@ -1176,6 +1172,8 @@ function prepareFreshStack(root, expirationTime) {
     // to it later, in case the render we're about to start gets aborted.
     // Generally we only reach this path via a ping, but we shouldn't assume
     // that will always be the case.
+    // Note: This is defensive coding to prevent a pending commit from
+    // being dropped without being rescheduled. It shouldn't be necessary.
     if (lastPingedTime === NoWork || lastPingedTime > lastSuspendedTime) {
       root.lastPingedTime = lastSuspendedTime;
     }
@@ -1772,7 +1770,6 @@ function commitRootImpl(root, renderPriorityLevel) {
   root.callbackNode = null;
   root.callbackExpirationTime = NoWork;
   root.callbackPriority = NoPriority;
-  root.nextKnownPendingLevel = NoWork;
 
   // Update the first and last pending times on this root. The new first
   // pending time is whatever is left on the root fiber.