From 6cd365cac69682f778e00238d8aa5c69730c13af Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= <sema@fb.com>
Date: Tue, 29 Oct 2019 13:38:25 -0700
Subject: [PATCH] Don't treat the last row in hidden as deleted if already
 mounted (#17206)

Already mounted rows that resuspend may be considered as part of a tail
if they're at the end. However, for purposes of the tail="..." option
they don't get deleted. We deal with that in cutOffTailIfNeeded.

However, if they're also the first to suspend in the "hidden" case, we have
a special case that deletes the actual rendered row. This needs to consider
if that row was already mounted or things go wrong.
---
 .../src/ReactFiberCompleteWork.js             |  5 +-
 .../ReactSuspenseList-test.internal.js        | 90 ++++++++++++++++++-
 2 files changed, 92 insertions(+), 3 deletions(-)

diff --git a/packages/react-reconciler/src/ReactFiberCompleteWork.js b/packages/react-reconciler/src/ReactFiberCompleteWork.js
index b599388864..a8ea88728e 100644
--- a/packages/react-reconciler/src/ReactFiberCompleteWork.js
+++ b/packages/react-reconciler/src/ReactFiberCompleteWork.js
@@ -1098,10 +1098,11 @@ function completeWork(
             // This might have been modified.
             if (
               renderState.tail === null &&
-              renderState.tailMode === 'hidden'
+              renderState.tailMode === 'hidden' &&
+              !renderedTail.alternate
             ) {
               // We need to delete the row we just rendered.
-              // Reset the effect list to what it w as before we rendered this
+              // Reset the effect list to what it was before we rendered this
               // child. The nested children have already appended themselves.
               let lastEffect = (workInProgress.lastEffect =
                 renderState.lastEffect);
diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseList-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspenseList-test.internal.js
index c2c9b333ab..506737b7f4 100644
--- a/packages/react-reconciler/src/__tests__/ReactSuspenseList-test.internal.js
+++ b/packages/react-reconciler/src/__tests__/ReactSuspenseList-test.internal.js
@@ -2023,7 +2023,7 @@ describe('ReactSuspenseList', () => {
     );
   });
 
-  it('eventually resolves two nested forwards suspense list with a hidden tail', async () => {
+  it('eventually resolves two nested forwards suspense lists with a hidden tail', async () => {
     let B = createAsyncText('B');
 
     function Foo({showB}) {
@@ -2135,4 +2135,92 @@ describe('ReactSuspenseList', () => {
       </div>,
     );
   });
+
+  it('is able to re-suspend the last rows during an update with hidden', async () => {
+    let AsyncB = createAsyncText('B');
+
+    let setAsyncB;
+
+    function B() {
+      let [shouldBeAsync, setAsync] = React.useState(false);
+      setAsyncB = setAsync;
+
+      return shouldBeAsync ? (
+        <Suspense fallback={<Text text="Loading B" />}>
+          <AsyncB />
+        </Suspense>
+      ) : (
+        <Text text="Sync B" />
+      );
+    }
+
+    function Foo({updateList}) {
+      return (
+        <SuspenseList revealOrder="forwards" tail="hidden">
+          <Suspense key="A" fallback={<Text text="Loading A" />}>
+            <Text text="A" />
+          </Suspense>
+          <B key="B" updateList={updateList} />
+        </SuspenseList>
+      );
+    }
+
+    ReactNoop.render(<Foo />);
+
+    expect(Scheduler).toFlushAndYield(['A', 'Sync B']);
+
+    expect(ReactNoop).toMatchRenderedOutput(
+      <>
+        <span>A</span>
+        <span>Sync B</span>
+      </>,
+    );
+
+    let previousInst = setAsyncB;
+
+    // During an update we suspend on B.
+    ReactNoop.act(() => setAsyncB(true));
+
+    expect(Scheduler).toHaveYielded([
+      'Suspend! [B]',
+      'Loading B',
+      // The second pass is the "force hide" pass
+      'Loading B',
+    ]);
+
+    expect(ReactNoop).toMatchRenderedOutput(
+      <>
+        <span>A</span>
+        <span>Loading B</span>
+      </>,
+    );
+
+    // Before we resolve we'll rerender the whole list.
+    // This should leave the tree intact.
+    ReactNoop.act(() => ReactNoop.render(<Foo updateList={true} />));
+
+    expect(Scheduler).toHaveYielded(['A', 'Suspend! [B]', 'Loading B']);
+
+    expect(ReactNoop).toMatchRenderedOutput(
+      <>
+        <span>A</span>
+        <span>Loading B</span>
+      </>,
+    );
+
+    await AsyncB.resolve();
+
+    expect(Scheduler).toFlushAndYield(['B']);
+
+    expect(ReactNoop).toMatchRenderedOutput(
+      <>
+        <span>A</span>
+        <span>B</span>
+      </>,
+    );
+
+    // This should be the same instance. I.e. it didn't
+    // remount.
+    expect(previousInst).toBe(setAsyncB);
+  });
 });