@samitouri / QOS-React-2 / commits / ae5d26154b

Fix: LegacyHidden should not toggle effects (#21928)

LegacyHidden is a transitional API that we added to replace the old `<div hidden={true} />` API that we used to use for pre-rendering. The plan is to replace this with the Offscreen component, once it's ready. The idea is that LegacyHidden has identical behavior to Offscreen except that it doesn't change the behavior of effects. (Which is basically how `<div hidden={true} />` worked — it prerendered the hidden content in the background, but nothing else.) That way, while we're rolling this out, we could toggle the feature behind a feature flag either for performance testing or as a kill switch. It looks like we accidentally enabled the effects flag for both Offscreen _and_ LegacyHidden. I suppose it's a good thing that nobody has complained yet, since we eventually do want to ship this behavior everywhere? But I do think we should remove it from LegacyHidden, and roll it out by gating the component type in the downstream repo. That way if there's an issue related to the use of LegacyHidden, we can disable that without disabling the behavior for Suspense boundaries. In retrospect, I might have implemented this as an unstable prop on Offscreen instead of a completely separate type — though at the time, Offscreen didn't exist. I originally added LegacyHidden to unblock the Lanes refactor, so I could move the deprioritization logic out of the HostComponent implementation. Not a big deal since we're going to remove this soon. The implementation is almost the same regardless: before disconnecting or reconnecting the effects, check the fiber tag. The rest of the logic is the same.

Andrew Clark committed Jul 21, 2021 at 13:34 UTC ae5d26154b1e206774533a8313221d309f273090
5 files changed +88 -40
packages/react-reconciler/src/ReactFiberCommitWork.new.js
+17 -19
@@ -2131,8 +2131,7 @@ function commitMutationEffectsOnFiber(finishedWork: Fiber, root: FiberRoot) {
2131 }
2132 break;
2133 }
2134 - case OffscreenComponent:
2135 - case LegacyHiddenComponent: {
2134 + case OffscreenComponent: {
2135 const newState: OffscreenState | null = finishedWork.memoizedState;
2136 const isHidden = newState !== null;
2137 const current = finishedWork.alternate;
@@ -2145,27 +2144,26 @@ function commitMutationEffectsOnFiber(finishedWork: Fiber, root: FiberRoot) {
2144 hideOrUnhideAllChildren(offscreenBoundary, isHidden);
2145 }
2146
2148 - if (isHidden) {
2149 - if (!wasHidden) {
2150 - if (
2151 - enableSuspenseLayoutEffectSemantics &&
2152 - (offscreenBoundary.mode & ConcurrentMode) !== NoMode
2153 - ) {
2154 - nextEffect = offscreenBoundary;
2155 - let offscreenChild = offscreenBoundary.child;
2156 - while (offscreenChild !== null) {
2157 - nextEffect = offscreenChild;
2158 - disappearLayoutEffects_begin(offscreenChild);
2159 - offscreenChild = offscreenChild.sibling;
2147 + if (enableSuspenseLayoutEffectSemantics) {
2148 + if (isHidden) {
2149 + if (!wasHidden) {
2150 + if ((offscreenBoundary.mode & ConcurrentMode) !== NoMode) {
2151 + nextEffect = offscreenBoundary;
2152 + let offscreenChild = offscreenBoundary.child;
2153 + while (offscreenChild !== null) {
2154 + nextEffect = offscreenChild;
2155 + disappearLayoutEffects_begin(offscreenChild);
2156 + offscreenChild = offscreenChild.sibling;
2157 + }
2158 }
2159 }
2160 + } else {
2161 + if (wasHidden) {
2162 + // TODO: Move re-appear call here for symmetry?
2163 + }
2164 }
2163 - } else {
2164 - if (wasHidden) {
2165 - // TODO: Move re-appear call here for symmetry?
2166 - }
2165 + break;
2166 }
2168 - break;
2167 }
2168 }
2169 }
packages/react-reconciler/src/ReactFiberCommitWork.old.js
+17 -19
@@ -2131,8 +2131,7 @@ function commitMutationEffectsOnFiber(finishedWork: Fiber, root: FiberRoot) {
2131 }
2132 break;
2133 }
2134 - case OffscreenComponent:
2135 - case LegacyHiddenComponent: {
2134 + case OffscreenComponent: {
2135 const newState: OffscreenState | null = finishedWork.memoizedState;
2136 const isHidden = newState !== null;
2137 const current = finishedWork.alternate;
@@ -2145,27 +2144,26 @@ function commitMutationEffectsOnFiber(finishedWork: Fiber, root: FiberRoot) {
2144 hideOrUnhideAllChildren(offscreenBoundary, isHidden);
2145 }
2146
2148 - if (isHidden) {
2149 - if (!wasHidden) {
2150 - if (
2151 - enableSuspenseLayoutEffectSemantics &&
2152 - (offscreenBoundary.mode & ConcurrentMode) !== NoMode
2153 - ) {
2154 - nextEffect = offscreenBoundary;
2155 - let offscreenChild = offscreenBoundary.child;
2156 - while (offscreenChild !== null) {
2157 - nextEffect = offscreenChild;
2158 - disappearLayoutEffects_begin(offscreenChild);
2159 - offscreenChild = offscreenChild.sibling;
2147 + if (enableSuspenseLayoutEffectSemantics) {
2148 + if (isHidden) {
2149 + if (!wasHidden) {
2150 + if ((offscreenBoundary.mode & ConcurrentMode) !== NoMode) {
2151 + nextEffect = offscreenBoundary;
2152 + let offscreenChild = offscreenBoundary.child;
2153 + while (offscreenChild !== null) {
2154 + nextEffect = offscreenChild;
2155 + disappearLayoutEffects_begin(offscreenChild);
2156 + offscreenChild = offscreenChild.sibling;
2157 + }
2158 }
2159 }
2160 + } else {
2161 + if (wasHidden) {
2162 + // TODO: Move re-appear call here for symmetry?
2163 + }
2164 }
2163 - } else {
2164 - if (wasHidden) {
2165 - // TODO: Move re-appear call here for symmetry?
2166 - }
2165 + break;
2166 }
2168 - break;
2167 }
2168 }
2169 }
packages/react-reconciler/src/ReactFiberCompleteWork.new.js
+3 -1
@@ -1361,7 +1361,9 @@ function completeWork(
1361 const prevIsHidden = prevState !== null;
1362 if (
1363 prevIsHidden !== nextIsHidden &&
1364 - newProps.mode !== 'unstable-defer-without-hiding'
1364 + newProps.mode !== 'unstable-defer-without-hiding' &&
1365 + // LegacyHidden doesn't do any hiding — it only pre-renders.
1366 + workInProgress.tag !== LegacyHiddenComponent
1367 ) {
1368 workInProgress.flags |= Visibility;
1369 }
packages/react-reconciler/src/ReactFiberCompleteWork.old.js
+3 -1
@@ -1361,7 +1361,9 @@ function completeWork(
1361 const prevIsHidden = prevState !== null;
1362 if (
1363 prevIsHidden !== nextIsHidden &&
1364 - newProps.mode !== 'unstable-defer-without-hiding'
1364 + newProps.mode !== 'unstable-defer-without-hiding' &&
1365 + // LegacyHidden doesn't do any hiding — it only pre-renders.
1366 + workInProgress.tag !== LegacyHiddenComponent
1367 ) {
1368 workInProgress.flags |= Visibility;
1369 }
packages/react-reconciler/src/__tests__/ReactOffscreen-test.js
+48
@@ -337,4 +337,52 @@ describe('ReactOffscreen', () => {
337 expect(root).toMatchRenderedOutput(<span prop="Child" />);
338 }
339 });
340 +
341 + // @gate experimental || www
342 + it('does not toggle effects for LegacyHidden component', async () => {
343 + // LegacyHidden is meant to be the same as offscreen except it doesn't
344 + // do anything to effects. Only used by www, as a temporary migration step.
345 + function Child({text}) {
346 + useLayoutEffect(() => {
347 + Scheduler.unstable_yieldValue('Mount layout');
348 + return () => {
349 + Scheduler.unstable_yieldValue('Unmount layout');
350 + };
351 + }, []);
352 + return <Text text="Child" />;
353 + }
354 +
355 + const root = ReactNoop.createRoot();
356 + await act(async () => {
357 + root.render(
358 + <LegacyHidden mode="visible">
359 + <Child />
360 + </LegacyHidden>,
361 + );
362 + });
363 + expect(Scheduler).toHaveYielded(['Child', 'Mount layout']);
364 +
365 + await act(async () => {
366 + root.render(
367 + <LegacyHidden mode="hidden">
368 + <Child />
369 + </LegacyHidden>,
370 + );
371 + });
372 + expect(Scheduler).toHaveYielded(['Child']);
373 +
374 + await act(async () => {
375 + root.render(
376 + <LegacyHidden mode="visible">
377 + <Child />
378 + </LegacyHidden>,
379 + );
380 + });
381 + expect(Scheduler).toHaveYielded(['Child']);
382 +
383 + await act(async () => {
384 + root.render(null);
385 + });
386 + expect(Scheduler).toHaveYielded(['Unmount layout']);
387 + });
388 });