Repository: facebook/react
Pull request: Fix for failed Suspense layout semantics

Resolves #21676. Fixes failing test added in #21677

When adding the new Suspense/Offscreen effects semantics, we intentionally left out the `subtreeFlags` checks since these values aren't 100% reliable and the check is just an optimization. (Refer to the `// TODO (Offscreen)` comments in the source code.)

It looks like #21386 (re)added one though, which was causing a failure case originally reported here:
https://github.com/reactwg/react-18/discussions/31

For now, this PR just removes that check and adds a follow up _TODO_ comment. This is just a bandaid fix to allow us to resume rolling out the new Suspense layout semantics. A proper long-term fix would be to identify _why_ the `subtreeFlags` are incorrect in this case.

Changed files:
filename: packages/react-reconciler/src/ReactFiberCommitWork.new.js
patch: @@ -2210,36 +2210,35 @@ function commitLayoutEffects_begin(
         commitLayoutMountEffects_complete(subtreeRoot, root, committedLanes);
         continue;
       } else {
-        if ((fiber.subtreeFlags & LayoutMask) !== NoFlags) {
-          const current = fiber.alternate;
-          const wasHidden = current !== null && current.memoizedState !== null;
-          const newOffscreenSubtreeWasHidden =
-            wasHidden || offscreenSubtreeWasHidden;
-          const prevOffscreenSubtreeIsHidden = offscreenSubtreeIsHidden;
-          const prevOffscreenSubtreeWasHidden = offscreenSubtreeWasHidden;
-
-          // Traverse the Offscreen subtree with the current Offscreen as the root.
-          offscreenSubtreeIsHidden = newOffscreenSubtreeIsHidden;
-          offscreenSubtreeWasHidden = newOffscreenSubtreeWasHidden;
-          let child = firstChild;
-          while (child !== null) {
-            nextEffect = child;
-            commitLayoutEffects_begin(
-              child, // New root; bubble back up to here and stop.
-              root,
-              committedLanes,
-            );
-            child = child.sibling;
-          }
+        // TODO (Offscreen) Also check: subtreeFlags & LayoutMask
+        const current = fiber.alternate;
+        const wasHidden = current !== null && current.memoizedState !== null;
+        const newOffscreenSubtreeWasHidden =
+          wasHidden || offscreenSubtreeWasHidden;
+        const prevOffscreenSubtreeIsHidden = offscreenSubtreeIsHidden;
+        const prevOffscreenSubtreeWasHidden = offscreenSubtreeWasHidden;
+
+        // Traverse the Offscreen subtree with the current Offscreen as the root.
+        offscreenSubtreeIsHidden = newOffscreenSubtreeIsHidden;
+        offscreenSubtreeWasHidden = newOffscreenSubtreeWasHidden;
+        let child = firstChild;
+        while (child !== null) {
+          nextEffect = child;
+          commitLayoutEffects_begin(
+            child, // New root; bubble back up to here and stop.
+            root,
+            committedLanes,
+          );
+          child = child.sibling;
+        }
 
-          // Restore Offscreen state and resume in our-progress traversal.
-          nextEffect = fiber;
-          offscreenSubtreeIsHidden = prevOffscreenSubtreeIsHidden;
-          offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden;
-          commitLayoutMountEffects_complete(subtreeRoot, root, committedLanes);
+        // Restore Offscreen state and resume in our-progress traversal.
+        nextEffect = fiber;
+        offscreenSubtreeIsHidden = prevOffscreenSubtreeIsHidden;
+        offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden;
+        commitLayoutMountEffects_complete(subtreeRoot, root, committedLanes);
 
-          continue;
-        }
+        continue;
       }
     }
 

