Repository: facebook/react
Pull request: Never attach ping listeners in legacy Suspense

I noticed a weird branch where we attach a ping listener even in legacy mode. It's weird because this shouldn't be necessary. Fallbacks always synchronously commit in legacy mode, so pings never happen. (A "ping" is when a suspended promise resolves before the fallback has committed.)

It took me a moment to remember why this case exists, but it's related to React.lazy.

There's a special case where we suspend while reconciling the children of a Suspense boundary's inner Offscreen wrapper fiber. This happens when a React.lazy component is a direct child of a Suspense boundary.

Suspense boundaries are implemented as multiple fibers, but they are a single conceptual unit. The legacy mode behavior where we pretend the suspended fiber committed as `null` won't work, because in this case the "suspended" fiber is the inner Offscreen wrapper.

Because the contents of the boundary haven't started rendering yet (i.e. nothing in the tree has partially rendered) we can switch to the regular, concurrent mode behavior: mark the boundary with ShouldCapture and enter the unwind phase.

However, even though we're switching to the concurrent mode behavior, we don't need to attach a ping listener. So I refactored the logic so that it doesn't escape back into the regular path.

It's not really a big deal that we attach an unnecessary ping listener, since this case is so unusual. The motivation is not performance related — it's to make the logic clearer, because I'm about to add another case where we trigger a Suspense boundary without attaching a ping listener.

To confirm we have coverage for this branch, I commented it out and ran the test suite. The relevant tests are in ReactLazy-test.js.

I suggest reviewing without whitespace changes: https://github.com/facebook/react/pull/22407/files?diff=split&w=1

Changed files:
filename: packages/react-reconciler/src/ReactFiberThrow.new.js
patch: @@ -297,76 +297,82 @@ function throwException(
           wakeables.add(wakeable);
         }
 
