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.
Andrew Clark committed
Oct 6, 2022 at 17:23 UTC
5d60a0b8407d57ac9ab0302ab241107ab289e561
3 files changed
+130
-4
packages/react-reconciler/src/ReactFiberCommitWork.new.js
+37
-2
@@ -52,6 +52,7 @@ import {
52
enableUseEventHook,
53
enableStrictEffects,
54
enableFloat,
55
+ enableLegacyHidden,
56
} from 'shared/ReactFeatureFlags';
57
import {
58
FunctionComponent,
@@ -3419,7 +3420,23 @@ function commitPassiveMountOnFiber(
3420
}
3421
break;
3422
}
3422
- case LegacyHiddenComponent:
3423
+ case LegacyHiddenComponent: {
3424
+ if (enableLegacyHidden) {
3425
+ recursivelyTraversePassiveMountEffects(
3426
+ finishedRoot,
3427
+ finishedWork,
3428
+ committedLanes,
3429
+ committedTransitions,
3430
+ );
3431
+
3432
+ if (flags & Passive) {
3433
+ const current = finishedWork.alternate;
3434
+ const instance: OffscreenInstance = finishedWork.stateNode;
3435
+ commitOffscreenPassiveMountEffects(current, finishedWork, instance);
3436
+ }
3437
+ }
3438
+ break;
3439
+ }
3440
case OffscreenComponent: {
3441
// TODO: Pass `current` as argument to this function
3442
const instance: OffscreenInstance = finishedWork.stateNode;
@@ -3600,7 +3617,25 @@ export function reconnectPassiveEffects(
3617
// case HostRoot: {
3618
// ...
3619
// }
3603
- case LegacyHiddenComponent:
3620
+ case LegacyHiddenComponent: {
3621
+ if (enableLegacyHidden) {
3622
+ recursivelyTraverseReconnectPassiveEffects(
3623
+ finishedRoot,
3624
+ finishedWork,
3625
+ committedLanes,
3626
+ committedTransitions,
3627
+ includeWorkInProgressEffects,
3628
+ );
3629
+
3630
+ if (includeWorkInProgressEffects && flags & Passive) {
3631
+ // TODO: Pass `current` as argument to this function
3632
+ const current: Fiber | null = finishedWork.alternate;
3633
+ const instance: OffscreenInstance = finishedWork.stateNode;
3634
+ commitOffscreenPassiveMountEffects(current, finishedWork, instance);
3635
+ }
3636
+ }
3637
+ break;
3638
+ }
3639
case OffscreenComponent: {
3640
const instance: OffscreenInstance = finishedWork.stateNode;
3641
const nextState: OffscreenState | null = finishedWork.memoizedState;
packages/react-reconciler/src/ReactFiberCommitWork.old.js
+37
-2
@@ -52,6 +52,7 @@ import {
52
enableUseEventHook,
53
enableStrictEffects,
54
enableFloat,
55
+ enableLegacyHidden,
56
} from 'shared/ReactFeatureFlags';
57
import {
58
FunctionComponent,
@@ -3419,7 +3420,23 @@ function commitPassiveMountOnFiber(
3420
}
3421
break;
3422
}
3422
- case LegacyHiddenComponent:
3423
+ case LegacyHiddenComponent: {
3424
+ if (enableLegacyHidden) {
3425
+ recursivelyTraversePassiveMountEffects(
3426
+ finishedRoot,
3427
+ finishedWork,
3428
+ committedLanes,
3429
+ committedTransitions,
3430
+ );
3431
+
3432
+ if (flags & Passive) {
3433
+ const current = finishedWork.alternate;
3434
+ const instance: OffscreenInstance = finishedWork.stateNode;
3435
+ commitOffscreenPassiveMountEffects(current, finishedWork, instance);
3436
+ }
3437
+ }
3438
+ break;
3439
+ }
3440
case OffscreenComponent: {
3441
// TODO: Pass `current` as argument to this function
3442
const instance: OffscreenInstance = finishedWork.stateNode;
@@ -3600,7 +3617,25 @@ export function reconnectPassiveEffects(
3617
// case HostRoot: {
3618
// ...
3619
// }
3603
- case LegacyHiddenComponent:
3620
+ case LegacyHiddenComponent: {
3621
+ if (enableLegacyHidden) {
3622
+ recursivelyTraverseReconnectPassiveEffects(
3623
+ finishedRoot,
3624
+ finishedWork,
3625
+ committedLanes,
3626
+ committedTransitions,
3627
+ includeWorkInProgressEffects,
3628
+ );
3629
+
3630
+ if (includeWorkInProgressEffects && flags & Passive) {
3631
+ // TODO: Pass `current` as argument to this function
3632
+ const current: Fiber | null = finishedWork.alternate;
3633
+ const instance: OffscreenInstance = finishedWork.stateNode;
3634
+ commitOffscreenPassiveMountEffects(current, finishedWork, instance);
3635
+ }
3636
+ }
3637
+ break;
3638
+ }
3639
case OffscreenComponent: {
3640
const instance: OffscreenInstance = finishedWork.stateNode;
3641
const nextState: OffscreenState | null = finishedWork.memoizedState;
packages/react-reconciler/src/__tests__/ReactOffscreen-test.js
+56
@@ -942,6 +942,62 @@ describe('ReactOffscreen', () => {
942
]);
943
});
944
945
+ // @gate enableLegacyHidden
946
+ it('do not defer passive effects when prerendering a new LegacyHidden tree', async () => {
947
+ function Child({label}) {
948
+ useEffect(() => {
949
+ Scheduler.unstable_yieldValue('Mount ' + label);
950
+ return () => {
951
+ Scheduler.unstable_yieldValue('Unmount ' + label);
952
+ };
953
+ }, [label]);
954
+ return <Text text={label} />;
955
+ }
956
+
957
+ function App({showMore}) {
958
+ return (
959
+ <>
960
+ <Child label="Shell" />
961
+ <LegacyHidden
962
+ mode={showMore ? 'visible' : 'unstable-defer-without-hiding'}>
963
+ <Child label="More" />
964
+ </LegacyHidden>
965
+ </>
966
+ );
967
+ }
968
+
969
+ const root = ReactNoop.createRoot();
970
+
971
+ // Mount the app without showing the extra content
972
+ await act(async () => {
973
+ root.render(<App showMore={false} />);
974
+ });
975
+ expect(Scheduler).toHaveYielded([
976
+ // First mount the outer visible shell
977
+ 'Shell',
978
+ 'Mount Shell',
979
+
980
+ // Then prerender the hidden extra context. Unlike Offscreen, the passive
981
+ // effects in the hidden tree *should* fire
982
+ 'More',
983
+ 'Mount More',
984
+ ]);
985
+
986
+ // The hidden content has been prerendered
987
+ expect(root).toMatchRenderedOutput(
988
+ <>
989
+ <span prop="Shell" />
990
+ <span prop="More" />
991
+ </>,
992
+ );
993
+
994
+ // Reveal the prerendered tree
995
+ await act(async () => {
996
+ root.render(<App showMore={true} />);
997
+ });
998
+ expect(Scheduler).toHaveYielded(['Shell', 'More']);
999
+ });
1000
+
1001
// @gate enableOffscreen
1002
it('passive effects are connected and disconnected when the visibility changes', async () => {
1003
function Child({step}) {