Bugfix: LegacyHidden shouldn't defer effects (#25442)

In #24967, I changed the behavior of Offscreen so that passive effects are not fired when the tree is hidden. I accidentally applied this behavior to the old LegacyHidden API, too, which is a deprecated internal-only type that www has been using while they wait for Offscreen to be ready. This fixes LegacyHidden so that the effects do not get deferred, like before. The new behavior still remains in the Offscreen API, which is experimental and not currently in use in www.
facebook · Oct 6, 2022 · 5d60a0b · 5d60a0b
1 parent e40893d
commit 5d60a0b
Show file tree

Hide file tree

Showing 3 changed files with 130 additions and 4 deletions.
diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js
@@ -52,6 +52,7 @@ import {
   enableUseEventHook,
   enableStrictEffects,
   enableFloat,
+  enableLegacyHidden,
 } from 'shared/ReactFeatureFlags';
 import {
   FunctionComponent,
@@ -3419,7 +3420,23 @@ function commitPassiveMountOnFiber(
       }
       break;
     }
-    case LegacyHiddenComponent:
+    case LegacyHiddenComponent: {
+      if (enableLegacyHidden) {
+        recursivelyTraversePassiveMountEffects(
+          finishedRoot,
+          finishedWork,
+          committedLanes,
+          committedTransitions,
+        );
+
+        if (flags & Passive) {
+          const current = finishedWork.alternate;
+          const instance: OffscreenInstance = finishedWork.stateNode;
+          commitOffscreenPassiveMountEffects(current, finishedWork, instance);
+        }
+      }
+      break;
+    }
     case OffscreenComponent: {
       // TODO: Pass `current` as argument to this function
       const instance: OffscreenInstance = finishedWork.stateNode;
@@ -3600,7 +3617,25 @@ export function reconnectPassiveEffects(
     // case HostRoot: {
     //  ...
     // }
-    case LegacyHiddenComponent:
+    case LegacyHiddenComponent: {
+      if (enableLegacyHidden) {
+        recursivelyTraverseReconnectPassiveEffects(
+          finishedRoot,
+          finishedWork,
+          committedLanes,
+          committedTransitions,
+          includeWorkInProgressEffects,
+        );
+
+        if (includeWorkInProgressEffects && flags & Passive) {
+          // TODO: Pass `current` as argument to this function
+          const current: Fiber | null = finishedWork.alternate;
+          const instance: OffscreenInstance = finishedWork.stateNode;
+          commitOffscreenPassiveMountEffects(current, finishedWork, instance);
+        }
+      }
+      break;
+    }
     case OffscreenComponent: {
       const instance: OffscreenInstance = finishedWork.stateNode;
       const nextState: OffscreenState | null = finishedWork.memoizedState;

diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.old.js b/packages/react-reconciler/src/ReactFiberCommitWork.old.js
@@ -52,6 +52,7 @@ import {
   enableUseEventHook,
   enableStrictEffects,
   enableFloat,
+  enableLegacyHidden,
 } from 'shared/ReactFeatureFlags';
 import {
   FunctionComponent,
@@ -3419,7 +3420,23 @@ function commitPassiveMountOnFiber(
       }
       break;
     }
-    case LegacyHiddenComponent:
+    case LegacyHiddenComponent: {
+      if (enableLegacyHidden) {
+        recursivelyTraversePassiveMountEffects(
+          finishedRoot,
+          finishedWork,
+          committedLanes,
+          committedTransitions,
+        );
+
+        if (flags & Passive) {
+          const current = finishedWork.alternate;
+          const instance: OffscreenInstance = finishedWork.stateNode;
+          commitOffscreenPassiveMountEffects(current, finishedWork, instance);
+        }
+      }
+      break;
+    }
     case OffscreenComponent: {
       // TODO: Pass `current` as argument to this function
       const instance: OffscreenInstance = finishedWork.stateNode;
@@ -3600,7 +3617,25 @@ export function reconnectPassiveEffects(
     // case HostRoot: {
     //  ...
     // }
-    case LegacyHiddenComponent:
+    case LegacyHiddenComponent: {
+      if (enableLegacyHidden) {
+        recursivelyTraverseReconnectPassiveEffects(
+          finishedRoot,
+          finishedWork,
+          committedLanes,
+          committedTransitions,
+          includeWorkInProgressEffects,
+        );
+
+        if (includeWorkInProgressEffects && flags & Passive) {
+          // TODO: Pass `current` as argument to this function
+          const current: Fiber | null = finishedWork.alternate;
+          const instance: OffscreenInstance = finishedWork.stateNode;
+          commitOffscreenPassiveMountEffects(current, finishedWork, instance);
+        }
+      }
+      break;
+    }
     case OffscreenComponent: {
       const instance: OffscreenInstance = finishedWork.stateNode;
       const nextState: OffscreenState | null = finishedWork.memoizedState;

diff --git a/packages/react-reconciler/src/__tests__/ReactOffscreen-test.js b/packages/react-reconciler/src/__tests__/ReactOffscreen-test.js
@@ -942,6 +942,62 @@ describe('ReactOffscreen', () => {
     ]);
   });
 
+  // @gate enableLegacyHidden
+  it('do not defer passive effects when prerendering a new LegacyHidden tree', async () => {
+    function Child({label}) {
+      useEffect(() => {
+        Scheduler.unstable_yieldValue('Mount ' + label);
+        return () => {
+          Scheduler.unstable_yieldValue('Unmount ' + label);
+        };
+      }, [label]);
+      return <Text text={label} />;
+    }
+
+    function App({showMore}) {
+      return (
+        <>
+          <Child label="Shell" />
+          <LegacyHidden
+            mode={showMore ? 'visible' : 'unstable-defer-without-hiding'}>
+            <Child label="More" />
+          </LegacyHidden>
+        </>
+      );
+    }
+
+    const root = ReactNoop.createRoot();
+
+    // Mount the app without showing the extra content
+    await act(async () => {
+      root.render(<App showMore={false} />);
+    });
+    expect(Scheduler).toHaveYielded([
+      // First mount the outer visible shell
+      'Shell',
+      'Mount Shell',
+
+      // Then prerender the hidden extra context. Unlike Offscreen, the passive
+      // effects in the hidden tree *should* fire
+      'More',
+      'Mount More',
+    ]);
+
+    // The hidden content has been prerendered
+    expect(root).toMatchRenderedOutput(
+      <>
+        <span prop="Shell" />
+        <span prop="More" />
+      </>,
+    );
+
+    // Reveal the prerendered tree
+    await act(async () => {
+      root.render(<App showMore={true} />);
+    });
+    expect(Scheduler).toHaveYielded(['Shell', 'More']);
+  });
+
   // @gate enableOffscreen
   it('passive effects are connected and disconnected when the visibility changes', async () => {
     function Child({step}) {