-        // If the boundary is in legacy mode, we should *not*
-        // suspend the commit. Pretend as if the suspended component rendered
-        // null and keep rendering. In the commit phase, we'll schedule a
-        // subsequent synchronous update to re-render the Suspense.
-        //
-        // Note: It doesn't matter whether the component that suspended was
-        // inside a concurrent mode tree. If the Suspense is outside of it, we
-        // should *not* suspend the commit.
-        //
-        // If the suspense boundary suspended itself suspended, we don't have to
-        // do this trick because nothing was partially started. We can just
-        // directly do a second pass over the fallback in this render and
-        // pretend we meant to render that directly.
-        if (
-          (workInProgress.mode & ConcurrentMode) === NoMode &&
-          workInProgress !== returnFiber
-        ) {
-          workInProgress.flags |= DidCapture;
-          sourceFiber.flags |= ForceUpdateForLegacySuspense;
+        if ((workInProgress.mode & ConcurrentMode) === NoMode) {
+          // Legacy Mode Suspense
+          //
+          // If the boundary is in legacy mode, we should *not*
+          // suspend the commit. Pretend as if the suspended component rendered
+          // null and keep rendering. When the Suspense boundary completes,
+          // we'll do a second pass to render the fallback.
+          if (workInProgress === returnFiber) {
+            // Special case where we suspended while reconciling the children of
+            // a Suspense boundary's inner Offscreen wrapper fiber. This happens
+            // when a React.lazy component is a direct child of a
+            // Suspense boundary.
+            //
+            // Suspense boundaries are implemented as multiple fibers, but they
+            // are a single conceptual unit. The legacy mode behavior where we
+            // pretend the suspended fiber committed as `null` won't work,
+            // because in this case the "suspended" fiber is the inner
+            // Offscreen wrapper.
+            //
+            // Because the contents of the boundary haven't started rendering
+            // yet (i.e. nothing in the tree has partially rendered) we can
+            // switch to the regular, concurrent mode behavior: mark the
+            // boundary with ShouldCapture and enter the unwind phase.
+            workInProgress.flags |= ShouldCapture;
+          } else {
+            workInProgress.flags |= DidCapture;
+            sourceFiber.flags |= ForceUpdateForLegacySuspense;
 
-          // We're going to commit this fiber even though it didn't complete.
-          // But we shouldn't call any lifecycle methods or callbacks. Remove
-          // all lifecycle effect tags.
-          sourceFiber.flags &= ~(LifecycleEffectMask | Incomplete);
+            // We're going to commit this fiber even though it didn't complete.
+            // But we shouldn't call any lifecycle methods or callbacks. Remove
+            // all lifecycle effect tags.
+            sourceFiber.flags &= ~(LifecycleEffectMask | Incomplete);
 
-          if (supportsPersistence && enablePersistentOffscreenHostContainer) {
-            // Another legacy Suspense quirk. In persistent mode, if this is the
-            // initial mount, override the props of the host container to hide
-            // its contents.
-            const currentSuspenseBoundary = workInProgress.alternate;
-            if (currentSuspenseBoundary === null) {
-              const offscreenFiber: Fiber = (workInProgress.child: any);
-              const offscreenContainer = offscreenFiber.child;
-              if (offscreenContainer !== null) {
-                const children = offscreenContainer.memoizedProps.children;
-                const containerProps = getOffscreenContainerProps(
-                  'hidden',
-                  children,
-                );
-                offscreenContainer.pendingProps = containerProps;
-                offscreenContainer.memoizedProps = containerProps;
+            if (supportsPersistence && enablePersistentOffscreenHostContainer) {
+              // Another legacy Suspense quirk. In persistent mode, if this is the
+              // initial mount, override the props of the host container to hide
+              // its contents.
+              const currentSuspenseBoundary = workInProgress.alternate;
+              if (currentSuspenseBoundary === null) {
+                const offscreenFiber: Fiber = (workInProgress.child: any);
+                const offscreenContainer = offscreenFiber.child;
+                if (offscreenContainer !== null) {
+                  const children = offscreenContainer.memoizedProps.children;
+                  const containerProps = getOffscreenContainerProps(
+                    'hidden',
+                    children,
+                  );
+                  offscreenContainer.pendingProps = containerProps;
+                  offscreenContainer.memoizedProps = containerProps;
+                }
               }
             }
-          }
 
-          if (sourceFiber.tag === ClassComponent) {
-            const currentSourceFiber = sourceFiber.alternate;
-            if (currentSourceFiber === null) {
-              // This is a new mount. Change the tag so it's not mistaken for a
-              // completed class component. For example, we should not call
-              // componentWillUnmount if it is deleted.
-              sourceFiber.tag = IncompleteClassComponent;
-            } else {
-              // When we try rendering again, we should not reuse the current fiber,
-              // since it's known to be in an inconsistent state. Use a force update to
-              // prevent a bail out.
-              const update = createUpdate(NoTimestamp, SyncLane);
-              update.tag = ForceUpdate;
-              enqueueUpdate(sourceFiber, update, SyncLane);
+            if (sourceFiber.tag === ClassComponent) {
+              const currentSourceFiber = sourceFiber.alternate;
+              if (currentSourceFiber === null) {
+                // This is a new mount. Change the tag so it's not mistaken for a
+                // completed class component. For example, we should not call
+                // componentWillUnmount if it is deleted.
+                sourceFiber.tag = IncompleteClassComponent;
+              } else {
+                // When we try rendering again, we should not reuse the current fiber,
+                // since it's known to be in an inconsistent state. Use a force update to
+                // prevent a bail out.
+                const update = createUpdate(NoTimestamp, SyncLane);
+                update.tag = ForceUpdate;
+                enqueueUpdate(sourceFiber, update, SyncLane);
+              }
             }
-          }
-
-          // The source fiber did not complete. Mark it with Sync priority to
-          // indicate that it still has pending work.
-          sourceFiber.lanes = mergeLanes(sourceFiber.lanes, SyncLane);
 
-          // Exit without suspending.
+            // The source fiber did not complete. Mark it with Sync priority to
+            // indicate that it still has pending work.
+            sourceFiber.lanes = mergeLanes(sourceFiber.lanes, SyncLane);
+          }
           return;
         }
-
         // Confirmed that the boundary is in a concurrent mode tree. Continue
         // with the normal suspend path.
         //
@@ -415,7 +421,6 @@ function throwException(
         // TODO: I think we can remove this, since we now use `DidCapture` in
         // the begin phase to prevent an early bailout.
         workInProgress.lanes = rootRenderLanes;
-
         return;
       }
       // This boundary already captured during this render. Continue to the next

filename: packages/react-reconciler/src/ReactFiberThrow.old.js
patch: @@ -297,76 +297,82 @@ function throwException(
           wakeables.add(wakeable);
         }
 
-        // If the boundary is in legacy mode, we should *not*
-        // suspend the commit. Pretend as if the suspended component rendered
-        // null and keep rendering. In the commit phase, we'll schedule a
-        // subsequent synchronous update to re-render the Suspense.
-        //
-        // Note: It doesn't matter whether the component that suspended was
-        // inside a concurrent mode tree. If the Suspense is outside of it, we
-        // should *not* suspend the commit.
-        //
-        // If the suspense boundary suspended itself suspended, we don't have to
-        // do this trick because nothing was partially started. We can just
-        // directly do a second pass over the fallback in this render and
-        // pretend we meant to render that directly.
-        if (
-          (workInProgress.mode & ConcurrentMode) === NoMode &&
-          workInProgress !== returnFiber
-        ) {
-          workInProgress.flags |= DidCapture;
-          sourceFiber.flags |= ForceUpdateForLegacySuspense;
+        if ((workInProgress.mode & ConcurrentMode) === NoMode) {
+          // Legacy Mode Suspense
+          //
+          // If the boundary is in legacy mode, we should *not*
+          // suspend the commit. Pretend as if the suspended component rendered
+          // null and keep rendering. When the Suspense boundary completes,
+          // we'll do a second pass to render the fallback.
+          if (workInProgress === returnFiber) {
+            // Special case where we suspended while reconciling the children of
+            // a Suspense boundary's inner Offscreen wrapper fiber. This happens
+            // when a React.lazy component is a direct child of a
+            // Suspense boundary.
+            //
+            // Suspense boundaries are implemented as multiple fibers, but they
+            // are a single conceptual unit. The legacy mode behavior where we
+            // pretend the suspended fiber committed as `null` won't work,
+            // because in this case the "suspended" fiber is the inner
+            // Offscreen wrapper.
+            //
+            // Because the contents of the boundary haven't started rendering
+            // yet (i.e. nothing in the tree has partially rendered) we can
+            // switch to the regular, concurrent mode behavior: mark the
+            // boundary with ShouldCapture and enter the unwind phase.
+            workInProgress.flags |= ShouldCapture;
+          } else {
+            workInProgress.flags |= DidCapture;
+            sourceFiber.flags |= ForceUpdateForLegacySuspense;
 
-          // We're going to commit this fiber even though it didn't complete.
-          // But we shouldn't call any lifecycle methods or callbacks. Remove
-          // all lifecycle effect tags.
-          sourceFiber.flags &= ~(LifecycleEffectMask | Incomplete);
+            // We're going to commit this fiber even though it didn't complete.
+            // But we shouldn't call any lifecycle methods or callbacks. Remove
+            // all lifecycle effect tags.
+            sourceFiber.flags &= ~(LifecycleEffectMask | Incomplete);
 
-          if (supportsPersistence && enablePersistentOffscreenHostContainer) {
-            // Another legacy Suspense quirk. In persistent mode, if this is the
-            // initial mount, override the props of the host container to hide
-            // its contents.
-            const currentSuspenseBoundary = workInProgress.alternate;
-            if (currentSuspenseBoundary === null) {
-              const offscreenFiber: Fiber = (workInProgress.child: any);
-              const offscreenContainer = offscreenFiber.child;
-              if (offscreenContainer !== null) {
-                const children = offscreenContainer.memoizedProps.children;
-                const containerProps = getOffscreenContainerProps(
-                  'hidden',
-                  children,
-                );
-                offscreenContainer.pendingProps = containerProps;
-                offscreenContainer.memoizedProps = containerProps;
+            if (supportsPersistence && enablePersistentOffscreenHostContainer) {
+              // Another legacy Suspense quirk. In persistent mode, if this is the
+              // initial mount, override the props of the host container to hide
+              // its contents.
+              const currentSuspenseBoundary = workInProgress.alternate;
+              if (currentSuspenseBoundary === null) {
+                const offscreenFiber: Fiber = (workInProgress.child: any);
+                const offscreenContainer = offscreenFiber.child;
+                if (offscreenContainer !== null) {
+                  const children = offscreenContainer.memoizedProps.children;
+                  const containerProps = getOffscreenContainerProps(
+                    'hidden',
+                    children,
+                  );
+                  offscreenContainer.pendingProps = containerProps;
+                  offscreenContainer.memoizedProps = containerProps;
+                }
               }
             }
-          }
 
-          if (sourceFiber.tag === ClassComponent) {
-            const currentSourceFiber = sourceFiber.alternate;
-            if (currentSourceFiber === null) {
-              // This is a new mount. Change the tag so it's not mistaken for a
-              // completed class component. For example, we should not call
-              // componentWillUnmount if it is deleted.
-              sourceFiber.tag = IncompleteClassComponent;
-            } else {
-              // When we try rendering again, we should not reuse the current fiber,
-              // since it's known to be in an inconsistent state. Use a force update to
-              // prevent a bail out.
-              const update = createUpdate(NoTimestamp, SyncLane);
-              update.tag = ForceUpdate;
-              enqueueUpdate(sourceFiber, update, SyncLane);
+            if (sourceFiber.tag === ClassComponent) {
+              const currentSourceFiber = sourceFiber.alternate;
+              if (currentSourceFiber === null) {
+                // This is a new mount. Change the tag so it's not mistaken for a
+                // completed class component. For example, we should not call
+                // componentWillUnmount if it is deleted.
+                sourceFiber.tag = IncompleteClassComponent;
+              } else {
+                // When we try rendering again, we should not reuse the current fiber,
+                // since it's known to be in an inconsistent state. Use a force update to
+                // prevent a bail out.
+                const update = createUpdate(NoTimestamp, SyncLane);
+                update.tag = ForceUpdate;
+                enqueueUpdate(sourceFiber, update, SyncLane);
+              }
             }
-          }
-
-          // The source fiber did not complete. Mark it with Sync priority to
-          // indicate that it still has pending work.
-          sourceFiber.lanes = mergeLanes(sourceFiber.lanes, SyncLane);
 
-          // Exit without suspending.
+            // The source fiber did not complete. Mark it with Sync priority to
+            // indicate that it still has pending work.
+            sourceFiber.lanes = mergeLanes(sourceFiber.lanes, SyncLane);
+          }
           return;
         }
-
         // Confirmed that the boundary is in a concurrent mode tree. Continue
         // with the normal suspend path.
         //
@@ -415,7 +421,6 @@ function throwException(
         // TODO: I think we can remove this, since we now use `DidCapture` in
         // the begin phase to prevent an early bailout.
         workInProgress.lanes = rootRenderLanes;
-
         return;
       }
       // This boundary already captured during this render. Continue to the next