filename: packages/react-reconciler/src/ReactFiberCommitWork.old.js
patch: @@ -2210,36 +2210,35 @@ function commitLayoutEffects_begin(
         commitLayoutMountEffects_complete(subtreeRoot, root, committedLanes);
         continue;
       } else {
-        if ((fiber.subtreeFlags & LayoutMask) !== NoFlags) {
-          const current = fiber.alternate;
-          const wasHidden = current !== null && current.memoizedState !== null;
-          const newOffscreenSubtreeWasHidden =
-            wasHidden || offscreenSubtreeWasHidden;
-          const prevOffscreenSubtreeIsHidden = offscreenSubtreeIsHidden;
-          const prevOffscreenSubtreeWasHidden = offscreenSubtreeWasHidden;
-
-          // Traverse the Offscreen subtree with the current Offscreen as the root.
-          offscreenSubtreeIsHidden = newOffscreenSubtreeIsHidden;
-          offscreenSubtreeWasHidden = newOffscreenSubtreeWasHidden;
-          let child = firstChild;
-          while (child !== null) {
-            nextEffect = child;
-            commitLayoutEffects_begin(
-              child, // New root; bubble back up to here and stop.
-              root,
-              committedLanes,
-            );
-            child = child.sibling;
-          }
+        // TODO (Offscreen) Also check: subtreeFlags & LayoutMask
+        const current = fiber.alternate;
+        const wasHidden = current !== null && current.memoizedState !== null;
+        const newOffscreenSubtreeWasHidden =
+          wasHidden || offscreenSubtreeWasHidden;
+        const prevOffscreenSubtreeIsHidden = offscreenSubtreeIsHidden;
+        const prevOffscreenSubtreeWasHidden = offscreenSubtreeWasHidden;
+
+        // Traverse the Offscreen subtree with the current Offscreen as the root.
+        offscreenSubtreeIsHidden = newOffscreenSubtreeIsHidden;
+        offscreenSubtreeWasHidden = newOffscreenSubtreeWasHidden;
+        let child = firstChild;
+        while (child !== null) {
+          nextEffect = child;
+          commitLayoutEffects_begin(
+            child, // New root; bubble back up to here and stop.
+            root,
+            committedLanes,
+          );
+          child = child.sibling;
+        }
 
-          // Restore Offscreen state and resume in our-progress traversal.
-          nextEffect = fiber;
-          offscreenSubtreeIsHidden = prevOffscreenSubtreeIsHidden;
-          offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden;
-          commitLayoutMountEffects_complete(subtreeRoot, root, committedLanes);
+        // Restore Offscreen state and resume in our-progress traversal.
+        nextEffect = fiber;
+        offscreenSubtreeIsHidden = prevOffscreenSubtreeIsHidden;
+        offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden;
+        commitLayoutMountEffects_complete(subtreeRoot, root, committedLanes);
 
-          continue;
-        }
+        continue;
       }
     }
 

filename: packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js
patch: @@ -663,6 +663,64 @@ describe('ReactSuspense', () => {
     expect(root).toMatchRenderedOutput('new value');
   });
 
+  // @gate enableSuspenseLayoutEffectSemantics
+  it('re-fires layout effects when re-showing Suspense', () => {
+    function TextWithLayout(props) {
+      Scheduler.unstable_yieldValue(props.text);
+      React.useLayoutEffect(() => {
+        Scheduler.unstable_yieldValue('create layout');
+        return () => {
+          Scheduler.unstable_yieldValue('destroy layout');
+        };
+      }, []);
+      return props.text;
+    }
+
+    let _setShow;
+    function App(props) {
+      const [show, setShow] = React.useState(false);
+      _setShow = setShow;
+      return (
+        <Suspense fallback={<Text text="Loading..." />}>
+          <TextWithLayout text="Child 1" />
+          {show && <AsyncText ms={1000} text="Child 2" />}
+        </Suspense>
+      );
+    }
+
+    const root = ReactTestRenderer.create(<App />, {
+      unstable_isConcurrent: true,
+    });
+
+    expect(Scheduler).toFlushAndYield(['Child 1', 'create layout']);
+    expect(root).toMatchRenderedOutput('Child 1');
+
+    ReactTestRenderer.act(() => {
+      _setShow(true);
+    });
+    expect(Scheduler).toHaveYielded(
+      // DEV behavior differs from prod
+      // In DEV sometimes the work loop sync-flushes the commit
+      // where as in production, it schedules it behind a timeout.
+      // See shouldForceFlushFallbacksInDEV() usage
+      __DEV__
+        ? ['Child 1', 'Suspend! [Child 2]', 'Loading...', 'destroy layout']
+        : ['Child 1', 'Suspend! [Child 2]', 'Loading...'],
+    );
+    jest.advanceTimersByTime(1000);
+    expect(Scheduler).toHaveYielded(
+      // DEV behavior differs from prod
+      // In DEV sometimes the work loop sync-flushes the commit
+      // where as in production, it schedules it behind a timeout.
+      // See shouldForceFlushFallbacksInDEV() usage
+      __DEV__
+        ? ['Promise resolved [Child 2]']
+        : ['destroy layout', 'Promise resolved [Child 2]'],
+    );
+    expect(Scheduler).toFlushAndYield(['Child 1', 'Child 2', 'create layout']);
+    expect(root).toMatchRenderedOutput(['Child 1', 'Child 2'].join(''));
+  });
+
   describe('outside concurrent mode', () => {
     it('a mounted class component can suspend without losing state', () => {
       class TextWithLifecycle extends React.Component {