Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Double invoked effects on suspended children in StrictMode #25307

Merged
merged 1 commit into from
Sep 21, 2022
Merged

Double invoked effects on suspended children in StrictMode #25307

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 46 additions & 15 deletions packages/react-reconciler/src/ReactFiberWorkLoop.new.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3211,35 +3211,66 @@ function recursivelyTraverseAndDoubleInvokeEffectsInDEV(
}
let child = parentFiber.child;
while (child !== null) {
doubleInvokeEffectsInDEV(root, child, isInStrictMode);
doubleInvokeEffectsInDEVIfNecessary(root, child, isInStrictMode);
child = child.sibling;
}
}

function doubleInvokeEffectsInDEV(
// Unconditionally disconnects and connects passive and layout effects.
function doubleInvokeEffectsOnFiber(root: FiberRoot, fiber: Fiber) {
disappearLayoutEffects(fiber);
disconnectPassiveEffect(fiber);
reappearLayoutEffects(root, fiber.alternate, fiber, false);
reconnectPassiveEffects(root, fiber, NoLanes, null, false);
}

function doubleInvokeEffectsInDEVIfNecessary(
root: FiberRoot,
fiber: Fiber,
parentIsInStrictMode: boolean,
) {
const isStrictModeFiber = fiber.type === REACT_STRICT_MODE_TYPE;
const isInStrictMode = parentIsInStrictMode || isStrictModeFiber;

if (fiber.flags & PlacementDEV || fiber.tag === OffscreenComponent) {
// First case: the fiber **is not** of type OffscreenComponent. No
// special rules apply to double invoking effects.
if (fiber.tag !== OffscreenComponent) {
if (fiber.flags & PlacementDEV) {
setCurrentDebugFiberInDEV(fiber);
if (isInStrictMode) {
doubleInvokeEffectsOnFiber(root, fiber);
}
resetCurrentDebugFiberInDEV();
} else {
recursivelyTraverseAndDoubleInvokeEffectsInDEV(
root,
fiber,
isInStrictMode,
);
}
return;
}

// Second case: the fiber **is** of type OffscreenComponent.
// This branch contains cases specific to Offscreen.
if (fiber.memoizedState === null) {
// Only consider Offscreen that is visible.
// TODO (Offscreen) Handle manual mode.
setCurrentDebugFiberInDEV(fiber);
const isNotOffscreen = fiber.tag !== OffscreenComponent;
// Checks if Offscreen is being revealed. For all other components, evaluates to true.
const hasOffscreenBecomeVisible =
isNotOffscreen ||
(fiber.flags & Visibility && fiber.memoizedState === null);
if (isInStrictMode && hasOffscreenBecomeVisible) {
disappearLayoutEffects(fiber);
disconnectPassiveEffect(fiber);
reappearLayoutEffects(root, fiber.alternate, fiber, false);
reconnectPassiveEffects(root, fiber, NoLanes, null, false);
if (isInStrictMode && fiber.flags & Visibility) {
// Double invoke effects on Offscreen's subtree only
// if it is visible and its visibility has changed.
doubleInvokeEffectsOnFiber(root, fiber);
} else if (fiber.subtreeFlags & PlacementDEV) {
// Something in the subtree could have been suspended.
// We need to continue traversal and find newly inserted fibers.
recursivelyTraverseAndDoubleInvokeEffectsInDEV(
root,
fiber,
isInStrictMode,
);
}
resetCurrentDebugFiberInDEV();
} else {
recursivelyTraverseAndDoubleInvokeEffectsInDEV(root, fiber, isInStrictMode);
}
}

Expand Down
61 changes: 46 additions & 15 deletions packages/react-reconciler/src/ReactFiberWorkLoop.old.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3211,35 +3211,66 @@ function recursivelyTraverseAndDoubleInvokeEffectsInDEV(
}
let child = parentFiber.child;
while (child !== null) {
doubleInvokeEffectsInDEV(root, child, isInStrictMode);
doubleInvokeEffectsInDEVIfNecessary(root, child, isInStrictMode);
child = child.sibling;
}
}

function doubleInvokeEffectsInDEV(
// Unconditionally disconnects and connects passive and layout effects.
function doubleInvokeEffectsOnFiber(root: FiberRoot, fiber: Fiber) {
disappearLayoutEffects(fiber);
disconnectPassiveEffect(fiber);
reappearLayoutEffects(root, fiber.alternate, fiber, false);
reconnectPassiveEffects(root, fiber, NoLanes, null, false);
}

function doubleInvokeEffectsInDEVIfNecessary(
root: FiberRoot,
fiber: Fiber,
parentIsInStrictMode: boolean,
) {
const isStrictModeFiber = fiber.type === REACT_STRICT_MODE_TYPE;
const isInStrictMode = parentIsInStrictMode || isStrictModeFiber;

if (fiber.flags & PlacementDEV || fiber.tag === OffscreenComponent) {
// First case: the fiber **is not** of type OffscreenComponent. No
// special rules apply to double invoking effects.
if (fiber.tag !== OffscreenComponent) {
if (fiber.flags & PlacementDEV) {
setCurrentDebugFiberInDEV(fiber);
if (isInStrictMode) {
doubleInvokeEffectsOnFiber(root, fiber);
}
resetCurrentDebugFiberInDEV();
} else {
recursivelyTraverseAndDoubleInvokeEffectsInDEV(
root,
fiber,
isInStrictMode,
);
}
return;
}

// Second case: the fiber **is** of type OffscreenComponent.
// This branch contains cases specific to Offscreen.
if (fiber.memoizedState === null) {
// Only consider Offscreen that is visible.
// TODO (Offscreen) Handle manual mode.
setCurrentDebugFiberInDEV(fiber);
const isNotOffscreen = fiber.tag !== OffscreenComponent;
// Checks if Offscreen is being revealed. For all other components, evaluates to true.
const hasOffscreenBecomeVisible =
isNotOffscreen ||
(fiber.flags & Visibility && fiber.memoizedState === null);
if (isInStrictMode && hasOffscreenBecomeVisible) {
disappearLayoutEffects(fiber);
disconnectPassiveEffect(fiber);
reappearLayoutEffects(root, fiber.alternate, fiber, false);
reconnectPassiveEffects(root, fiber, NoLanes, null, false);
if (isInStrictMode && fiber.flags & Visibility) {
// Double invoke effects on Offscreen's subtree only
// if it is visible and its visibility has changed.
doubleInvokeEffectsOnFiber(root, fiber);
} else if (fiber.subtreeFlags & PlacementDEV) {
// Something in the subtree could have been suspended.
// We need to continue traversal and find newly inserted fibers.
recursivelyTraverseAndDoubleInvokeEffectsInDEV(
root,
fiber,
isInStrictMode,
);
}
resetCurrentDebugFiberInDEV();
} else {
recursivelyTraverseAndDoubleInvokeEffectsInDEV(root, fiber, isInStrictMode);
}
}

Expand Down
73 changes: 73 additions & 0 deletions packages/react-reconciler/src/__tests__/ReactOffscreenStrictMode-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,4 +153,77 @@ describe('ReactOffscreenStrictMode', () => {
);
});
});

// @gate __DEV__ && enableStrictEffects && enableOffscreen
it('should double invoke effects on unsuspended child', async () => {
let shouldSuspend = true;
let resolve;
const suspensePromise = new Promise(_resolve => {
resolve = _resolve;
});

function Parent() {
log.push('Parent rendered');
React.useEffect(() => {
log.push('Parent mount');
return () => {
log.push('Parent unmount');
};
});

return (
<React.Suspense fallback="fallback">
<Child />
</React.Suspense>
);
}

function Child() {
log.push('Child rendered');
React.useEffect(() => {
log.push('Child mount');
return () => {
log.push('Child unmount');
};
});
if (shouldSuspend) {
log.push('Child suspended');
throw suspensePromise;
}
return null;
}

act(() => {
ReactNoop.render(
<React.StrictMode>
<Offscreen mode="visible">
<Parent />
</Offscreen>
</React.StrictMode>,
);
});

log.push('------------------------------');

await act(async () => {
resolve();
shouldSuspend = false;
});

expect(log).toEqual([
'Parent rendered',
'Parent rendered',
'Child rendered',
'Child suspended',
'Parent mount',
'Parent unmount',
'Parent mount',
'------------------------------',
'Child rendered',
'Child rendered',
'Child mount',
'Child unmount',
'Child mount',
]);
});
});