From ea7b2ec2898c615f648aec30fcbcf73aed156583 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 6 Apr 2022 23:01:07 -0400 Subject: [PATCH 01/10] Remove wrong return pointer warning I'm about to refactor part of the commit phase to use recursion instead of iteration. As part of that change, we will no longer assign the `return` pointer when traversing into a subtree. So I'm disabling the internal warning that fires if the return pointer is not consistent with the parent during the commit phase. I had originally added this warning to help prevent mistakes when traversing the tree iteratively, but since we're intentionally switching to recursion instead, we don't need it. --- .../__tests__/ReactWrongReturnPointer-test.js | 45 ------------------- .../src/ReactFiberCommitWork.new.js | 41 +++++------------ .../src/ReactFiberCommitWork.old.js | 41 +++++------------ 3 files changed, 24 insertions(+), 103 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactWrongReturnPointer-test.js b/packages/react-dom/src/__tests__/ReactWrongReturnPointer-test.js index ca5c9fac51624..f4303d68c0f4b 100644 --- a/packages/react-dom/src/__tests__/ReactWrongReturnPointer-test.js +++ b/packages/react-dom/src/__tests__/ReactWrongReturnPointer-test.js @@ -153,51 +153,6 @@ function resolveMostRecentTextCache(text) { const resolveText = resolveMostRecentTextCache; -// Don't feel too guilty if you have to delete this test. -// @gate dfsEffectsRefactor -// @gate __DEV__ -test('warns in DEV if return pointer is inconsistent', async () => { - const {useRef, useLayoutEffect} = React; - - let ref = null; - function App({text}) { - ref = useRef(null); - return ( - <> - -
{text}
- - ); - } - - function Sibling({text}) { - useLayoutEffect(() => { - if (text === 'B') { - // Mutate the return pointer of the div to point to the wrong alternate. - // This simulates the most common type of return pointer inconsistency. - const current = ref.current.fiber; - const workInProgress = current.alternate; - workInProgress.return = current.return; - } - }, [text]); - return null; - } - - const root = ReactNoop.createRoot(); - await act(async () => { - root.render(); - }); - - spyOnDev(console, 'error'); - await act(async () => { - root.render(); - }); - expect(console.error.calls.count()).toBe(1); - expect(console.error.calls.argsFor(0)[0]).toMatch( - 'Internal React error: Return pointer is inconsistent with parent.', - ); -}); - // @gate enableCache // @gate enableSuspenseList test('regression (#20932): return pointer is correct before entering deleted tree', async () => { diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index 6d3b0e555a64f..fa9e70c3923ae 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js @@ -360,7 +360,7 @@ function commitBeforeMutationEffects_begin() { (fiber.subtreeFlags & BeforeMutationMask) !== NoFlags && child !== null ) { - ensureCorrectReturnPointer(child, fiber); + child.return = fiber; nextEffect = child; } else { commitBeforeMutationEffects_complete(); @@ -382,7 +382,7 @@ function commitBeforeMutationEffects_complete() { const sibling = fiber.sibling; if (sibling !== null) { - ensureCorrectReturnPointer(sibling, fiber.return); + sibling.return = fiber.return; nextEffect = sibling; return; } @@ -2185,7 +2185,7 @@ function commitMutationEffects_begin(root: FiberRoot, lanes: Lanes) { const child = fiber.child; if ((fiber.subtreeFlags & MutationMask) !== NoFlags && child !== null) { - ensureCorrectReturnPointer(child, fiber); + child.return = fiber; nextEffect = child; } else { commitMutationEffects_complete(root, lanes); @@ -2207,7 +2207,7 @@ function commitMutationEffects_complete(root: FiberRoot, lanes: Lanes) { const sibling = fiber.sibling; if (sibling !== null) { - ensureCorrectReturnPointer(sibling, fiber.return); + sibling.return = fiber.return; nextEffect = sibling; return; } @@ -2425,7 +2425,7 @@ function commitLayoutEffects_begin( } if ((fiber.subtreeFlags & LayoutMask) !== NoFlags && firstChild !== null) { - ensureCorrectReturnPointer(firstChild, fiber); + firstChild.return = fiber; nextEffect = firstChild; } else { commitLayoutMountEffects_complete(subtreeRoot, root, committedLanes); @@ -2459,7 +2459,7 @@ function commitLayoutMountEffects_complete( const sibling = fiber.sibling; if (sibling !== null) { - ensureCorrectReturnPointer(sibling, fiber.return); + sibling.return = fiber.return; nextEffect = sibling; return; } @@ -2628,7 +2628,7 @@ function commitPassiveMountEffects_begin( const fiber = nextEffect; const firstChild = fiber.child; if ((fiber.subtreeFlags & PassiveMask) !== NoFlags && firstChild !== null) { - ensureCorrectReturnPointer(firstChild, fiber); + firstChild.return = fiber; nextEffect = firstChild; } else { commitPassiveMountEffects_complete(subtreeRoot, root, committedLanes); @@ -2662,7 +2662,7 @@ function commitPassiveMountEffects_complete( const sibling = fiber.sibling; if (sibling !== null) { - ensureCorrectReturnPointer(sibling, fiber.return); + sibling.return = fiber.return; nextEffect = sibling; return; } @@ -2849,7 +2849,7 @@ function commitPassiveUnmountEffects_begin() { } if ((fiber.subtreeFlags & PassiveMask) !== NoFlags && child !== null) { - ensureCorrectReturnPointer(child, fiber); + child.return = fiber; nextEffect = child; } else { commitPassiveUnmountEffects_complete(); @@ -2868,7 +2868,7 @@ function commitPassiveUnmountEffects_complete() { const sibling = fiber.sibling; if (sibling !== null) { - ensureCorrectReturnPointer(sibling, fiber.return); + sibling.return = fiber.return; nextEffect = sibling; return; } @@ -2923,7 +2923,7 @@ function commitPassiveUnmountEffectsInsideOfDeletedTree_begin( // TODO: Only traverse subtree if it has a PassiveStatic flag. (But, if we // do this, still need to handle `deletedTreeCleanUpLevel` correctly.) if (child !== null) { - ensureCorrectReturnPointer(child, fiber); + child.return = fiber; nextEffect = child; } else { commitPassiveUnmountEffectsInsideOfDeletedTree_complete( @@ -2961,7 +2961,7 @@ function commitPassiveUnmountEffectsInsideOfDeletedTree_complete( } if (sibling !== null) { - ensureCorrectReturnPointer(sibling, returnFiber); + sibling.return = returnFiber; nextEffect = sibling; return; } @@ -3039,23 +3039,6 @@ function commitPassiveUnmountInsideDeletedTreeOnFiber( } } -let didWarnWrongReturnPointer = false; -function ensureCorrectReturnPointer(fiber, expectedReturnFiber) { - if (__DEV__) { - if (!didWarnWrongReturnPointer && fiber.return !== expectedReturnFiber) { - didWarnWrongReturnPointer = true; - console.error( - 'Internal React error: Return pointer is inconsistent ' + - 'with parent.', - ); - } - } - - // TODO: Remove this assignment once we're confident that it won't break - // anything, by checking the warning logs for the above invariant - fiber.return = expectedReturnFiber; -} - // TODO: Reuse reappearLayoutEffects traversal here? function invokeLayoutEffectMountInDEV(fiber: Fiber): void { if (__DEV__ && enableStrictEffects) { diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.old.js b/packages/react-reconciler/src/ReactFiberCommitWork.old.js index 9a8ba5bcd6d57..df30a1bc26f50 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.old.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.old.js @@ -360,7 +360,7 @@ function commitBeforeMutationEffects_begin() { (fiber.subtreeFlags & BeforeMutationMask) !== NoFlags && child !== null ) { - ensureCorrectReturnPointer(child, fiber); + child.return = fiber; nextEffect = child; } else { commitBeforeMutationEffects_complete(); @@ -382,7 +382,7 @@ function commitBeforeMutationEffects_complete() { const sibling = fiber.sibling; if (sibling !== null) { - ensureCorrectReturnPointer(sibling, fiber.return); + sibling.return = fiber.return; nextEffect = sibling; return; } @@ -2185,7 +2185,7 @@ function commitMutationEffects_begin(root: FiberRoot, lanes: Lanes) { const child = fiber.child; if ((fiber.subtreeFlags & MutationMask) !== NoFlags && child !== null) { - ensureCorrectReturnPointer(child, fiber); + child.return = fiber; nextEffect = child; } else { commitMutationEffects_complete(root, lanes); @@ -2207,7 +2207,7 @@ function commitMutationEffects_complete(root: FiberRoot, lanes: Lanes) { const sibling = fiber.sibling; if (sibling !== null) { - ensureCorrectReturnPointer(sibling, fiber.return); + sibling.return = fiber.return; nextEffect = sibling; return; } @@ -2425,7 +2425,7 @@ function commitLayoutEffects_begin( } if ((fiber.subtreeFlags & LayoutMask) !== NoFlags && firstChild !== null) { - ensureCorrectReturnPointer(firstChild, fiber); + firstChild.return = fiber; nextEffect = firstChild; } else { commitLayoutMountEffects_complete(subtreeRoot, root, committedLanes); @@ -2459,7 +2459,7 @@ function commitLayoutMountEffects_complete( const sibling = fiber.sibling; if (sibling !== null) { - ensureCorrectReturnPointer(sibling, fiber.return); + sibling.return = fiber.return; nextEffect = sibling; return; } @@ -2628,7 +2628,7 @@ function commitPassiveMountEffects_begin( const fiber = nextEffect; const firstChild = fiber.child; if ((fiber.subtreeFlags & PassiveMask) !== NoFlags && firstChild !== null) { - ensureCorrectReturnPointer(firstChild, fiber); + firstChild.return = fiber; nextEffect = firstChild; } else { commitPassiveMountEffects_complete(subtreeRoot, root, committedLanes); @@ -2662,7 +2662,7 @@ function commitPassiveMountEffects_complete( const sibling = fiber.sibling; if (sibling !== null) { - ensureCorrectReturnPointer(sibling, fiber.return); + sibling.return = fiber.return; nextEffect = sibling; return; } @@ -2849,7 +2849,7 @@ function commitPassiveUnmountEffects_begin() { } if ((fiber.subtreeFlags & PassiveMask) !== NoFlags && child !== null) { - ensureCorrectReturnPointer(child, fiber); + child.return = fiber; nextEffect = child; } else { commitPassiveUnmountEffects_complete(); @@ -2868,7 +2868,7 @@ function commitPassiveUnmountEffects_complete() { const sibling = fiber.sibling; if (sibling !== null) { - ensureCorrectReturnPointer(sibling, fiber.return); + sibling.return = fiber.return; nextEffect = sibling; return; } @@ -2923,7 +2923,7 @@ function commitPassiveUnmountEffectsInsideOfDeletedTree_begin( // TODO: Only traverse subtree if it has a PassiveStatic flag. (But, if we // do this, still need to handle `deletedTreeCleanUpLevel` correctly.) if (child !== null) { - ensureCorrectReturnPointer(child, fiber); + child.return = fiber; nextEffect = child; } else { commitPassiveUnmountEffectsInsideOfDeletedTree_complete( @@ -2961,7 +2961,7 @@ function commitPassiveUnmountEffectsInsideOfDeletedTree_complete( } if (sibling !== null) { - ensureCorrectReturnPointer(sibling, returnFiber); + sibling.return = returnFiber; nextEffect = sibling; return; } @@ -3039,23 +3039,6 @@ function commitPassiveUnmountInsideDeletedTreeOnFiber( } } -let didWarnWrongReturnPointer = false; -function ensureCorrectReturnPointer(fiber, expectedReturnFiber) { - if (__DEV__) { - if (!didWarnWrongReturnPointer && fiber.return !== expectedReturnFiber) { - didWarnWrongReturnPointer = true; - console.error( - 'Internal React error: Return pointer is inconsistent ' + - 'with parent.', - ); - } - } - - // TODO: Remove this assignment once we're confident that it won't break - // anything, by checking the warning logs for the above invariant - fiber.return = expectedReturnFiber; -} - // TODO: Reuse reappearLayoutEffects traversal here? function invokeLayoutEffectMountInDEV(fiber: Fiber): void { if (__DEV__ && enableStrictEffects) { From 12d7a9ad7082106102b7dd9fec4c75abafe1c2a8 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Thu, 7 Apr 2022 19:00:20 -0400 Subject: [PATCH 02/10] Combine commitWork into single switch statement commitWork is forked into a separate implementation for mutation mode (DOM) and persistent mode (React Native). But unlike when it was first introduced, there's more overlap than differences between the forks, mainly because we've added new types of fibers. So this joins the two forks and adds more local branches where the behavior actually diverges: host nodes, host containers, and portals. --- .../src/ReactFiberCommitWork.new.js | 202 +++++------------- .../src/ReactFiberCommitWork.old.js | 202 +++++------------- 2 files changed, 110 insertions(+), 294 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index fa9e70c3923ae..337f3e50b2316 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js @@ -1481,36 +1481,6 @@ function emptyPortalContainer(current: Fiber) { replaceContainerChildren(containerInfo, emptyChildSet); } -function commitContainer(finishedWork: Fiber) { - if (!supportsPersistence) { - return; - } - - switch (finishedWork.tag) { - case ClassComponent: - case HostComponent: - case HostText: { - return; - } - case HostRoot: - case HostPortal: { - const portalOrRoot: { - containerInfo: Container, - pendingChildren: ChildSet, - ... - } = finishedWork.stateNode; - const {containerInfo, pendingChildren} = portalOrRoot; - replaceContainerChildren(containerInfo, pendingChildren); - return; - } - } - - throw new Error( - 'This unit of work tag should not have side-effects. This error is ' + - 'likely caused by a bug in React. Please file an issue.', - ); -} - function getHostParentFiber(fiber: Fiber): Fiber { let parent = fiber.return; while (parent !== null) { @@ -1830,87 +1800,6 @@ function commitDeletion( } function commitWork(current: Fiber | null, finishedWork: Fiber): void { - if (!supportsMutation) { - switch (finishedWork.tag) { - case FunctionComponent: - case ForwardRef: - case MemoComponent: - case SimpleMemoComponent: { - commitHookEffectListUnmount( - HookInsertion | HookHasEffect, - finishedWork, - finishedWork.return, - ); - commitHookEffectListMount(HookInsertion | HookHasEffect, finishedWork); - - // Layout effects are destroyed during the mutation phase so that all - // destroy functions for all fibers are called before any create functions. - // This prevents sibling component effects from interfering with each other, - // e.g. a destroy function in one component should never override a ref set - // by a create function in another component during the same commit. - // TODO: Check if we're inside an Offscreen subtree that disappeared - // during this commit. If so, we would have already unmounted its - // layout hooks. (However, since we null out the `destroy` function - // right before calling it, the behavior is already correct, so this - // would mostly be for modeling purposes.) - if ( - enableProfilerTimer && - enableProfilerCommitHooks && - finishedWork.mode & ProfileMode - ) { - try { - startLayoutEffectTimer(); - commitHookEffectListUnmount( - HookLayout | HookHasEffect, - finishedWork, - finishedWork.return, - ); - } finally { - recordLayoutEffectDuration(finishedWork); - } - } else { - commitHookEffectListUnmount( - HookLayout | HookHasEffect, - finishedWork, - finishedWork.return, - ); - } - return; - } - case Profiler: { - return; - } - case SuspenseComponent: { - commitSuspenseCallback(finishedWork); - attachSuspenseRetryListeners(finishedWork); - return; - } - case SuspenseListComponent: { - attachSuspenseRetryListeners(finishedWork); - return; - } - case HostRoot: { - if (supportsHydration) { - if (current !== null) { - const prevRootState: RootState = current.memoizedState; - if (prevRootState.isDehydrated) { - const root: FiberRoot = finishedWork.stateNode; - commitHydratedContainer(root.containerInfo); - } - } - } - break; - } - case OffscreenComponent: - case LegacyHiddenComponent: { - return; - } - } - - commitContainer(finishedWork); - return; - } - switch (finishedWork.tag) { case FunctionComponent: case ForwardRef: @@ -1955,51 +1844,55 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void { return; } case HostComponent: { - const instance: Instance = finishedWork.stateNode; - if (instance != null) { - // Commit the work prepared earlier. - const newProps = finishedWork.memoizedProps; - // For hydration we reuse the update path but we treat the oldProps - // as the newProps. The updatePayload will contain the real change in - // this case. - const oldProps = current !== null ? current.memoizedProps : newProps; - const type = finishedWork.type; - // TODO: Type the updateQueue to be specific to host components. - const updatePayload: null | UpdatePayload = (finishedWork.updateQueue: any); - finishedWork.updateQueue = null; - if (updatePayload !== null) { - commitUpdate( - instance, - updatePayload, - type, - oldProps, - newProps, - finishedWork, - ); + if (supportsMutation) { + const instance: Instance = finishedWork.stateNode; + if (instance != null) { + // Commit the work prepared earlier. + const newProps = finishedWork.memoizedProps; + // For hydration we reuse the update path but we treat the oldProps + // as the newProps. The updatePayload will contain the real change in + // this case. + const oldProps = current !== null ? current.memoizedProps : newProps; + const type = finishedWork.type; + // TODO: Type the updateQueue to be specific to host components. + const updatePayload: null | UpdatePayload = (finishedWork.updateQueue: any); + finishedWork.updateQueue = null; + if (updatePayload !== null) { + commitUpdate( + instance, + updatePayload, + type, + oldProps, + newProps, + finishedWork, + ); + } } } return; } case HostText: { - if (finishedWork.stateNode === null) { - throw new Error( - 'This should have a text node initialized. This error is likely ' + - 'caused by a bug in React. Please file an issue.', - ); - } + if (supportsMutation) { + if (finishedWork.stateNode === null) { + throw new Error( + 'This should have a text node initialized. This error is likely ' + + 'caused by a bug in React. Please file an issue.', + ); + } - const textInstance: TextInstance = finishedWork.stateNode; - const newText: string = finishedWork.memoizedProps; - // For hydration we reuse the update path but we treat the oldProps - // as the newProps. The updatePayload will contain the real change in - // this case. - const oldText: string = - current !== null ? current.memoizedProps : newText; - commitTextUpdate(textInstance, oldText, newText); + const textInstance: TextInstance = finishedWork.stateNode; + const newText: string = finishedWork.memoizedProps; + // For hydration we reuse the update path but we treat the oldProps + // as the newProps. The updatePayload will contain the real change in + // this case. + const oldText: string = + current !== null ? current.memoizedProps : newText; + commitTextUpdate(textInstance, oldText, newText); + } return; } case HostRoot: { - if (supportsHydration) { + if (supportsMutation && supportsHydration) { if (current !== null) { const prevRootState: RootState = current.memoizedState; if (prevRootState.isDehydrated) { @@ -2008,6 +1901,21 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void { } } } + if (supportsPersistence) { + const fiberRoot: FiberRoot = finishedWork.stateNode; + const containerInfo = fiberRoot.containerInfo; + const pendingChildren = fiberRoot.pendingChildren; + replaceContainerChildren(containerInfo, pendingChildren); + } + return; + } + case HostPortal: { + if (supportsPersistence) { + const portal = finishedWork.stateNode; + const containerInfo = portal.containerInfo; + const pendingChildren = portal.pendingChildren; + replaceContainerChildren(containerInfo, pendingChildren); + } return; } case Profiler: { diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.old.js b/packages/react-reconciler/src/ReactFiberCommitWork.old.js index df30a1bc26f50..cd250eadfd130 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.old.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.old.js @@ -1481,36 +1481,6 @@ function emptyPortalContainer(current: Fiber) { replaceContainerChildren(containerInfo, emptyChildSet); } -function commitContainer(finishedWork: Fiber) { - if (!supportsPersistence) { - return; - } - - switch (finishedWork.tag) { - case ClassComponent: - case HostComponent: - case HostText: { - return; - } - case HostRoot: - case HostPortal: { - const portalOrRoot: { - containerInfo: Container, - pendingChildren: ChildSet, - ... - } = finishedWork.stateNode; - const {containerInfo, pendingChildren} = portalOrRoot; - replaceContainerChildren(containerInfo, pendingChildren); - return; - } - } - - throw new Error( - 'This unit of work tag should not have side-effects. This error is ' + - 'likely caused by a bug in React. Please file an issue.', - ); -} - function getHostParentFiber(fiber: Fiber): Fiber { let parent = fiber.return; while (parent !== null) { @@ -1830,87 +1800,6 @@ function commitDeletion( } function commitWork(current: Fiber | null, finishedWork: Fiber): void { - if (!supportsMutation) { - switch (finishedWork.tag) { - case FunctionComponent: - case ForwardRef: - case MemoComponent: - case SimpleMemoComponent: { - commitHookEffectListUnmount( - HookInsertion | HookHasEffect, - finishedWork, - finishedWork.return, - ); - commitHookEffectListMount(HookInsertion | HookHasEffect, finishedWork); - - // Layout effects are destroyed during the mutation phase so that all - // destroy functions for all fibers are called before any create functions. - // This prevents sibling component effects from interfering with each other, - // e.g. a destroy function in one component should never override a ref set - // by a create function in another component during the same commit. - // TODO: Check if we're inside an Offscreen subtree that disappeared - // during this commit. If so, we would have already unmounted its - // layout hooks. (However, since we null out the `destroy` function - // right before calling it, the behavior is already correct, so this - // would mostly be for modeling purposes.) - if ( - enableProfilerTimer && - enableProfilerCommitHooks && - finishedWork.mode & ProfileMode - ) { - try { - startLayoutEffectTimer(); - commitHookEffectListUnmount( - HookLayout | HookHasEffect, - finishedWork, - finishedWork.return, - ); - } finally { - recordLayoutEffectDuration(finishedWork); - } - } else { - commitHookEffectListUnmount( - HookLayout | HookHasEffect, - finishedWork, - finishedWork.return, - ); - } - return; - } - case Profiler: { - return; - } - case SuspenseComponent: { - commitSuspenseCallback(finishedWork); - attachSuspenseRetryListeners(finishedWork); - return; - } - case SuspenseListComponent: { - attachSuspenseRetryListeners(finishedWork); - return; - } - case HostRoot: { - if (supportsHydration) { - if (current !== null) { - const prevRootState: RootState = current.memoizedState; - if (prevRootState.isDehydrated) { - const root: FiberRoot = finishedWork.stateNode; - commitHydratedContainer(root.containerInfo); - } - } - } - break; - } - case OffscreenComponent: - case LegacyHiddenComponent: { - return; - } - } - - commitContainer(finishedWork); - return; - } - switch (finishedWork.tag) { case FunctionComponent: case ForwardRef: @@ -1955,51 +1844,55 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void { return; } case HostComponent: { - const instance: Instance = finishedWork.stateNode; - if (instance != null) { - // Commit the work prepared earlier. - const newProps = finishedWork.memoizedProps; - // For hydration we reuse the update path but we treat the oldProps - // as the newProps. The updatePayload will contain the real change in - // this case. - const oldProps = current !== null ? current.memoizedProps : newProps; - const type = finishedWork.type; - // TODO: Type the updateQueue to be specific to host components. - const updatePayload: null | UpdatePayload = (finishedWork.updateQueue: any); - finishedWork.updateQueue = null; - if (updatePayload !== null) { - commitUpdate( - instance, - updatePayload, - type, - oldProps, - newProps, - finishedWork, - ); + if (supportsMutation) { + const instance: Instance = finishedWork.stateNode; + if (instance != null) { + // Commit the work prepared earlier. + const newProps = finishedWork.memoizedProps; + // For hydration we reuse the update path but we treat the oldProps + // as the newProps. The updatePayload will contain the real change in + // this case. + const oldProps = current !== null ? current.memoizedProps : newProps; + const type = finishedWork.type; + // TODO: Type the updateQueue to be specific to host components. + const updatePayload: null | UpdatePayload = (finishedWork.updateQueue: any); + finishedWork.updateQueue = null; + if (updatePayload !== null) { + commitUpdate( + instance, + updatePayload, + type, + oldProps, + newProps, + finishedWork, + ); + } } } return; } case HostText: { - if (finishedWork.stateNode === null) { - throw new Error( - 'This should have a text node initialized. This error is likely ' + - 'caused by a bug in React. Please file an issue.', - ); - } + if (supportsMutation) { + if (finishedWork.stateNode === null) { + throw new Error( + 'This should have a text node initialized. This error is likely ' + + 'caused by a bug in React. Please file an issue.', + ); + } - const textInstance: TextInstance = finishedWork.stateNode; - const newText: string = finishedWork.memoizedProps; - // For hydration we reuse the update path but we treat the oldProps - // as the newProps. The updatePayload will contain the real change in - // this case. - const oldText: string = - current !== null ? current.memoizedProps : newText; - commitTextUpdate(textInstance, oldText, newText); + const textInstance: TextInstance = finishedWork.stateNode; + const newText: string = finishedWork.memoizedProps; + // For hydration we reuse the update path but we treat the oldProps + // as the newProps. The updatePayload will contain the real change in + // this case. + const oldText: string = + current !== null ? current.memoizedProps : newText; + commitTextUpdate(textInstance, oldText, newText); + } return; } case HostRoot: { - if (supportsHydration) { + if (supportsMutation && supportsHydration) { if (current !== null) { const prevRootState: RootState = current.memoizedState; if (prevRootState.isDehydrated) { @@ -2008,6 +1901,21 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void { } } } + if (supportsPersistence) { + const fiberRoot: FiberRoot = finishedWork.stateNode; + const containerInfo = fiberRoot.containerInfo; + const pendingChildren = fiberRoot.pendingChildren; + replaceContainerChildren(containerInfo, pendingChildren); + } + return; + } + case HostPortal: { + if (supportsPersistence) { + const portal = finishedWork.stateNode; + const containerInfo = portal.containerInfo; + const pendingChildren = portal.pendingChildren; + replaceContainerChildren(containerInfo, pendingChildren); + } return; } case Profiler: { From e66e7a0fb875dac2d3e34c005c5be7503f979143 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Thu, 7 Apr 2022 19:09:07 -0400 Subject: [PATCH 03/10] Inline commitWork into commitMutationOnFiber There's not really any reason these should be separate functions. The factoring has gotten sloppy and redundant because there's similar logic in both places, which is more obvious now that they're combined. Next I'll start combining the redundant branches. --- .../src/ReactFiberCommitWork.new.js | 357 ++++++++---------- .../src/ReactFiberCommitWork.old.js | 357 ++++++++---------- .../react-reconciler/src/ReactFiberFlags.js | 2 - 3 files changed, 320 insertions(+), 396 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index 337f3e50b2316..35a7e71ed330b 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js @@ -68,13 +68,11 @@ import { NoFlags, ContentReset, Placement, - PlacementAndUpdate, ChildDeletion, Snapshot, Update, Ref, Hydrating, - HydratingAndUpdate, Passive, BeforeMutationMask, MutationMask, @@ -1799,156 +1797,6 @@ function commitDeletion( detachFiberMutation(current); } -function commitWork(current: Fiber | null, finishedWork: Fiber): void { - switch (finishedWork.tag) { - case FunctionComponent: - case ForwardRef: - case MemoComponent: - case SimpleMemoComponent: { - commitHookEffectListUnmount( - HookInsertion | HookHasEffect, - finishedWork, - finishedWork.return, - ); - commitHookEffectListMount(HookInsertion | HookHasEffect, finishedWork); - // Layout effects are destroyed during the mutation phase so that all - // destroy functions for all fibers are called before any create functions. - // This prevents sibling component effects from interfering with each other, - // e.g. a destroy function in one component should never override a ref set - // by a create function in another component during the same commit. - if ( - enableProfilerTimer && - enableProfilerCommitHooks && - finishedWork.mode & ProfileMode - ) { - try { - startLayoutEffectTimer(); - commitHookEffectListUnmount( - HookLayout | HookHasEffect, - finishedWork, - finishedWork.return, - ); - } finally { - recordLayoutEffectDuration(finishedWork); - } - } else { - commitHookEffectListUnmount( - HookLayout | HookHasEffect, - finishedWork, - finishedWork.return, - ); - } - return; - } - case ClassComponent: { - return; - } - case HostComponent: { - if (supportsMutation) { - const instance: Instance = finishedWork.stateNode; - if (instance != null) { - // Commit the work prepared earlier. - const newProps = finishedWork.memoizedProps; - // For hydration we reuse the update path but we treat the oldProps - // as the newProps. The updatePayload will contain the real change in - // this case. - const oldProps = current !== null ? current.memoizedProps : newProps; - const type = finishedWork.type; - // TODO: Type the updateQueue to be specific to host components. - const updatePayload: null | UpdatePayload = (finishedWork.updateQueue: any); - finishedWork.updateQueue = null; - if (updatePayload !== null) { - commitUpdate( - instance, - updatePayload, - type, - oldProps, - newProps, - finishedWork, - ); - } - } - } - return; - } - case HostText: { - if (supportsMutation) { - if (finishedWork.stateNode === null) { - throw new Error( - 'This should have a text node initialized. This error is likely ' + - 'caused by a bug in React. Please file an issue.', - ); - } - - const textInstance: TextInstance = finishedWork.stateNode; - const newText: string = finishedWork.memoizedProps; - // For hydration we reuse the update path but we treat the oldProps - // as the newProps. The updatePayload will contain the real change in - // this case. - const oldText: string = - current !== null ? current.memoizedProps : newText; - commitTextUpdate(textInstance, oldText, newText); - } - return; - } - case HostRoot: { - if (supportsMutation && supportsHydration) { - if (current !== null) { - const prevRootState: RootState = current.memoizedState; - if (prevRootState.isDehydrated) { - const root: FiberRoot = finishedWork.stateNode; - commitHydratedContainer(root.containerInfo); - } - } - } - if (supportsPersistence) { - const fiberRoot: FiberRoot = finishedWork.stateNode; - const containerInfo = fiberRoot.containerInfo; - const pendingChildren = fiberRoot.pendingChildren; - replaceContainerChildren(containerInfo, pendingChildren); - } - return; - } - case HostPortal: { - if (supportsPersistence) { - const portal = finishedWork.stateNode; - const containerInfo = portal.containerInfo; - const pendingChildren = portal.pendingChildren; - replaceContainerChildren(containerInfo, pendingChildren); - } - return; - } - case Profiler: { - return; - } - case SuspenseComponent: { - commitSuspenseCallback(finishedWork); - attachSuspenseRetryListeners(finishedWork); - return; - } - case SuspenseListComponent: { - attachSuspenseRetryListeners(finishedWork); - return; - } - case IncompleteClassComponent: { - return; - } - case ScopeComponent: { - if (enableScopeAPI) { - const scopeInstance = finishedWork.stateNode; - prepareScopeUpdate(scopeInstance, finishedWork); - return; - } - break; - } - } - - throw new Error( - 'This unit of work tag should not have side-effects. This error is ' + - 'likely caused by a bug in React. Please file an issue.', - ); -} - function commitSuspenseCallback(finishedWork: Fiber) { // TODO: Move this to passive phase const newState: SuspenseState | null = finishedWork.memoizedState; @@ -2133,6 +1981,7 @@ function commitMutationEffectsOnFiber( // switching on the type of work before checking the flags. That's what // we do in all the other phases. I think this one is only different // because of the shared reconciliation logic below. + const current = finishedWork.alternate; const flags = finishedWork.flags; if (flags & ContentReset) { @@ -2140,7 +1989,6 @@ function commitMutationEffectsOnFiber( } if (flags & Ref) { - const current = finishedWork.alternate; if (current !== null) { commitDetachRef(current); } @@ -2159,7 +2007,6 @@ function commitMutationEffectsOnFiber( const newState: OffscreenState | null = finishedWork.memoizedState; const isHidden = newState !== null; if (isHidden) { - const current = finishedWork.alternate; const wasHidden = current !== null && current.memoizedState !== null; if (!wasHidden) { // TODO: Move to passive phase @@ -2171,7 +2018,6 @@ function commitMutationEffectsOnFiber( case OffscreenComponent: { const newState: OffscreenState | null = finishedWork.memoizedState; const isHidden = newState !== null; - const current = finishedWork.alternate; const wasHidden = current !== null && current.memoizedState !== null; const offscreenBoundary: Fiber = finishedWork; @@ -2205,49 +2051,167 @@ function commitMutationEffectsOnFiber( } } - // The following switch statement is only concerned about placement, - // updates, and deletions. To avoid needing to add a case for every possible - // bitmap value, we remove the secondary effects from the effect tag and - // switch on that value. - const primaryFlags = flags & (Placement | Update | Hydrating); - outer: switch (primaryFlags) { - case Placement: { - commitPlacement(finishedWork); - // Clear the "placement" from effect tag so that we know that this is - // inserted, before any life-cycles like componentDidMount gets called. - // TODO: findDOMNode doesn't rely on this any more but isMounted does - // and isMounted is deprecated anyway so we should be able to kill this. - finishedWork.flags &= ~Placement; - break; - } - case PlacementAndUpdate: { - // Placement - commitPlacement(finishedWork); - // Clear the "placement" from effect tag so that we know that this is - // inserted, before any life-cycles like componentDidMount gets called. - finishedWork.flags &= ~Placement; + // These are related to reconciliation so they affect every fiber type; that's + // why they aren't in the main switch statement below. + if (flags & Placement) { + commitPlacement(finishedWork); + // Clear the "placement" from effect tag so that we know that this is + // inserted, before any life-cycles like componentDidMount gets called. + // TODO: findDOMNode doesn't rely on this any more but isMounted does + // and isMounted is deprecated anyway so we should be able to kill this. + finishedWork.flags &= ~Placement; + } + if (flags & Hydrating) { + finishedWork.flags &= ~Hydrating; + } - // Update - const current = finishedWork.alternate; - commitWork(current, finishedWork); - break; - } - case Hydrating: { - finishedWork.flags &= ~Hydrating; - break; - } - case HydratingAndUpdate: { - finishedWork.flags &= ~Hydrating; + // TODO: Move the ad-hoc flag checks above into the main switch statement. + if (flags & Update) { + switch (finishedWork.tag) { + case FunctionComponent: + case ForwardRef: + case MemoComponent: + case SimpleMemoComponent: { + commitHookEffectListUnmount( + HookInsertion | HookHasEffect, + finishedWork, + finishedWork.return, + ); + commitHookEffectListMount(HookInsertion | HookHasEffect, finishedWork); + // Layout effects are destroyed during the mutation phase so that all + // destroy functions for all fibers are called before any create functions. + // This prevents sibling component effects from interfering with each other, + // e.g. a destroy function in one component should never override a ref set + // by a create function in another component during the same commit. + if ( + enableProfilerTimer && + enableProfilerCommitHooks && + finishedWork.mode & ProfileMode + ) { + try { + startLayoutEffectTimer(); + commitHookEffectListUnmount( + HookLayout | HookHasEffect, + finishedWork, + finishedWork.return, + ); + } finally { + recordLayoutEffectDuration(finishedWork); + } + } else { + commitHookEffectListUnmount( + HookLayout | HookHasEffect, + finishedWork, + finishedWork.return, + ); + } + return; + } + case ClassComponent: { + return; + } + case HostComponent: { + if (supportsMutation) { + const instance: Instance = finishedWork.stateNode; + if (instance != null) { + // Commit the work prepared earlier. + const newProps = finishedWork.memoizedProps; + // For hydration we reuse the update path but we treat the oldProps + // as the newProps. The updatePayload will contain the real change in + // this case. + const oldProps = + current !== null ? current.memoizedProps : newProps; + const type = finishedWork.type; + // TODO: Type the updateQueue to be specific to host components. + const updatePayload: null | UpdatePayload = (finishedWork.updateQueue: any); + finishedWork.updateQueue = null; + if (updatePayload !== null) { + commitUpdate( + instance, + updatePayload, + type, + oldProps, + newProps, + finishedWork, + ); + } + } + } + return; + } + case HostText: { + if (supportsMutation) { + if (finishedWork.stateNode === null) { + throw new Error( + 'This should have a text node initialized. This error is likely ' + + 'caused by a bug in React. Please file an issue.', + ); + } - // Update - const current = finishedWork.alternate; - commitWork(current, finishedWork); - break; - } - case Update: { - const current = finishedWork.alternate; - commitWork(current, finishedWork); - break; + const textInstance: TextInstance = finishedWork.stateNode; + const newText: string = finishedWork.memoizedProps; + // For hydration we reuse the update path but we treat the oldProps + // as the newProps. The updatePayload will contain the real change in + // this case. + const oldText: string = + current !== null ? current.memoizedProps : newText; + commitTextUpdate(textInstance, oldText, newText); + } + return; + } + case HostRoot: { + if (supportsMutation && supportsHydration) { + if (current !== null) { + const prevRootState: RootState = current.memoizedState; + if (prevRootState.isDehydrated) { + commitHydratedContainer(root.containerInfo); + } + } + } + if (supportsPersistence) { + const containerInfo = root.containerInfo; + const pendingChildren = root.pendingChildren; + replaceContainerChildren(containerInfo, pendingChildren); + } + return; + } + case HostPortal: { + if (supportsPersistence) { + const portal = finishedWork.stateNode; + const containerInfo = portal.containerInfo; + const pendingChildren = portal.pendingChildren; + replaceContainerChildren(containerInfo, pendingChildren); + } + return; + } + case Profiler: { + return; + } + case SuspenseComponent: { + commitSuspenseCallback(finishedWork); + attachSuspenseRetryListeners(finishedWork); + return; + } + case SuspenseListComponent: { + attachSuspenseRetryListeners(finishedWork); + return; + } + case IncompleteClassComponent: { + return; + } + case ScopeComponent: { + if (enableScopeAPI) { + const scopeInstance = finishedWork.stateNode; + prepareScopeUpdate(scopeInstance, finishedWork); + } + return; + } + default: { + throw new Error( + 'This unit of work tag should not have side-effects. This error is ' + + 'likely caused by a bug in React. Please file an issue.', + ); + } } } } @@ -3056,7 +3020,6 @@ export { commitResetTextContent, commitPlacement, commitDeletion, - commitWork, commitAttachRef, commitDetachRef, invokeLayoutEffectMountInDEV, diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.old.js b/packages/react-reconciler/src/ReactFiberCommitWork.old.js index cd250eadfd130..334d08b952595 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.old.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.old.js @@ -68,13 +68,11 @@ import { NoFlags, ContentReset, Placement, - PlacementAndUpdate, ChildDeletion, Snapshot, Update, Ref, Hydrating, - HydratingAndUpdate, Passive, BeforeMutationMask, MutationMask, @@ -1799,156 +1797,6 @@ function commitDeletion( detachFiberMutation(current); } -function commitWork(current: Fiber | null, finishedWork: Fiber): void { - switch (finishedWork.tag) { - case FunctionComponent: - case ForwardRef: - case MemoComponent: - case SimpleMemoComponent: { - commitHookEffectListUnmount( - HookInsertion | HookHasEffect, - finishedWork, - finishedWork.return, - ); - commitHookEffectListMount(HookInsertion | HookHasEffect, finishedWork); - // Layout effects are destroyed during the mutation phase so that all - // destroy functions for all fibers are called before any create functions. - // This prevents sibling component effects from interfering with each other, - // e.g. a destroy function in one component should never override a ref set - // by a create function in another component during the same commit. - if ( - enableProfilerTimer && - enableProfilerCommitHooks && - finishedWork.mode & ProfileMode - ) { - try { - startLayoutEffectTimer(); - commitHookEffectListUnmount( - HookLayout | HookHasEffect, - finishedWork, - finishedWork.return, - ); - } finally { - recordLayoutEffectDuration(finishedWork); - } - } else { - commitHookEffectListUnmount( - HookLayout | HookHasEffect, - finishedWork, - finishedWork.return, - ); - } - return; - } - case ClassComponent: { - return; - } - case HostComponent: { - if (supportsMutation) { - const instance: Instance = finishedWork.stateNode; - if (instance != null) { - // Commit the work prepared earlier. - const newProps = finishedWork.memoizedProps; - // For hydration we reuse the update path but we treat the oldProps - // as the newProps. The updatePayload will contain the real change in - // this case. - const oldProps = current !== null ? current.memoizedProps : newProps; - const type = finishedWork.type; - // TODO: Type the updateQueue to be specific to host components. - const updatePayload: null | UpdatePayload = (finishedWork.updateQueue: any); - finishedWork.updateQueue = null; - if (updatePayload !== null) { - commitUpdate( - instance, - updatePayload, - type, - oldProps, - newProps, - finishedWork, - ); - } - } - } - return; - } - case HostText: { - if (supportsMutation) { - if (finishedWork.stateNode === null) { - throw new Error( - 'This should have a text node initialized. This error is likely ' + - 'caused by a bug in React. Please file an issue.', - ); - } - - const textInstance: TextInstance = finishedWork.stateNode; - const newText: string = finishedWork.memoizedProps; - // For hydration we reuse the update path but we treat the oldProps - // as the newProps. The updatePayload will contain the real change in - // this case. - const oldText: string = - current !== null ? current.memoizedProps : newText; - commitTextUpdate(textInstance, oldText, newText); - } - return; - } - case HostRoot: { - if (supportsMutation && supportsHydration) { - if (current !== null) { - const prevRootState: RootState = current.memoizedState; - if (prevRootState.isDehydrated) { - const root: FiberRoot = finishedWork.stateNode; - commitHydratedContainer(root.containerInfo); - } - } - } - if (supportsPersistence) { - const fiberRoot: FiberRoot = finishedWork.stateNode; - const containerInfo = fiberRoot.containerInfo; - const pendingChildren = fiberRoot.pendingChildren; - replaceContainerChildren(containerInfo, pendingChildren); - } - return; - } - case HostPortal: { - if (supportsPersistence) { - const portal = finishedWork.stateNode; - const containerInfo = portal.containerInfo; - const pendingChildren = portal.pendingChildren; - replaceContainerChildren(containerInfo, pendingChildren); - } - return; - } - case Profiler: { - return; - } - case SuspenseComponent: { - commitSuspenseCallback(finishedWork); - attachSuspenseRetryListeners(finishedWork); - return; - } - case SuspenseListComponent: { - attachSuspenseRetryListeners(finishedWork); - return; - } - case IncompleteClassComponent: { - return; - } - case ScopeComponent: { - if (enableScopeAPI) { - const scopeInstance = finishedWork.stateNode; - prepareScopeUpdate(scopeInstance, finishedWork); - return; - } - break; - } - } - - throw new Error( - 'This unit of work tag should not have side-effects. This error is ' + - 'likely caused by a bug in React. Please file an issue.', - ); -} - function commitSuspenseCallback(finishedWork: Fiber) { // TODO: Move this to passive phase const newState: SuspenseState | null = finishedWork.memoizedState; @@ -2133,6 +1981,7 @@ function commitMutationEffectsOnFiber( // switching on the type of work before checking the flags. That's what // we do in all the other phases. I think this one is only different // because of the shared reconciliation logic below. + const current = finishedWork.alternate; const flags = finishedWork.flags; if (flags & ContentReset) { @@ -2140,7 +1989,6 @@ function commitMutationEffectsOnFiber( } if (flags & Ref) { - const current = finishedWork.alternate; if (current !== null) { commitDetachRef(current); } @@ -2159,7 +2007,6 @@ function commitMutationEffectsOnFiber( const newState: OffscreenState | null = finishedWork.memoizedState; const isHidden = newState !== null; if (isHidden) { - const current = finishedWork.alternate; const wasHidden = current !== null && current.memoizedState !== null; if (!wasHidden) { // TODO: Move to passive phase @@ -2171,7 +2018,6 @@ function commitMutationEffectsOnFiber( case OffscreenComponent: { const newState: OffscreenState | null = finishedWork.memoizedState; const isHidden = newState !== null; - const current = finishedWork.alternate; const wasHidden = current !== null && current.memoizedState !== null; const offscreenBoundary: Fiber = finishedWork; @@ -2205,49 +2051,167 @@ function commitMutationEffectsOnFiber( } } - // The following switch statement is only concerned about placement, - // updates, and deletions. To avoid needing to add a case for every possible - // bitmap value, we remove the secondary effects from the effect tag and - // switch on that value. - const primaryFlags = flags & (Placement | Update | Hydrating); - outer: switch (primaryFlags) { - case Placement: { - commitPlacement(finishedWork); - // Clear the "placement" from effect tag so that we know that this is - // inserted, before any life-cycles like componentDidMount gets called. - // TODO: findDOMNode doesn't rely on this any more but isMounted does - // and isMounted is deprecated anyway so we should be able to kill this. - finishedWork.flags &= ~Placement; - break; - } - case PlacementAndUpdate: { - // Placement - commitPlacement(finishedWork); - // Clear the "placement" from effect tag so that we know that this is - // inserted, before any life-cycles like componentDidMount gets called. - finishedWork.flags &= ~Placement; + // These are related to reconciliation so they affect every fiber type; that's + // why they aren't in the main switch statement below. + if (flags & Placement) { + commitPlacement(finishedWork); + // Clear the "placement" from effect tag so that we know that this is + // inserted, before any life-cycles like componentDidMount gets called. + // TODO: findDOMNode doesn't rely on this any more but isMounted does + // and isMounted is deprecated anyway so we should be able to kill this. + finishedWork.flags &= ~Placement; + } + if (flags & Hydrating) { + finishedWork.flags &= ~Hydrating; + } - // Update - const current = finishedWork.alternate; - commitWork(current, finishedWork); - break; - } - case Hydrating: { - finishedWork.flags &= ~Hydrating; - break; - } - case HydratingAndUpdate: { - finishedWork.flags &= ~Hydrating; + // TODO: Move the ad-hoc flag checks above into the main switch statement. + if (flags & Update) { + switch (finishedWork.tag) { + case FunctionComponent: + case ForwardRef: + case MemoComponent: + case SimpleMemoComponent: { + commitHookEffectListUnmount( + HookInsertion | HookHasEffect, + finishedWork, + finishedWork.return, + ); + commitHookEffectListMount(HookInsertion | HookHasEffect, finishedWork); + // Layout effects are destroyed during the mutation phase so that all + // destroy functions for all fibers are called before any create functions. + // This prevents sibling component effects from interfering with each other, + // e.g. a destroy function in one component should never override a ref set + // by a create function in another component during the same commit. + if ( + enableProfilerTimer && + enableProfilerCommitHooks && + finishedWork.mode & ProfileMode + ) { + try { + startLayoutEffectTimer(); + commitHookEffectListUnmount( + HookLayout | HookHasEffect, + finishedWork, + finishedWork.return, + ); + } finally { + recordLayoutEffectDuration(finishedWork); + } + } else { + commitHookEffectListUnmount( + HookLayout | HookHasEffect, + finishedWork, + finishedWork.return, + ); + } + return; + } + case ClassComponent: { + return; + } + case HostComponent: { + if (supportsMutation) { + const instance: Instance = finishedWork.stateNode; + if (instance != null) { + // Commit the work prepared earlier. + const newProps = finishedWork.memoizedProps; + // For hydration we reuse the update path but we treat the oldProps + // as the newProps. The updatePayload will contain the real change in + // this case. + const oldProps = + current !== null ? current.memoizedProps : newProps; + const type = finishedWork.type; + // TODO: Type the updateQueue to be specific to host components. + const updatePayload: null | UpdatePayload = (finishedWork.updateQueue: any); + finishedWork.updateQueue = null; + if (updatePayload !== null) { + commitUpdate( + instance, + updatePayload, + type, + oldProps, + newProps, + finishedWork, + ); + } + } + } + return; + } + case HostText: { + if (supportsMutation) { + if (finishedWork.stateNode === null) { + throw new Error( + 'This should have a text node initialized. This error is likely ' + + 'caused by a bug in React. Please file an issue.', + ); + } - // Update - const current = finishedWork.alternate; - commitWork(current, finishedWork); - break; - } - case Update: { - const current = finishedWork.alternate; - commitWork(current, finishedWork); - break; + const textInstance: TextInstance = finishedWork.stateNode; + const newText: string = finishedWork.memoizedProps; + // For hydration we reuse the update path but we treat the oldProps + // as the newProps. The updatePayload will contain the real change in + // this case. + const oldText: string = + current !== null ? current.memoizedProps : newText; + commitTextUpdate(textInstance, oldText, newText); + } + return; + } + case HostRoot: { + if (supportsMutation && supportsHydration) { + if (current !== null) { + const prevRootState: RootState = current.memoizedState; + if (prevRootState.isDehydrated) { + commitHydratedContainer(root.containerInfo); + } + } + } + if (supportsPersistence) { + const containerInfo = root.containerInfo; + const pendingChildren = root.pendingChildren; + replaceContainerChildren(containerInfo, pendingChildren); + } + return; + } + case HostPortal: { + if (supportsPersistence) { + const portal = finishedWork.stateNode; + const containerInfo = portal.containerInfo; + const pendingChildren = portal.pendingChildren; + replaceContainerChildren(containerInfo, pendingChildren); + } + return; + } + case Profiler: { + return; + } + case SuspenseComponent: { + commitSuspenseCallback(finishedWork); + attachSuspenseRetryListeners(finishedWork); + return; + } + case SuspenseListComponent: { + attachSuspenseRetryListeners(finishedWork); + return; + } + case IncompleteClassComponent: { + return; + } + case ScopeComponent: { + if (enableScopeAPI) { + const scopeInstance = finishedWork.stateNode; + prepareScopeUpdate(scopeInstance, finishedWork); + } + return; + } + default: { + throw new Error( + 'This unit of work tag should not have side-effects. This error is ' + + 'likely caused by a bug in React. Please file an issue.', + ); + } } } } @@ -3056,7 +3020,6 @@ export { commitResetTextContent, commitPlacement, commitDeletion, - commitWork, commitAttachRef, commitDetachRef, invokeLayoutEffectMountInDEV, diff --git a/packages/react-reconciler/src/ReactFiberFlags.js b/packages/react-reconciler/src/ReactFiberFlags.js index 805c4bed918e9..d22d9a189b6e5 100644 --- a/packages/react-reconciler/src/ReactFiberFlags.js +++ b/packages/react-reconciler/src/ReactFiberFlags.js @@ -18,7 +18,6 @@ export const PerformedWork = /* */ 0b00000000000000000000000001; // You can change the rest (and add more). export const Placement = /* */ 0b00000000000000000000000010; export const Update = /* */ 0b00000000000000000000000100; -export const PlacementAndUpdate = /* */ Placement | Update; export const Deletion = /* */ 0b00000000000000000000001000; export const ChildDeletion = /* */ 0b00000000000000000000010000; export const ContentReset = /* */ 0b00000000000000000000100000; @@ -29,7 +28,6 @@ export const Ref = /* */ 0b00000000000000001000000000; export const Snapshot = /* */ 0b00000000000000010000000000; export const Passive = /* */ 0b00000000000000100000000000; export const Hydrating = /* */ 0b00000000000001000000000000; -export const HydratingAndUpdate = /* */ Hydrating | Update; export const Visibility = /* */ 0b00000000000010000000000000; export const StoreConsistency = /* */ 0b00000000000100000000000000; From 54b5b32d53355ad1634a97e604a583e8da6fa842 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Thu, 7 Apr 2022 19:51:05 -0400 Subject: [PATCH 04/10] Move Update flag check into each switch case The fiber tag is more specific than the effect flag, so we should always refine the type of work first, to minimize redundant checks. In the next step I'll move all other other flag checks in this function into the same switch statement. --- .../src/ReactFiberCommitWork.new.js | 72 +++++++++---------- .../src/ReactFiberCommitWork.old.js | 72 +++++++++---------- 2 files changed, 72 insertions(+), 72 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index 35a7e71ed330b..33854bc3f74ea 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js @@ -2065,13 +2065,14 @@ function commitMutationEffectsOnFiber( finishedWork.flags &= ~Hydrating; } + // All logic in these branches should be wrapped in a flag check. // TODO: Move the ad-hoc flag checks above into the main switch statement. - if (flags & Update) { - switch (finishedWork.tag) { - case FunctionComponent: - case ForwardRef: - case MemoComponent: - case SimpleMemoComponent: { + switch (finishedWork.tag) { + case FunctionComponent: + case ForwardRef: + case MemoComponent: + case SimpleMemoComponent: { + if (flags & Update) { commitHookEffectListUnmount( HookInsertion | HookHasEffect, finishedWork, @@ -2105,12 +2106,11 @@ function commitMutationEffectsOnFiber( finishedWork.return, ); } - return; - } - case ClassComponent: { - return; } - case HostComponent: { + return; + } + case HostComponent: { + if (flags & Update) { if (supportsMutation) { const instance: Instance = finishedWork.stateNode; if (instance != null) { @@ -2137,9 +2137,11 @@ function commitMutationEffectsOnFiber( } } } - return; } - case HostText: { + return; + } + case HostText: { + if (flags & Update) { if (supportsMutation) { if (finishedWork.stateNode === null) { throw new Error( @@ -2157,9 +2159,11 @@ function commitMutationEffectsOnFiber( current !== null ? current.memoizedProps : newText; commitTextUpdate(textInstance, oldText, newText); } - return; } - case HostRoot: { + return; + } + case HostRoot: { + if (flags & Update) { if (supportsMutation && supportsHydration) { if (current !== null) { const prevRootState: RootState = current.memoizedState; @@ -2173,45 +2177,41 @@ function commitMutationEffectsOnFiber( const pendingChildren = root.pendingChildren; replaceContainerChildren(containerInfo, pendingChildren); } - return; } - case HostPortal: { + return; + } + case HostPortal: { + if (flags & Update) { if (supportsPersistence) { const portal = finishedWork.stateNode; const containerInfo = portal.containerInfo; const pendingChildren = portal.pendingChildren; replaceContainerChildren(containerInfo, pendingChildren); } - return; - } - case Profiler: { - return; } - case SuspenseComponent: { + return; + } + case SuspenseComponent: { + if (flags & Update) { commitSuspenseCallback(finishedWork); attachSuspenseRetryListeners(finishedWork); - return; } - case SuspenseListComponent: { + return; + } + case SuspenseListComponent: { + if (flags & Update) { attachSuspenseRetryListeners(finishedWork); - return; - } - case IncompleteClassComponent: { - return; } - case ScopeComponent: { + return; + } + case ScopeComponent: { + if (flags & Update) { if (enableScopeAPI) { const scopeInstance = finishedWork.stateNode; prepareScopeUpdate(scopeInstance, finishedWork); } - return; - } - default: { - throw new Error( - 'This unit of work tag should not have side-effects. This error is ' + - 'likely caused by a bug in React. Please file an issue.', - ); } + return; } } } diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.old.js b/packages/react-reconciler/src/ReactFiberCommitWork.old.js index 334d08b952595..de1e3239abbb9 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.old.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.old.js @@ -2065,13 +2065,14 @@ function commitMutationEffectsOnFiber( finishedWork.flags &= ~Hydrating; } + // All logic in these branches should be wrapped in a flag check. // TODO: Move the ad-hoc flag checks above into the main switch statement. - if (flags & Update) { - switch (finishedWork.tag) { - case FunctionComponent: - case ForwardRef: - case MemoComponent: - case SimpleMemoComponent: { + switch (finishedWork.tag) { + case FunctionComponent: + case ForwardRef: + case MemoComponent: + case SimpleMemoComponent: { + if (flags & Update) { commitHookEffectListUnmount( HookInsertion | HookHasEffect, finishedWork, @@ -2105,12 +2106,11 @@ function commitMutationEffectsOnFiber( finishedWork.return, ); } - return; - } - case ClassComponent: { - return; } - case HostComponent: { + return; + } + case HostComponent: { + if (flags & Update) { if (supportsMutation) { const instance: Instance = finishedWork.stateNode; if (instance != null) { @@ -2137,9 +2137,11 @@ function commitMutationEffectsOnFiber( } } } - return; } - case HostText: { + return; + } + case HostText: { + if (flags & Update) { if (supportsMutation) { if (finishedWork.stateNode === null) { throw new Error( @@ -2157,9 +2159,11 @@ function commitMutationEffectsOnFiber( current !== null ? current.memoizedProps : newText; commitTextUpdate(textInstance, oldText, newText); } - return; } - case HostRoot: { + return; + } + case HostRoot: { + if (flags & Update) { if (supportsMutation && supportsHydration) { if (current !== null) { const prevRootState: RootState = current.memoizedState; @@ -2173,45 +2177,41 @@ function commitMutationEffectsOnFiber( const pendingChildren = root.pendingChildren; replaceContainerChildren(containerInfo, pendingChildren); } - return; } - case HostPortal: { + return; + } + case HostPortal: { + if (flags & Update) { if (supportsPersistence) { const portal = finishedWork.stateNode; const containerInfo = portal.containerInfo; const pendingChildren = portal.pendingChildren; replaceContainerChildren(containerInfo, pendingChildren); } - return; - } - case Profiler: { - return; } - case SuspenseComponent: { + return; + } + case SuspenseComponent: { + if (flags & Update) { commitSuspenseCallback(finishedWork); attachSuspenseRetryListeners(finishedWork); - return; } - case SuspenseListComponent: { + return; + } + case SuspenseListComponent: { + if (flags & Update) { attachSuspenseRetryListeners(finishedWork); - return; - } - case IncompleteClassComponent: { - return; } - case ScopeComponent: { + return; + } + case ScopeComponent: { + if (flags & Update) { if (enableScopeAPI) { const scopeInstance = finishedWork.stateNode; prepareScopeUpdate(scopeInstance, finishedWork); } - return; - } - default: { - throw new Error( - 'This unit of work tag should not have side-effects. This error is ' + - 'likely caused by a bug in React. Please file an issue.', - ); } + return; } } } From c99c5f1df6e3c274d34bd2e5f0b27ac4af481f94 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Thu, 7 Apr 2022 20:05:50 -0400 Subject: [PATCH 05/10] Move ad hoc flag checks into main switch statement We should always refine the type of fiber before checking the effect flag, because the fiber tag is more specific. Now we have a single switch statement for all mutation effects. --- .../src/ReactFiberCommitWork.new.js | 219 ++++++++++-------- .../src/ReactFiberCommitWork.old.js | 219 ++++++++++-------- 2 files changed, 240 insertions(+), 198 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index 33854bc3f74ea..09ee3ca15b5f5 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js @@ -1899,13 +1899,6 @@ export function isSuspenseBoundaryBeingHidden( return false; } -function commitResetTextContent(current: Fiber) { - if (!supportsMutation) { - return; - } - resetTextContent(current.stateNode); -} - export function commitMutationEffects( root: FiberRoot, firstChild: Fiber, @@ -1977,101 +1970,19 @@ function commitMutationEffectsOnFiber( root: FiberRoot, lanes: Lanes, ) { - // TODO: The factoring of this phase could probably be improved. Consider - // switching on the type of work before checking the flags. That's what - // we do in all the other phases. I think this one is only different - // because of the shared reconciliation logic below. const current = finishedWork.alternate; const flags = finishedWork.flags; - if (flags & ContentReset) { - commitResetTextContent(finishedWork); - } - - if (flags & Ref) { - if (current !== null) { - commitDetachRef(current); - } - if (enableScopeAPI) { - // TODO: This is a temporary solution that allowed us to transition away - // from React Flare on www. - if (finishedWork.tag === ScopeComponent) { - commitAttachRef(finishedWork); - } - } - } - - if (flags & Visibility) { - switch (finishedWork.tag) { - case SuspenseComponent: { - const newState: OffscreenState | null = finishedWork.memoizedState; - const isHidden = newState !== null; - if (isHidden) { - const wasHidden = current !== null && current.memoizedState !== null; - if (!wasHidden) { - // TODO: Move to passive phase - markCommitTimeOfFallback(); - } - } - break; - } - case OffscreenComponent: { - const newState: OffscreenState | null = finishedWork.memoizedState; - const isHidden = newState !== null; - const wasHidden = current !== null && current.memoizedState !== null; - const offscreenBoundary: Fiber = finishedWork; - - if (supportsMutation) { - // TODO: This needs to run whenever there's an insertion or update - // inside a hidden Offscreen tree. - hideOrUnhideAllChildren(offscreenBoundary, isHidden); - } - - if (enableSuspenseLayoutEffectSemantics) { - if (isHidden) { - if (!wasHidden) { - if ((offscreenBoundary.mode & ConcurrentMode) !== NoMode) { - nextEffect = offscreenBoundary; - let offscreenChild = offscreenBoundary.child; - while (offscreenChild !== null) { - nextEffect = offscreenChild; - disappearLayoutEffects_begin(offscreenChild); - offscreenChild = offscreenChild.sibling; - } - } - } - } else { - if (wasHidden) { - // TODO: Move re-appear call here for symmetry? - } - } - break; - } - } - } - } - - // These are related to reconciliation so they affect every fiber type; that's - // why they aren't in the main switch statement below. - if (flags & Placement) { - commitPlacement(finishedWork); - // Clear the "placement" from effect tag so that we know that this is - // inserted, before any life-cycles like componentDidMount gets called. - // TODO: findDOMNode doesn't rely on this any more but isMounted does - // and isMounted is deprecated anyway so we should be able to kill this. - finishedWork.flags &= ~Placement; - } - if (flags & Hydrating) { - finishedWork.flags &= ~Hydrating; - } - - // All logic in these branches should be wrapped in a flag check. - // TODO: Move the ad-hoc flag checks above into the main switch statement. + // The effect flag should be checked *after* we refine the type of fiber, + // because the fiber tag is more specific. An exception is any flag related + // to reconcilation, because those can be set on all fiber types. switch (finishedWork.tag) { case FunctionComponent: case ForwardRef: case MemoComponent: case SimpleMemoComponent: { + commitReconciliationEffects(finishedWork); + if (flags & Update) { commitHookEffectListUnmount( HookInsertion | HookHasEffect, @@ -2109,9 +2020,31 @@ function commitMutationEffectsOnFiber( } return; } + case ClassComponent: { + commitReconciliationEffects(finishedWork); + + if (flags & Ref) { + if (current !== null) { + commitDetachRef(current); + } + } + return; + } case HostComponent: { - if (flags & Update) { - if (supportsMutation) { + commitReconciliationEffects(finishedWork); + + if (flags & Ref) { + if (current !== null) { + commitDetachRef(current); + } + } + if (supportsMutation) { + if (flags & ContentReset) { + const instance: Instance = finishedWork.stateNode; + resetTextContent(instance); + } + + if (flags & Update) { const instance: Instance = finishedWork.stateNode; if (instance != null) { // Commit the work prepared earlier. @@ -2141,6 +2074,8 @@ function commitMutationEffectsOnFiber( return; } case HostText: { + commitReconciliationEffects(finishedWork); + if (flags & Update) { if (supportsMutation) { if (finishedWork.stateNode === null) { @@ -2163,6 +2098,8 @@ function commitMutationEffectsOnFiber( return; } case HostRoot: { + commitReconciliationEffects(finishedWork); + if (flags & Update) { if (supportsMutation && supportsHydration) { if (current !== null) { @@ -2181,6 +2118,8 @@ function commitMutationEffectsOnFiber( return; } case HostPortal: { + commitReconciliationEffects(finishedWork); + if (flags & Update) { if (supportsPersistence) { const portal = finishedWork.stateNode; @@ -2192,27 +2131,110 @@ function commitMutationEffectsOnFiber( return; } case SuspenseComponent: { + commitReconciliationEffects(finishedWork); + + if (flags & Visibility) { + const newState: OffscreenState | null = finishedWork.memoizedState; + const isHidden = newState !== null; + if (isHidden) { + const wasHidden = current !== null && current.memoizedState !== null; + if (!wasHidden) { + // TODO: Move to passive phase + markCommitTimeOfFallback(); + } + } + } if (flags & Update) { commitSuspenseCallback(finishedWork); attachSuspenseRetryListeners(finishedWork); } return; } + case OffscreenComponent: { + commitReconciliationEffects(finishedWork); + + if (flags & Visibility) { + const newState: OffscreenState | null = finishedWork.memoizedState; + const isHidden = newState !== null; + const wasHidden = current !== null && current.memoizedState !== null; + const offscreenBoundary: Fiber = finishedWork; + + if (supportsMutation) { + // TODO: This needs to run whenever there's an insertion or update + // inside a hidden Offscreen tree. + hideOrUnhideAllChildren(offscreenBoundary, isHidden); + } + + if (enableSuspenseLayoutEffectSemantics) { + if (isHidden) { + if (!wasHidden) { + if ((offscreenBoundary.mode & ConcurrentMode) !== NoMode) { + nextEffect = offscreenBoundary; + let offscreenChild = offscreenBoundary.child; + while (offscreenChild !== null) { + nextEffect = offscreenChild; + disappearLayoutEffects_begin(offscreenChild); + offscreenChild = offscreenChild.sibling; + } + } + } + } else { + if (wasHidden) { + // TODO: Move re-appear call here for symmetry? + } + } + } + } + return; + } case SuspenseListComponent: { + commitReconciliationEffects(finishedWork); + if (flags & Update) { attachSuspenseRetryListeners(finishedWork); } return; } case ScopeComponent: { - if (flags & Update) { - if (enableScopeAPI) { + if (enableScopeAPI) { + commitReconciliationEffects(finishedWork); + + // TODO: This is a temporary solution that allowed us to transition away + // from React Flare on www. + if (flags & Ref) { + if (current !== null) { + commitDetachRef(current); + } + commitAttachRef(finishedWork); + } + if (flags & Update) { const scopeInstance = finishedWork.stateNode; prepareScopeUpdate(scopeInstance, finishedWork); } } return; } + default: { + commitReconciliationEffects(finishedWork); + } + } +} + +function commitReconciliationEffects(finishedWork: Fiber) { + // Placement effects (insertions, reorders) can be scheduled on any fiber + // type. They needs to happen after the children effects have fired, but + // before the effects on this fiber have fired. + const flags = finishedWork.flags; + if (flags & Placement) { + commitPlacement(finishedWork); + // Clear the "placement" from effect tag so that we know that this is + // inserted, before any life-cycles like componentDidMount gets called. + // TODO: findDOMNode doesn't rely on this any more but isMounted does + // and isMounted is deprecated anyway so we should be able to kill this. + finishedWork.flags &= ~Placement; + } + if (flags & Hydrating) { + finishedWork.flags &= ~Hydrating; } } @@ -3017,7 +3039,6 @@ function invokePassiveEffectUnmountInDEV(fiber: Fiber): void { } export { - commitResetTextContent, commitPlacement, commitDeletion, commitAttachRef, diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.old.js b/packages/react-reconciler/src/ReactFiberCommitWork.old.js index de1e3239abbb9..d3ef4f4022707 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.old.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.old.js @@ -1899,13 +1899,6 @@ export function isSuspenseBoundaryBeingHidden( return false; } -function commitResetTextContent(current: Fiber) { - if (!supportsMutation) { - return; - } - resetTextContent(current.stateNode); -} - export function commitMutationEffects( root: FiberRoot, firstChild: Fiber, @@ -1977,101 +1970,19 @@ function commitMutationEffectsOnFiber( root: FiberRoot, lanes: Lanes, ) { - // TODO: The factoring of this phase could probably be improved. Consider - // switching on the type of work before checking the flags. That's what - // we do in all the other phases. I think this one is only different - // because of the shared reconciliation logic below. const current = finishedWork.alternate; const flags = finishedWork.flags; - if (flags & ContentReset) { - commitResetTextContent(finishedWork); - } - - if (flags & Ref) { - if (current !== null) { - commitDetachRef(current); - } - if (enableScopeAPI) { - // TODO: This is a temporary solution that allowed us to transition away - // from React Flare on www. - if (finishedWork.tag === ScopeComponent) { - commitAttachRef(finishedWork); - } - } - } - - if (flags & Visibility) { - switch (finishedWork.tag) { - case SuspenseComponent: { - const newState: OffscreenState | null = finishedWork.memoizedState; - const isHidden = newState !== null; - if (isHidden) { - const wasHidden = current !== null && current.memoizedState !== null; - if (!wasHidden) { - // TODO: Move to passive phase - markCommitTimeOfFallback(); - } - } - break; - } - case OffscreenComponent: { - const newState: OffscreenState | null = finishedWork.memoizedState; - const isHidden = newState !== null; - const wasHidden = current !== null && current.memoizedState !== null; - const offscreenBoundary: Fiber = finishedWork; - - if (supportsMutation) { - // TODO: This needs to run whenever there's an insertion or update - // inside a hidden Offscreen tree. - hideOrUnhideAllChildren(offscreenBoundary, isHidden); - } - - if (enableSuspenseLayoutEffectSemantics) { - if (isHidden) { - if (!wasHidden) { - if ((offscreenBoundary.mode & ConcurrentMode) !== NoMode) { - nextEffect = offscreenBoundary; - let offscreenChild = offscreenBoundary.child; - while (offscreenChild !== null) { - nextEffect = offscreenChild; - disappearLayoutEffects_begin(offscreenChild); - offscreenChild = offscreenChild.sibling; - } - } - } - } else { - if (wasHidden) { - // TODO: Move re-appear call here for symmetry? - } - } - break; - } - } - } - } - - // These are related to reconciliation so they affect every fiber type; that's - // why they aren't in the main switch statement below. - if (flags & Placement) { - commitPlacement(finishedWork); - // Clear the "placement" from effect tag so that we know that this is - // inserted, before any life-cycles like componentDidMount gets called. - // TODO: findDOMNode doesn't rely on this any more but isMounted does - // and isMounted is deprecated anyway so we should be able to kill this. - finishedWork.flags &= ~Placement; - } - if (flags & Hydrating) { - finishedWork.flags &= ~Hydrating; - } - - // All logic in these branches should be wrapped in a flag check. - // TODO: Move the ad-hoc flag checks above into the main switch statement. + // The effect flag should be checked *after* we refine the type of fiber, + // because the fiber tag is more specific. An exception is any flag related + // to reconcilation, because those can be set on all fiber types. switch (finishedWork.tag) { case FunctionComponent: case ForwardRef: case MemoComponent: case SimpleMemoComponent: { + commitReconciliationEffects(finishedWork); + if (flags & Update) { commitHookEffectListUnmount( HookInsertion | HookHasEffect, @@ -2109,9 +2020,31 @@ function commitMutationEffectsOnFiber( } return; } + case ClassComponent: { + commitReconciliationEffects(finishedWork); + + if (flags & Ref) { + if (current !== null) { + commitDetachRef(current); + } + } + return; + } case HostComponent: { - if (flags & Update) { - if (supportsMutation) { + commitReconciliationEffects(finishedWork); + + if (flags & Ref) { + if (current !== null) { + commitDetachRef(current); + } + } + if (supportsMutation) { + if (flags & ContentReset) { + const instance: Instance = finishedWork.stateNode; + resetTextContent(instance); + } + + if (flags & Update) { const instance: Instance = finishedWork.stateNode; if (instance != null) { // Commit the work prepared earlier. @@ -2141,6 +2074,8 @@ function commitMutationEffectsOnFiber( return; } case HostText: { + commitReconciliationEffects(finishedWork); + if (flags & Update) { if (supportsMutation) { if (finishedWork.stateNode === null) { @@ -2163,6 +2098,8 @@ function commitMutationEffectsOnFiber( return; } case HostRoot: { + commitReconciliationEffects(finishedWork); + if (flags & Update) { if (supportsMutation && supportsHydration) { if (current !== null) { @@ -2181,6 +2118,8 @@ function commitMutationEffectsOnFiber( return; } case HostPortal: { + commitReconciliationEffects(finishedWork); + if (flags & Update) { if (supportsPersistence) { const portal = finishedWork.stateNode; @@ -2192,27 +2131,110 @@ function commitMutationEffectsOnFiber( return; } case SuspenseComponent: { + commitReconciliationEffects(finishedWork); + + if (flags & Visibility) { + const newState: OffscreenState | null = finishedWork.memoizedState; + const isHidden = newState !== null; + if (isHidden) { + const wasHidden = current !== null && current.memoizedState !== null; + if (!wasHidden) { + // TODO: Move to passive phase + markCommitTimeOfFallback(); + } + } + } if (flags & Update) { commitSuspenseCallback(finishedWork); attachSuspenseRetryListeners(finishedWork); } return; } + case OffscreenComponent: { + commitReconciliationEffects(finishedWork); + + if (flags & Visibility) { + const newState: OffscreenState | null = finishedWork.memoizedState; + const isHidden = newState !== null; + const wasHidden = current !== null && current.memoizedState !== null; + const offscreenBoundary: Fiber = finishedWork; + + if (supportsMutation) { + // TODO: This needs to run whenever there's an insertion or update + // inside a hidden Offscreen tree. + hideOrUnhideAllChildren(offscreenBoundary, isHidden); + } + + if (enableSuspenseLayoutEffectSemantics) { + if (isHidden) { + if (!wasHidden) { + if ((offscreenBoundary.mode & ConcurrentMode) !== NoMode) { + nextEffect = offscreenBoundary; + let offscreenChild = offscreenBoundary.child; + while (offscreenChild !== null) { + nextEffect = offscreenChild; + disappearLayoutEffects_begin(offscreenChild); + offscreenChild = offscreenChild.sibling; + } + } + } + } else { + if (wasHidden) { + // TODO: Move re-appear call here for symmetry? + } + } + } + } + return; + } case SuspenseListComponent: { + commitReconciliationEffects(finishedWork); + if (flags & Update) { attachSuspenseRetryListeners(finishedWork); } return; } case ScopeComponent: { - if (flags & Update) { - if (enableScopeAPI) { + if (enableScopeAPI) { + commitReconciliationEffects(finishedWork); + + // TODO: This is a temporary solution that allowed us to transition away + // from React Flare on www. + if (flags & Ref) { + if (current !== null) { + commitDetachRef(current); + } + commitAttachRef(finishedWork); + } + if (flags & Update) { const scopeInstance = finishedWork.stateNode; prepareScopeUpdate(scopeInstance, finishedWork); } } return; } + default: { + commitReconciliationEffects(finishedWork); + } + } +} + +function commitReconciliationEffects(finishedWork: Fiber) { + // Placement effects (insertions, reorders) can be scheduled on any fiber + // type. They needs to happen after the children effects have fired, but + // before the effects on this fiber have fired. + const flags = finishedWork.flags; + if (flags & Placement) { + commitPlacement(finishedWork); + // Clear the "placement" from effect tag so that we know that this is + // inserted, before any life-cycles like componentDidMount gets called. + // TODO: findDOMNode doesn't rely on this any more but isMounted does + // and isMounted is deprecated anyway so we should be able to kill this. + finishedWork.flags &= ~Placement; + } + if (flags & Hydrating) { + finishedWork.flags &= ~Hydrating; } } @@ -3017,7 +3039,6 @@ function invokePassiveEffectUnmountInDEV(fiber: Fiber): void { } export { - commitResetTextContent, commitPlacement, commitDeletion, commitAttachRef, From bcc1b3121e3052f88a643eaefe93f300a4bfa7b2 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Fri, 8 Apr 2022 00:05:15 -0400 Subject: [PATCH 06/10] Move reportUncaughtErrorInDev to captureCommitPhaseError reportUncaughtErrorInDev is always followed by captureCommitPhaseError, so we can move it into that function. --- .../src/ReactFiberCommitWork.new.js | 19 +------------------ .../src/ReactFiberCommitWork.old.js | 19 +------------------ .../src/ReactFiberWorkLoop.new.js | 2 ++ .../src/ReactFiberWorkLoop.old.js | 2 ++ 4 files changed, 6 insertions(+), 36 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index 09ee3ca15b5f5..f4760c5d89ad6 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js @@ -180,7 +180,7 @@ let nextEffect: Fiber | null = null; let inProgressLanes: Lanes | null = null; let inProgressRoot: FiberRoot | null = null; -function reportUncaughtErrorInDEV(error) { +export function reportUncaughtErrorInDEV(error: mixed) { // Wrapping each small part of the commit phase into a guarded // callback is a bit too slow (https://github.com/facebook/react/pull/21666). // But we rely on it to surface errors to DEV tools like overlays @@ -221,7 +221,6 @@ function safelyCallCommitHookLayoutEffectListMount( try { commitHookEffectListMount(HookLayout, current); } catch (error) { - reportUncaughtErrorInDEV(error); captureCommitPhaseError(current, nearestMountedAncestor, error); } } @@ -235,7 +234,6 @@ function safelyCallComponentWillUnmount( try { callComponentWillUnmountWithTimer(current, instance); } catch (error) { - reportUncaughtErrorInDEV(error); captureCommitPhaseError(current, nearestMountedAncestor, error); } } @@ -249,7 +247,6 @@ function safelyCallComponentDidMount( try { instance.componentDidMount(); } catch (error) { - reportUncaughtErrorInDEV(error); captureCommitPhaseError(current, nearestMountedAncestor, error); } } @@ -259,7 +256,6 @@ function safelyAttachRef(current: Fiber, nearestMountedAncestor: Fiber | null) { try { commitAttachRef(current); } catch (error) { - reportUncaughtErrorInDEV(error); captureCommitPhaseError(current, nearestMountedAncestor, error); } } @@ -285,7 +281,6 @@ function safelyDetachRef(current: Fiber, nearestMountedAncestor: Fiber | null) { retVal = ref(null); } } catch (error) { - reportUncaughtErrorInDEV(error); captureCommitPhaseError(current, nearestMountedAncestor, error); } if (__DEV__) { @@ -311,7 +306,6 @@ function safelyCallDestroy( try { destroy(); } catch (error) { - reportUncaughtErrorInDEV(error); captureCommitPhaseError(current, nearestMountedAncestor, error); } } @@ -373,7 +367,6 @@ function commitBeforeMutationEffects_complete() { try { commitBeforeMutationEffectsOnFiber(fiber); } catch (error) { - reportUncaughtErrorInDEV(error); captureCommitPhaseError(fiber, fiber.return, error); } resetCurrentDebugFiberInDEV(); @@ -1926,7 +1919,6 @@ function commitMutationEffects_begin(root: FiberRoot, lanes: Lanes) { try { commitDeletion(root, childToDelete, fiber); } catch (error) { - reportUncaughtErrorInDEV(error); captureCommitPhaseError(childToDelete, fiber, error); } } @@ -1949,7 +1941,6 @@ function commitMutationEffects_complete(root: FiberRoot, lanes: Lanes) { try { commitMutationEffectsOnFiber(fiber, root, lanes); } catch (error) { - reportUncaughtErrorInDEV(error); captureCommitPhaseError(fiber, fiber.return, error); } resetCurrentDebugFiberInDEV(); @@ -2340,7 +2331,6 @@ function commitLayoutMountEffects_complete( try { commitLayoutEffectOnFiber(root, current, fiber, committedLanes); } catch (error) { - reportUncaughtErrorInDEV(error); captureCommitPhaseError(fiber, fiber.return, error); } resetCurrentDebugFiberInDEV(); @@ -2481,7 +2471,6 @@ function reappearLayoutEffects_complete(subtreeRoot: Fiber) { try { reappearLayoutEffectsOnFiber(fiber); } catch (error) { - reportUncaughtErrorInDEV(error); captureCommitPhaseError(fiber, fiber.return, error); } resetCurrentDebugFiberInDEV(); @@ -2543,7 +2532,6 @@ function commitPassiveMountEffects_complete( try { commitPassiveMountOnFiber(root, fiber, committedLanes); } catch (error) { - reportUncaughtErrorInDEV(error); captureCommitPhaseError(fiber, fiber.return, error); } resetCurrentDebugFiberInDEV(); @@ -2945,7 +2933,6 @@ function invokeLayoutEffectMountInDEV(fiber: Fiber): void { try { commitHookEffectListMount(HookLayout | HookHasEffect, fiber); } catch (error) { - reportUncaughtErrorInDEV(error); captureCommitPhaseError(fiber, fiber.return, error); } break; @@ -2955,7 +2942,6 @@ function invokeLayoutEffectMountInDEV(fiber: Fiber): void { try { instance.componentDidMount(); } catch (error) { - reportUncaughtErrorInDEV(error); captureCommitPhaseError(fiber, fiber.return, error); } break; @@ -2975,7 +2961,6 @@ function invokePassiveEffectMountInDEV(fiber: Fiber): void { try { commitHookEffectListMount(HookPassive | HookHasEffect, fiber); } catch (error) { - reportUncaughtErrorInDEV(error); captureCommitPhaseError(fiber, fiber.return, error); } break; @@ -2999,7 +2984,6 @@ function invokeLayoutEffectUnmountInDEV(fiber: Fiber): void { fiber.return, ); } catch (error) { - reportUncaughtErrorInDEV(error); captureCommitPhaseError(fiber, fiber.return, error); } break; @@ -3030,7 +3014,6 @@ function invokePassiveEffectUnmountInDEV(fiber: Fiber): void { fiber.return, ); } catch (error) { - reportUncaughtErrorInDEV(error); captureCommitPhaseError(fiber, fiber.return, error); } } diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.old.js b/packages/react-reconciler/src/ReactFiberCommitWork.old.js index d3ef4f4022707..078a5b500ff9b 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.old.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.old.js @@ -180,7 +180,7 @@ let nextEffect: Fiber | null = null; let inProgressLanes: Lanes | null = null; let inProgressRoot: FiberRoot | null = null; -function reportUncaughtErrorInDEV(error) { +export function reportUncaughtErrorInDEV(error: mixed) { // Wrapping each small part of the commit phase into a guarded // callback is a bit too slow (https://github.com/facebook/react/pull/21666). // But we rely on it to surface errors to DEV tools like overlays @@ -221,7 +221,6 @@ function safelyCallCommitHookLayoutEffectListMount( try { commitHookEffectListMount(HookLayout, current); } catch (error) { - reportUncaughtErrorInDEV(error); captureCommitPhaseError(current, nearestMountedAncestor, error); } } @@ -235,7 +234,6 @@ function safelyCallComponentWillUnmount( try { callComponentWillUnmountWithTimer(current, instance); } catch (error) { - reportUncaughtErrorInDEV(error); captureCommitPhaseError(current, nearestMountedAncestor, error); } } @@ -249,7 +247,6 @@ function safelyCallComponentDidMount( try { instance.componentDidMount(); } catch (error) { - reportUncaughtErrorInDEV(error); captureCommitPhaseError(current, nearestMountedAncestor, error); } } @@ -259,7 +256,6 @@ function safelyAttachRef(current: Fiber, nearestMountedAncestor: Fiber | null) { try { commitAttachRef(current); } catch (error) { - reportUncaughtErrorInDEV(error); captureCommitPhaseError(current, nearestMountedAncestor, error); } } @@ -285,7 +281,6 @@ function safelyDetachRef(current: Fiber, nearestMountedAncestor: Fiber | null) { retVal = ref(null); } } catch (error) { - reportUncaughtErrorInDEV(error); captureCommitPhaseError(current, nearestMountedAncestor, error); } if (__DEV__) { @@ -311,7 +306,6 @@ function safelyCallDestroy( try { destroy(); } catch (error) { - reportUncaughtErrorInDEV(error); captureCommitPhaseError(current, nearestMountedAncestor, error); } } @@ -373,7 +367,6 @@ function commitBeforeMutationEffects_complete() { try { commitBeforeMutationEffectsOnFiber(fiber); } catch (error) { - reportUncaughtErrorInDEV(error); captureCommitPhaseError(fiber, fiber.return, error); } resetCurrentDebugFiberInDEV(); @@ -1926,7 +1919,6 @@ function commitMutationEffects_begin(root: FiberRoot, lanes: Lanes) { try { commitDeletion(root, childToDelete, fiber); } catch (error) { - reportUncaughtErrorInDEV(error); captureCommitPhaseError(childToDelete, fiber, error); } } @@ -1949,7 +1941,6 @@ function commitMutationEffects_complete(root: FiberRoot, lanes: Lanes) { try { commitMutationEffectsOnFiber(fiber, root, lanes); } catch (error) { - reportUncaughtErrorInDEV(error); captureCommitPhaseError(fiber, fiber.return, error); } resetCurrentDebugFiberInDEV(); @@ -2340,7 +2331,6 @@ function commitLayoutMountEffects_complete( try { commitLayoutEffectOnFiber(root, current, fiber, committedLanes); } catch (error) { - reportUncaughtErrorInDEV(error); captureCommitPhaseError(fiber, fiber.return, error); } resetCurrentDebugFiberInDEV(); @@ -2481,7 +2471,6 @@ function reappearLayoutEffects_complete(subtreeRoot: Fiber) { try { reappearLayoutEffectsOnFiber(fiber); } catch (error) { - reportUncaughtErrorInDEV(error); captureCommitPhaseError(fiber, fiber.return, error); } resetCurrentDebugFiberInDEV(); @@ -2543,7 +2532,6 @@ function commitPassiveMountEffects_complete( try { commitPassiveMountOnFiber(root, fiber, committedLanes); } catch (error) { - reportUncaughtErrorInDEV(error); captureCommitPhaseError(fiber, fiber.return, error); } resetCurrentDebugFiberInDEV(); @@ -2945,7 +2933,6 @@ function invokeLayoutEffectMountInDEV(fiber: Fiber): void { try { commitHookEffectListMount(HookLayout | HookHasEffect, fiber); } catch (error) { - reportUncaughtErrorInDEV(error); captureCommitPhaseError(fiber, fiber.return, error); } break; @@ -2955,7 +2942,6 @@ function invokeLayoutEffectMountInDEV(fiber: Fiber): void { try { instance.componentDidMount(); } catch (error) { - reportUncaughtErrorInDEV(error); captureCommitPhaseError(fiber, fiber.return, error); } break; @@ -2975,7 +2961,6 @@ function invokePassiveEffectMountInDEV(fiber: Fiber): void { try { commitHookEffectListMount(HookPassive | HookHasEffect, fiber); } catch (error) { - reportUncaughtErrorInDEV(error); captureCommitPhaseError(fiber, fiber.return, error); } break; @@ -2999,7 +2984,6 @@ function invokeLayoutEffectUnmountInDEV(fiber: Fiber): void { fiber.return, ); } catch (error) { - reportUncaughtErrorInDEV(error); captureCommitPhaseError(fiber, fiber.return, error); } break; @@ -3030,7 +3014,6 @@ function invokePassiveEffectUnmountInDEV(fiber: Fiber): void { fiber.return, ); } catch (error) { - reportUncaughtErrorInDEV(error); captureCommitPhaseError(fiber, fiber.return, error); } } diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index bbd4acdf9e9be..643724724aaaa 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -180,6 +180,7 @@ import { invokePassiveEffectMountInDEV, invokeLayoutEffectUnmountInDEV, invokePassiveEffectUnmountInDEV, + reportUncaughtErrorInDEV, } from './ReactFiberCommitWork.new'; import {enqueueUpdate} from './ReactUpdateQueue.new'; import {resetContextDependencies} from './ReactFiberNewContext.new'; @@ -2567,6 +2568,7 @@ export function captureCommitPhaseError( error: mixed, ) { if (__DEV__) { + reportUncaughtErrorInDEV(error); setIsRunningInsertionEffect(false); } if (sourceFiber.tag === HostRoot) { diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js index 8e2dd32c9f166..a79e2071b25a3 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js @@ -180,6 +180,7 @@ import { invokePassiveEffectMountInDEV, invokeLayoutEffectUnmountInDEV, invokePassiveEffectUnmountInDEV, + reportUncaughtErrorInDEV, } from './ReactFiberCommitWork.old'; import {enqueueUpdate} from './ReactUpdateQueue.old'; import {resetContextDependencies} from './ReactFiberNewContext.old'; @@ -2567,6 +2568,7 @@ export function captureCommitPhaseError( error: mixed, ) { if (__DEV__) { + reportUncaughtErrorInDEV(error); setIsRunningInsertionEffect(false); } if (sourceFiber.tag === HostRoot) { From f9e6aef82880615d7d11fb9facf9edfd8c80dcf6 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Fri, 8 Apr 2022 00:34:56 -0400 Subject: [PATCH 07/10] Wrap try-catch directly around each user function This moves the try-catch from around each fiber's mutation phase to direclty around each user function (effect function, callback, etc). We already do this when unmounting because if one unmount function errors, we still need to call all the others so they can clean up their resources. Previously we didn't bother to do this for anything but unmount, because if a mount effect throws, we're going to delete that whole tree anyway. But now that we're switching from an iterative loop to a recursive one, we don't want every call frame on the stack to have a try-catch, since the error handling requires additional memory. Wrapping every user function is a bit tedious, but it's better for performance. Many of them already had try blocks around them already. --- .../src/ReactFiberCommitWork.new.js | 152 ++++++++++++------ .../src/ReactFiberCommitWork.old.js | 152 ++++++++++++------ 2 files changed, 208 insertions(+), 96 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index f4760c5d89ad6..05ea090d87881 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js @@ -1077,21 +1077,28 @@ function hideOrUnhideAllChildren(finishedWork, isHidden) { if (node.tag === HostComponent) { if (hostSubtreeRoot === null) { hostSubtreeRoot = node; - - const instance = node.stateNode; - if (isHidden) { - hideInstance(instance); - } else { - unhideInstance(node.stateNode, node.memoizedProps); + try { + const instance = node.stateNode; + if (isHidden) { + hideInstance(instance); + } else { + unhideInstance(node.stateNode, node.memoizedProps); + } + } catch (error) { + captureCommitPhaseError(finishedWork, finishedWork.return, error); } } } else if (node.tag === HostText) { if (hostSubtreeRoot === null) { - const instance = node.stateNode; - if (isHidden) { - hideTextInstance(instance); - } else { - unhideTextInstance(instance, node.memoizedProps); + try { + const instance = node.stateNode; + if (isHidden) { + hideTextInstance(instance); + } else { + unhideTextInstance(instance, node.memoizedProps); + } + } catch (error) { + captureCommitPhaseError(finishedWork, finishedWork.return, error); } } } else if ( @@ -1938,11 +1945,7 @@ function commitMutationEffects_complete(root: FiberRoot, lanes: Lanes) { while (nextEffect !== null) { const fiber = nextEffect; setCurrentDebugFiberInDEV(fiber); - try { - commitMutationEffectsOnFiber(fiber, root, lanes); - } catch (error) { - captureCommitPhaseError(fiber, fiber.return, error); - } + commitMutationEffectsOnFiber(fiber, root, lanes); resetCurrentDebugFiberInDEV(); const sibling = fiber.sibling; @@ -1975,12 +1978,19 @@ function commitMutationEffectsOnFiber( commitReconciliationEffects(finishedWork); if (flags & Update) { - commitHookEffectListUnmount( - HookInsertion | HookHasEffect, - finishedWork, - finishedWork.return, - ); - commitHookEffectListMount(HookInsertion | HookHasEffect, finishedWork); + try { + commitHookEffectListUnmount( + HookInsertion | HookHasEffect, + finishedWork, + finishedWork.return, + ); + commitHookEffectListMount( + HookInsertion | HookHasEffect, + finishedWork, + ); + } catch (error) { + captureCommitPhaseError(finishedWork, finishedWork.return, error); + } // Layout effects are destroyed during the mutation phase so that all // destroy functions for all fibers are called before any create functions. // This prevents sibling component effects from interfering with each other, @@ -1998,15 +2008,20 @@ function commitMutationEffectsOnFiber( finishedWork, finishedWork.return, ); - } finally { - recordLayoutEffectDuration(finishedWork); + } catch (error) { + captureCommitPhaseError(finishedWork, finishedWork.return, error); } + recordLayoutEffectDuration(finishedWork); } else { - commitHookEffectListUnmount( - HookLayout | HookHasEffect, - finishedWork, - finishedWork.return, - ); + try { + commitHookEffectListUnmount( + HookLayout | HookHasEffect, + finishedWork, + finishedWork.return, + ); + } catch (error) { + captureCommitPhaseError(finishedWork, finishedWork.return, error); + } } } return; @@ -2016,7 +2031,7 @@ function commitMutationEffectsOnFiber( if (flags & Ref) { if (current !== null) { - commitDetachRef(current); + safelyDetachRef(current, current.return); } } return; @@ -2026,13 +2041,17 @@ function commitMutationEffectsOnFiber( if (flags & Ref) { if (current !== null) { - commitDetachRef(current); + safelyDetachRef(current, current.return); } } if (supportsMutation) { if (flags & ContentReset) { const instance: Instance = finishedWork.stateNode; - resetTextContent(instance); + try { + resetTextContent(instance); + } catch (error) { + captureCommitPhaseError(finishedWork, finishedWork.return, error); + } } if (flags & Update) { @@ -2050,14 +2069,22 @@ function commitMutationEffectsOnFiber( const updatePayload: null | UpdatePayload = (finishedWork.updateQueue: any); finishedWork.updateQueue = null; if (updatePayload !== null) { - commitUpdate( - instance, - updatePayload, - type, - oldProps, - newProps, - finishedWork, - ); + try { + commitUpdate( + instance, + updatePayload, + type, + oldProps, + newProps, + finishedWork, + ); + } catch (error) { + captureCommitPhaseError( + finishedWork, + finishedWork.return, + error, + ); + } } } } @@ -2083,7 +2110,12 @@ function commitMutationEffectsOnFiber( // this case. const oldText: string = current !== null ? current.memoizedProps : newText; - commitTextUpdate(textInstance, oldText, newText); + + try { + commitTextUpdate(textInstance, oldText, newText); + } catch (error) { + captureCommitPhaseError(finishedWork, finishedWork.return, error); + } } } return; @@ -2096,14 +2128,26 @@ function commitMutationEffectsOnFiber( if (current !== null) { const prevRootState: RootState = current.memoizedState; if (prevRootState.isDehydrated) { - commitHydratedContainer(root.containerInfo); + try { + commitHydratedContainer(root.containerInfo); + } catch (error) { + captureCommitPhaseError( + finishedWork, + finishedWork.return, + error, + ); + } } } } if (supportsPersistence) { const containerInfo = root.containerInfo; const pendingChildren = root.pendingChildren; - replaceContainerChildren(containerInfo, pendingChildren); + try { + replaceContainerChildren(containerInfo, pendingChildren); + } catch (error) { + captureCommitPhaseError(finishedWork, finishedWork.return, error); + } } } return; @@ -2116,7 +2160,11 @@ function commitMutationEffectsOnFiber( const portal = finishedWork.stateNode; const containerInfo = portal.containerInfo; const pendingChildren = portal.pendingChildren; - replaceContainerChildren(containerInfo, pendingChildren); + try { + replaceContainerChildren(containerInfo, pendingChildren); + } catch (error) { + captureCommitPhaseError(finishedWork, finishedWork.return, error); + } } } return; @@ -2136,7 +2184,11 @@ function commitMutationEffectsOnFiber( } } if (flags & Update) { - commitSuspenseCallback(finishedWork); + try { + commitSuspenseCallback(finishedWork); + } catch (error) { + captureCommitPhaseError(finishedWork, finishedWork.return, error); + } attachSuspenseRetryListeners(finishedWork); } return; @@ -2194,9 +2246,9 @@ function commitMutationEffectsOnFiber( // from React Flare on www. if (flags & Ref) { if (current !== null) { - commitDetachRef(current); + safelyDetachRef(finishedWork, finishedWork.return); } - commitAttachRef(finishedWork); + safelyAttachRef(finishedWork, finishedWork.return); } if (flags & Update) { const scopeInstance = finishedWork.stateNode; @@ -2217,7 +2269,11 @@ function commitReconciliationEffects(finishedWork: Fiber) { // before the effects on this fiber have fired. const flags = finishedWork.flags; if (flags & Placement) { - commitPlacement(finishedWork); + try { + commitPlacement(finishedWork); + } catch (error) { + captureCommitPhaseError(finishedWork, finishedWork.return, error); + } // Clear the "placement" from effect tag so that we know that this is // inserted, before any life-cycles like componentDidMount gets called. // TODO: findDOMNode doesn't rely on this any more but isMounted does diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.old.js b/packages/react-reconciler/src/ReactFiberCommitWork.old.js index 078a5b500ff9b..49521099d384f 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.old.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.old.js @@ -1077,21 +1077,28 @@ function hideOrUnhideAllChildren(finishedWork, isHidden) { if (node.tag === HostComponent) { if (hostSubtreeRoot === null) { hostSubtreeRoot = node; - - const instance = node.stateNode; - if (isHidden) { - hideInstance(instance); - } else { - unhideInstance(node.stateNode, node.memoizedProps); + try { + const instance = node.stateNode; + if (isHidden) { + hideInstance(instance); + } else { + unhideInstance(node.stateNode, node.memoizedProps); + } + } catch (error) { + captureCommitPhaseError(finishedWork, finishedWork.return, error); } } } else if (node.tag === HostText) { if (hostSubtreeRoot === null) { - const instance = node.stateNode; - if (isHidden) { - hideTextInstance(instance); - } else { - unhideTextInstance(instance, node.memoizedProps); + try { + const instance = node.stateNode; + if (isHidden) { + hideTextInstance(instance); + } else { + unhideTextInstance(instance, node.memoizedProps); + } + } catch (error) { + captureCommitPhaseError(finishedWork, finishedWork.return, error); } } } else if ( @@ -1938,11 +1945,7 @@ function commitMutationEffects_complete(root: FiberRoot, lanes: Lanes) { while (nextEffect !== null) { const fiber = nextEffect; setCurrentDebugFiberInDEV(fiber); - try { - commitMutationEffectsOnFiber(fiber, root, lanes); - } catch (error) { - captureCommitPhaseError(fiber, fiber.return, error); - } + commitMutationEffectsOnFiber(fiber, root, lanes); resetCurrentDebugFiberInDEV(); const sibling = fiber.sibling; @@ -1975,12 +1978,19 @@ function commitMutationEffectsOnFiber( commitReconciliationEffects(finishedWork); if (flags & Update) { - commitHookEffectListUnmount( - HookInsertion | HookHasEffect, - finishedWork, - finishedWork.return, - ); - commitHookEffectListMount(HookInsertion | HookHasEffect, finishedWork); + try { + commitHookEffectListUnmount( + HookInsertion | HookHasEffect, + finishedWork, + finishedWork.return, + ); + commitHookEffectListMount( + HookInsertion | HookHasEffect, + finishedWork, + ); + } catch (error) { + captureCommitPhaseError(finishedWork, finishedWork.return, error); + } // Layout effects are destroyed during the mutation phase so that all // destroy functions for all fibers are called before any create functions. // This prevents sibling component effects from interfering with each other, @@ -1998,15 +2008,20 @@ function commitMutationEffectsOnFiber( finishedWork, finishedWork.return, ); - } finally { - recordLayoutEffectDuration(finishedWork); + } catch (error) { + captureCommitPhaseError(finishedWork, finishedWork.return, error); } + recordLayoutEffectDuration(finishedWork); } else { - commitHookEffectListUnmount( - HookLayout | HookHasEffect, - finishedWork, - finishedWork.return, - ); + try { + commitHookEffectListUnmount( + HookLayout | HookHasEffect, + finishedWork, + finishedWork.return, + ); + } catch (error) { + captureCommitPhaseError(finishedWork, finishedWork.return, error); + } } } return; @@ -2016,7 +2031,7 @@ function commitMutationEffectsOnFiber( if (flags & Ref) { if (current !== null) { - commitDetachRef(current); + safelyDetachRef(current, current.return); } } return; @@ -2026,13 +2041,17 @@ function commitMutationEffectsOnFiber( if (flags & Ref) { if (current !== null) { - commitDetachRef(current); + safelyDetachRef(current, current.return); } } if (supportsMutation) { if (flags & ContentReset) { const instance: Instance = finishedWork.stateNode; - resetTextContent(instance); + try { + resetTextContent(instance); + } catch (error) { + captureCommitPhaseError(finishedWork, finishedWork.return, error); + } } if (flags & Update) { @@ -2050,14 +2069,22 @@ function commitMutationEffectsOnFiber( const updatePayload: null | UpdatePayload = (finishedWork.updateQueue: any); finishedWork.updateQueue = null; if (updatePayload !== null) { - commitUpdate( - instance, - updatePayload, - type, - oldProps, - newProps, - finishedWork, - ); + try { + commitUpdate( + instance, + updatePayload, + type, + oldProps, + newProps, + finishedWork, + ); + } catch (error) { + captureCommitPhaseError( + finishedWork, + finishedWork.return, + error, + ); + } } } } @@ -2083,7 +2110,12 @@ function commitMutationEffectsOnFiber( // this case. const oldText: string = current !== null ? current.memoizedProps : newText; - commitTextUpdate(textInstance, oldText, newText); + + try { + commitTextUpdate(textInstance, oldText, newText); + } catch (error) { + captureCommitPhaseError(finishedWork, finishedWork.return, error); + } } } return; @@ -2096,14 +2128,26 @@ function commitMutationEffectsOnFiber( if (current !== null) { const prevRootState: RootState = current.memoizedState; if (prevRootState.isDehydrated) { - commitHydratedContainer(root.containerInfo); + try { + commitHydratedContainer(root.containerInfo); + } catch (error) { + captureCommitPhaseError( + finishedWork, + finishedWork.return, + error, + ); + } } } } if (supportsPersistence) { const containerInfo = root.containerInfo; const pendingChildren = root.pendingChildren; - replaceContainerChildren(containerInfo, pendingChildren); + try { + replaceContainerChildren(containerInfo, pendingChildren); + } catch (error) { + captureCommitPhaseError(finishedWork, finishedWork.return, error); + } } } return; @@ -2116,7 +2160,11 @@ function commitMutationEffectsOnFiber( const portal = finishedWork.stateNode; const containerInfo = portal.containerInfo; const pendingChildren = portal.pendingChildren; - replaceContainerChildren(containerInfo, pendingChildren); + try { + replaceContainerChildren(containerInfo, pendingChildren); + } catch (error) { + captureCommitPhaseError(finishedWork, finishedWork.return, error); + } } } return; @@ -2136,7 +2184,11 @@ function commitMutationEffectsOnFiber( } } if (flags & Update) { - commitSuspenseCallback(finishedWork); + try { + commitSuspenseCallback(finishedWork); + } catch (error) { + captureCommitPhaseError(finishedWork, finishedWork.return, error); + } attachSuspenseRetryListeners(finishedWork); } return; @@ -2194,9 +2246,9 @@ function commitMutationEffectsOnFiber( // from React Flare on www. if (flags & Ref) { if (current !== null) { - commitDetachRef(current); + safelyDetachRef(finishedWork, finishedWork.return); } - commitAttachRef(finishedWork); + safelyAttachRef(finishedWork, finishedWork.return); } if (flags & Update) { const scopeInstance = finishedWork.stateNode; @@ -2217,7 +2269,11 @@ function commitReconciliationEffects(finishedWork: Fiber) { // before the effects on this fiber have fired. const flags = finishedWork.flags; if (flags & Placement) { - commitPlacement(finishedWork); + try { + commitPlacement(finishedWork); + } catch (error) { + captureCommitPhaseError(finishedWork, finishedWork.return, error); + } // Clear the "placement" from effect tag so that we know that this is // inserted, before any life-cycles like componentDidMount gets called. // TODO: findDOMNode doesn't rely on this any more but isMounted does From 481dece5808d1207acd66a6a0365efee729cf0f7 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Fri, 8 Apr 2022 00:45:10 -0400 Subject: [PATCH 08/10] Use recursion to traverse during mutation phase Most of the commit phase uses iterative loops to traverse the tree. Originally we thought this would be faster than using recursion, but a while back @trueadm did some performance testing and found that the loop was slower because we assign to the `return` pointer before entering a subtree (which we have to do because the `return` pointer is not always consistent; it could point to one of two fibers). The other motivation is so we can take advantage of the JS stack to track contextual information, like the nearest host parent. We already use recursion in a few places; this changes the mutation phase to use it, too. --- .../react-reconciler/src/ReactCurrentFiber.js | 12 ++- .../src/ReactFiberCommitWork.new.js | 91 ++++++++++--------- .../src/ReactFiberCommitWork.old.js | 91 ++++++++++--------- 3 files changed, 108 insertions(+), 86 deletions(-) diff --git a/packages/react-reconciler/src/ReactCurrentFiber.js b/packages/react-reconciler/src/ReactCurrentFiber.js index b2186131a98a5..08853b384bc65 100644 --- a/packages/react-reconciler/src/ReactCurrentFiber.js +++ b/packages/react-reconciler/src/ReactCurrentFiber.js @@ -51,14 +51,22 @@ export function resetCurrentFiber() { } } -export function setCurrentFiber(fiber: Fiber) { +export function setCurrentFiber(fiber: Fiber | null) { if (__DEV__) { - ReactDebugCurrentFrame.getCurrentStack = getCurrentFiberStackInDev; + ReactDebugCurrentFrame.getCurrentStack = + fiber === null ? null : getCurrentFiberStackInDev; current = fiber; isRendering = false; } } +export function getCurrentFiber(): Fiber | null { + if (__DEV__) { + return current; + } + return null; +} + export function setIsRendering(rendering: boolean) { if (__DEV__) { isRendering = rendering; diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index 05ea090d87881..2042755449235 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js @@ -84,6 +84,7 @@ import getComponentNameFromFiber from 'react-reconciler/src/getComponentNameFrom import { resetCurrentFiber as resetCurrentDebugFiberInDEV, setCurrentFiber as setCurrentDebugFiberInDEV, + getCurrentFiber as getCurrentDebugFiberInDEV, } from './ReactCurrentFiber'; import {resolveDefaultProps} from './ReactFiberLazyComponent.new'; import { @@ -1901,62 +1902,50 @@ export function isSuspenseBoundaryBeingHidden( export function commitMutationEffects( root: FiberRoot, - firstChild: Fiber, + finishedWork: Fiber, committedLanes: Lanes, ) { inProgressLanes = committedLanes; inProgressRoot = root; - nextEffect = firstChild; + nextEffect = finishedWork; - commitMutationEffects_begin(root, committedLanes); + setCurrentDebugFiberInDEV(finishedWork); + commitMutationEffectsOnFiber(finishedWork, root, committedLanes); + setCurrentDebugFiberInDEV(finishedWork); inProgressLanes = null; inProgressRoot = null; } -function commitMutationEffects_begin(root: FiberRoot, lanes: Lanes) { - while (nextEffect !== null) { - const fiber = nextEffect; - - // TODO: Should wrap this in flags check, too, as optimization - const deletions = fiber.deletions; - if (deletions !== null) { - for (let i = 0; i < deletions.length; i++) { - const childToDelete = deletions[i]; - try { - commitDeletion(root, childToDelete, fiber); - } catch (error) { - captureCommitPhaseError(childToDelete, fiber, error); - } +function recursivelyTraverseMutationEffects( + root: FiberRoot, + parentFiber: Fiber, + lanes: Lanes, +) { + // Deletions effects can be scheduled on any fiber type. They need to happen + // before the children effects hae fired. + const deletions = parentFiber.deletions; + if (deletions !== null) { + for (let i = 0; i < deletions.length; i++) { + const childToDelete = deletions[i]; + try { + commitDeletion(root, childToDelete, parentFiber); + } catch (error) { + captureCommitPhaseError(childToDelete, parentFiber, error); } } - - const child = fiber.child; - if ((fiber.subtreeFlags & MutationMask) !== NoFlags && child !== null) { - child.return = fiber; - nextEffect = child; - } else { - commitMutationEffects_complete(root, lanes); - } } -} -function commitMutationEffects_complete(root: FiberRoot, lanes: Lanes) { - while (nextEffect !== null) { - const fiber = nextEffect; - setCurrentDebugFiberInDEV(fiber); - commitMutationEffectsOnFiber(fiber, root, lanes); - resetCurrentDebugFiberInDEV(); - - const sibling = fiber.sibling; - if (sibling !== null) { - sibling.return = fiber.return; - nextEffect = sibling; - return; + const prevDebugFiber = getCurrentDebugFiberInDEV(); + if (parentFiber.subtreeFlags & MutationMask) { + let child = parentFiber.child; + while (child !== null) { + setCurrentDebugFiberInDEV(child); + commitMutationEffectsOnFiber(child, root, lanes); + child = child.sibling; } - - nextEffect = fiber.return; } + setCurrentDebugFiberInDEV(prevDebugFiber); } function commitMutationEffectsOnFiber( @@ -1975,6 +1964,7 @@ function commitMutationEffectsOnFiber( case ForwardRef: case MemoComponent: case SimpleMemoComponent: { + recursivelyTraverseMutationEffects(root, finishedWork, lanes); commitReconciliationEffects(finishedWork); if (flags & Update) { @@ -2027,6 +2017,7 @@ function commitMutationEffectsOnFiber( return; } case ClassComponent: { + recursivelyTraverseMutationEffects(root, finishedWork, lanes); commitReconciliationEffects(finishedWork); if (flags & Ref) { @@ -2037,6 +2028,7 @@ function commitMutationEffectsOnFiber( return; } case HostComponent: { + recursivelyTraverseMutationEffects(root, finishedWork, lanes); commitReconciliationEffects(finishedWork); if (flags & Ref) { @@ -2045,7 +2037,13 @@ function commitMutationEffectsOnFiber( } } if (supportsMutation) { - if (flags & ContentReset) { + // TODO: ContentReset gets cleared by the children during the commit + // phase. This is a refactor hazard because it means we must read + // flags the flags after `commitReconciliationEffects` has already run; + // the order matters. We should refactor so that ContentReset does not + // rely on mutating the flag during commit. Like by setting a flag + // during the render phase instead. + if (finishedWork.flags & ContentReset) { const instance: Instance = finishedWork.stateNode; try { resetTextContent(instance); @@ -2092,6 +2090,7 @@ function commitMutationEffectsOnFiber( return; } case HostText: { + recursivelyTraverseMutationEffects(root, finishedWork, lanes); commitReconciliationEffects(finishedWork); if (flags & Update) { @@ -2121,6 +2120,7 @@ function commitMutationEffectsOnFiber( return; } case HostRoot: { + recursivelyTraverseMutationEffects(root, finishedWork, lanes); commitReconciliationEffects(finishedWork); if (flags & Update) { @@ -2153,6 +2153,7 @@ function commitMutationEffectsOnFiber( return; } case HostPortal: { + recursivelyTraverseMutationEffects(root, finishedWork, lanes); commitReconciliationEffects(finishedWork); if (flags & Update) { @@ -2170,6 +2171,7 @@ function commitMutationEffectsOnFiber( return; } case SuspenseComponent: { + recursivelyTraverseMutationEffects(root, finishedWork, lanes); commitReconciliationEffects(finishedWork); if (flags & Visibility) { @@ -2194,6 +2196,7 @@ function commitMutationEffectsOnFiber( return; } case OffscreenComponent: { + recursivelyTraverseMutationEffects(root, finishedWork, lanes); commitReconciliationEffects(finishedWork); if (flags & Visibility) { @@ -2231,6 +2234,7 @@ function commitMutationEffectsOnFiber( return; } case SuspenseListComponent: { + recursivelyTraverseMutationEffects(root, finishedWork, lanes); commitReconciliationEffects(finishedWork); if (flags & Update) { @@ -2240,6 +2244,7 @@ function commitMutationEffectsOnFiber( } case ScopeComponent: { if (enableScopeAPI) { + recursivelyTraverseMutationEffects(root, finishedWork, lanes); commitReconciliationEffects(finishedWork); // TODO: This is a temporary solution that allowed us to transition away @@ -2258,11 +2263,13 @@ function commitMutationEffectsOnFiber( return; } default: { + recursivelyTraverseMutationEffects(root, finishedWork, lanes); commitReconciliationEffects(finishedWork); + + return; } } } - function commitReconciliationEffects(finishedWork: Fiber) { // Placement effects (insertions, reorders) can be scheduled on any fiber // type. They needs to happen after the children effects have fired, but diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.old.js b/packages/react-reconciler/src/ReactFiberCommitWork.old.js index 49521099d384f..8c1f0b27242b2 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.old.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.old.js @@ -84,6 +84,7 @@ import getComponentNameFromFiber from 'react-reconciler/src/getComponentNameFrom import { resetCurrentFiber as resetCurrentDebugFiberInDEV, setCurrentFiber as setCurrentDebugFiberInDEV, + getCurrentFiber as getCurrentDebugFiberInDEV, } from './ReactCurrentFiber'; import {resolveDefaultProps} from './ReactFiberLazyComponent.old'; import { @@ -1901,62 +1902,50 @@ export function isSuspenseBoundaryBeingHidden( export function commitMutationEffects( root: FiberRoot, - firstChild: Fiber, + finishedWork: Fiber, committedLanes: Lanes, ) { inProgressLanes = committedLanes; inProgressRoot = root; - nextEffect = firstChild; + nextEffect = finishedWork; - commitMutationEffects_begin(root, committedLanes); + setCurrentDebugFiberInDEV(finishedWork); + commitMutationEffectsOnFiber(finishedWork, root, committedLanes); + setCurrentDebugFiberInDEV(finishedWork); inProgressLanes = null; inProgressRoot = null; } -function commitMutationEffects_begin(root: FiberRoot, lanes: Lanes) { - while (nextEffect !== null) { - const fiber = nextEffect; - - // TODO: Should wrap this in flags check, too, as optimization - const deletions = fiber.deletions; - if (deletions !== null) { - for (let i = 0; i < deletions.length; i++) { - const childToDelete = deletions[i]; - try { - commitDeletion(root, childToDelete, fiber); - } catch (error) { - captureCommitPhaseError(childToDelete, fiber, error); - } +function recursivelyTraverseMutationEffects( + root: FiberRoot, + parentFiber: Fiber, + lanes: Lanes, +) { + // Deletions effects can be scheduled on any fiber type. They need to happen + // before the children effects hae fired. + const deletions = parentFiber.deletions; + if (deletions !== null) { + for (let i = 0; i < deletions.length; i++) { + const childToDelete = deletions[i]; + try { + commitDeletion(root, childToDelete, parentFiber); + } catch (error) { + captureCommitPhaseError(childToDelete, parentFiber, error); } } - - const child = fiber.child; - if ((fiber.subtreeFlags & MutationMask) !== NoFlags && child !== null) { - child.return = fiber; - nextEffect = child; - } else { - commitMutationEffects_complete(root, lanes); - } } -} -function commitMutationEffects_complete(root: FiberRoot, lanes: Lanes) { - while (nextEffect !== null) { - const fiber = nextEffect; - setCurrentDebugFiberInDEV(fiber); - commitMutationEffectsOnFiber(fiber, root, lanes); - resetCurrentDebugFiberInDEV(); - - const sibling = fiber.sibling; - if (sibling !== null) { - sibling.return = fiber.return; - nextEffect = sibling; - return; + const prevDebugFiber = getCurrentDebugFiberInDEV(); + if (parentFiber.subtreeFlags & MutationMask) { + let child = parentFiber.child; + while (child !== null) { + setCurrentDebugFiberInDEV(child); + commitMutationEffectsOnFiber(child, root, lanes); + child = child.sibling; } - - nextEffect = fiber.return; } + setCurrentDebugFiberInDEV(prevDebugFiber); } function commitMutationEffectsOnFiber( @@ -1975,6 +1964,7 @@ function commitMutationEffectsOnFiber( case ForwardRef: case MemoComponent: case SimpleMemoComponent: { + recursivelyTraverseMutationEffects(root, finishedWork, lanes); commitReconciliationEffects(finishedWork); if (flags & Update) { @@ -2027,6 +2017,7 @@ function commitMutationEffectsOnFiber( return; } case ClassComponent: { + recursivelyTraverseMutationEffects(root, finishedWork, lanes); commitReconciliationEffects(finishedWork); if (flags & Ref) { @@ -2037,6 +2028,7 @@ function commitMutationEffectsOnFiber( return; } case HostComponent: { + recursivelyTraverseMutationEffects(root, finishedWork, lanes); commitReconciliationEffects(finishedWork); if (flags & Ref) { @@ -2045,7 +2037,13 @@ function commitMutationEffectsOnFiber( } } if (supportsMutation) { - if (flags & ContentReset) { + // TODO: ContentReset gets cleared by the children during the commit + // phase. This is a refactor hazard because it means we must read + // flags the flags after `commitReconciliationEffects` has already run; + // the order matters. We should refactor so that ContentReset does not + // rely on mutating the flag during commit. Like by setting a flag + // during the render phase instead. + if (finishedWork.flags & ContentReset) { const instance: Instance = finishedWork.stateNode; try { resetTextContent(instance); @@ -2092,6 +2090,7 @@ function commitMutationEffectsOnFiber( return; } case HostText: { + recursivelyTraverseMutationEffects(root, finishedWork, lanes); commitReconciliationEffects(finishedWork); if (flags & Update) { @@ -2121,6 +2120,7 @@ function commitMutationEffectsOnFiber( return; } case HostRoot: { + recursivelyTraverseMutationEffects(root, finishedWork, lanes); commitReconciliationEffects(finishedWork); if (flags & Update) { @@ -2153,6 +2153,7 @@ function commitMutationEffectsOnFiber( return; } case HostPortal: { + recursivelyTraverseMutationEffects(root, finishedWork, lanes); commitReconciliationEffects(finishedWork); if (flags & Update) { @@ -2170,6 +2171,7 @@ function commitMutationEffectsOnFiber( return; } case SuspenseComponent: { + recursivelyTraverseMutationEffects(root, finishedWork, lanes); commitReconciliationEffects(finishedWork); if (flags & Visibility) { @@ -2194,6 +2196,7 @@ function commitMutationEffectsOnFiber( return; } case OffscreenComponent: { + recursivelyTraverseMutationEffects(root, finishedWork, lanes); commitReconciliationEffects(finishedWork); if (flags & Visibility) { @@ -2231,6 +2234,7 @@ function commitMutationEffectsOnFiber( return; } case SuspenseListComponent: { + recursivelyTraverseMutationEffects(root, finishedWork, lanes); commitReconciliationEffects(finishedWork); if (flags & Update) { @@ -2240,6 +2244,7 @@ function commitMutationEffectsOnFiber( } case ScopeComponent: { if (enableScopeAPI) { + recursivelyTraverseMutationEffects(root, finishedWork, lanes); commitReconciliationEffects(finishedWork); // TODO: This is a temporary solution that allowed us to transition away @@ -2258,11 +2263,13 @@ function commitMutationEffectsOnFiber( return; } default: { + recursivelyTraverseMutationEffects(root, finishedWork, lanes); commitReconciliationEffects(finishedWork); + + return; } } } - function commitReconciliationEffects(finishedWork: Fiber) { // Placement effects (insertions, reorders) can be scheduled on any fiber // type. They needs to happen after the children effects have fired, but From 46db4e996d686ffbc8ec4cc7d559d6897288b780 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 6 Apr 2022 23:25:39 -0400 Subject: [PATCH 09/10] Combine deletion phase into single recursive function Similar to the previous step, this converts the deletion phase into a single recursive function. Although there's less code, this one is a bit trickier because it's already contains some stack-like logic for tracking the nearest host parent. But instead of using the actual stack, it repeatedly searches up the fiber return path to find the nearest host parent. Instead, I've changed it to track the nearest host parent on the JS stack. (We still search up the return path once, to set the initial host parent right before entering a deleted tree. As a follow up, we can instead push this to the stack as we traverse during the main mutation phase.) --- .../src/ReactFiberCommitWork.new.js | 544 +++++++++--------- .../src/ReactFiberCommitWork.old.js | 544 +++++++++--------- 2 files changed, 550 insertions(+), 538 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index 2042755449235..9fea320a69ba6 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js @@ -1222,148 +1222,6 @@ function commitDetachRef(current: Fiber) { } } -// User-originating errors (lifecycles and refs) should not interrupt -// deletion, so don't let them throw. Host-originating errors should -// interrupt deletion, so it's okay -function commitUnmount( - finishedRoot: FiberRoot, - current: Fiber, - nearestMountedAncestor: Fiber, -): void { - onCommitUnmount(current); - - switch (current.tag) { - case FunctionComponent: - case ForwardRef: - case MemoComponent: - case SimpleMemoComponent: { - const updateQueue: FunctionComponentUpdateQueue | null = (current.updateQueue: any); - if (updateQueue !== null) { - const lastEffect = updateQueue.lastEffect; - if (lastEffect !== null) { - const firstEffect = lastEffect.next; - - let effect = firstEffect; - do { - const {destroy, tag} = effect; - if (destroy !== undefined) { - if ((tag & HookInsertion) !== NoHookEffect) { - safelyCallDestroy(current, nearestMountedAncestor, destroy); - } else if ((tag & HookLayout) !== NoHookEffect) { - if (enableSchedulingProfiler) { - markComponentLayoutEffectUnmountStarted(current); - } - - if ( - enableProfilerTimer && - enableProfilerCommitHooks && - current.mode & ProfileMode - ) { - startLayoutEffectTimer(); - safelyCallDestroy(current, nearestMountedAncestor, destroy); - recordLayoutEffectDuration(current); - } else { - safelyCallDestroy(current, nearestMountedAncestor, destroy); - } - - if (enableSchedulingProfiler) { - markComponentLayoutEffectUnmountStopped(); - } - } - } - effect = effect.next; - } while (effect !== firstEffect); - } - } - return; - } - case ClassComponent: { - safelyDetachRef(current, nearestMountedAncestor); - const instance = current.stateNode; - if (typeof instance.componentWillUnmount === 'function') { - safelyCallComponentWillUnmount( - current, - nearestMountedAncestor, - instance, - ); - } - return; - } - case HostComponent: { - safelyDetachRef(current, nearestMountedAncestor); - return; - } - case HostPortal: { - // TODO: this is recursive. - // We are also not using this parent because - // the portal will get pushed immediately. - if (supportsMutation) { - unmountHostComponents(finishedRoot, current, nearestMountedAncestor); - } else if (supportsPersistence) { - emptyPortalContainer(current); - } - return; - } - case DehydratedFragment: { - if (enableSuspenseCallback) { - const hydrationCallbacks = finishedRoot.hydrationCallbacks; - if (hydrationCallbacks !== null) { - const onDeleted = hydrationCallbacks.onDeleted; - if (onDeleted) { - onDeleted((current.stateNode: SuspenseInstance)); - } - } - } - return; - } - case ScopeComponent: { - if (enableScopeAPI) { - safelyDetachRef(current, nearestMountedAncestor); - } - return; - } - } -} - -function commitNestedUnmounts( - finishedRoot: FiberRoot, - root: Fiber, - nearestMountedAncestor: Fiber, -): void { - // While we're inside a removed host node we don't want to call - // removeChild on the inner nodes because they're removed by the top - // call anyway. We also want to call componentWillUnmount on all - // composites before this host node is removed from the tree. Therefore - // we do an inner loop while we're still inside the host node. - let node: Fiber = root; - while (true) { - commitUnmount(finishedRoot, node, nearestMountedAncestor); - // Visit children because they may contain more composite or host nodes. - // Skip portals because commitUnmount() currently visits them recursively. - if ( - node.child !== null && - // If we use mutation we drill down into portals using commitUnmount above. - // If we don't use mutation we drill down into portals here instead. - (!supportsMutation || node.tag !== HostPortal) - ) { - node.child.return = node; - node = node.child; - continue; - } - if (node === root) { - return; - } - while (node.sibling === null) { - if (node.return === null || node.return === root) { - return; - } - node = node.return; - } - node.sibling.return = node.return; - node = node.sibling; - } -} - function detachFiberMutation(fiber: Fiber) { // Cut off the return pointer to disconnect it from the tree. // This enables us to detect and warn against state updates on an unmounted component. @@ -1652,152 +1510,302 @@ function insertOrAppendPlacementNode( } } -function unmountHostComponents( - finishedRoot: FiberRoot, - current: Fiber, - nearestMountedAncestor: Fiber, -): void { - // We only have the top Fiber that was deleted but we need to recurse down its - // children to find all the terminal nodes. - let node: Fiber = current; +// These are tracked on the stack as we recursively traverse a +// deleted subtree. +// TODO: Update these during the whole mutation phase, not just during +// a deletion. +let hostParent: Instance | Container | null = null; +let hostParentIsContainer: boolean = false; - // Each iteration, currentParent is populated with node's host parent if not - // currentParentIsValid. - let currentParentIsValid = false; +function commitDeletionEffects( + root: FiberRoot, + returnFiber: Fiber, + deletedFiber: Fiber, +) { + if (supportsMutation) { + // We only have the top Fiber that was deleted but we need to recurse down its + // children to find all the terminal nodes. - // Note: these two variables *must* always be updated together. - let currentParent; - let currentParentIsContainer; - - while (true) { - if (!currentParentIsValid) { - let parent = node.return; - findParent: while (true) { - if (parent === null) { - throw new Error( - 'Expected to find a host parent. This error is likely caused by ' + - 'a bug in React. Please file an issue.', - ); - } + // Recursively delete all host nodes from the parent, detach refs, clean + // up mounted layout effects, and call componentWillUnmount. + + // We only need to remove the topmost host child in each branch. But then we + // still need to keep traversing to unmount effects, refs, and cWU. TODO: We + // could split this into two separate traversals functions, where the second + // one doesn't include any removeChild logic. This is maybe the same + // function as "disappearLayoutEffects" (or whatever that turns into after + // the layout phase is refactored to use recursion). + + // Before starting, find the nearest host parent on the stack so we know + // which instance/container to remove the children from. + // TODO: Instead of searching up the fiber return path on every deletion, we + // can track the nearest host component on the JS stack as we traverse the + // tree during the commit phase. This would make insertions faster, too. + let parent = returnFiber; + findParent: while (parent !== null) { + switch (parent.tag) { + case HostComponent: { + hostParent = parent.stateNode; + hostParentIsContainer = false; + break findParent; + } + case HostRoot: { + hostParent = parent.stateNode.containerInfo; + hostParentIsContainer = true; + break findParent; + } + case HostPortal: { + hostParent = parent.stateNode.containerInfo; + hostParentIsContainer = true; + break findParent; + } + } + parent = parent.return; + } + if (hostParent === null) { + throw new Error( + 'Expected to find a host parent. This error is likely caused by ' + + 'a bug in React. Please file an issue.', + ); + } + commitDeletionEffectsOnFiber(root, returnFiber, deletedFiber); + hostParent = null; + hostParentIsContainer = false; + } else { + // Detach refs and call componentWillUnmount() on the whole subtree. + commitDeletionEffectsOnFiber(root, returnFiber, deletedFiber); + } - const parentStateNode = parent.stateNode; - switch (parent.tag) { - case HostComponent: - currentParent = parentStateNode; - currentParentIsContainer = false; - break findParent; - case HostRoot: - currentParent = parentStateNode.containerInfo; - currentParentIsContainer = true; - break findParent; - case HostPortal: - currentParent = parentStateNode.containerInfo; - currentParentIsContainer = true; - break findParent; - } - parent = parent.return; - } - currentParentIsValid = true; - } - - if (node.tag === HostComponent || node.tag === HostText) { - commitNestedUnmounts(finishedRoot, node, nearestMountedAncestor); - // After all the children have unmounted, it is now safe to remove the - // node from the tree. - if (currentParentIsContainer) { - removeChildFromContainer( - ((currentParent: any): Container), - (node.stateNode: Instance | TextInstance), + detachFiberMutation(deletedFiber); +} + +function recursivelyTraverseDeletionEffects( + finishedRoot, + nearestMountedAncestor, + parent, +) { + // TODO: Use a static flag to skip trees that don't have unmount effects + let child = parent.child; + while (child !== null) { + commitDeletionEffectsOnFiber(finishedRoot, nearestMountedAncestor, child); + child = child.sibling; + } +} + +function commitDeletionEffectsOnFiber( + finishedRoot: FiberRoot, + nearestMountedAncestor: Fiber, + deletedFiber: Fiber, +) { + onCommitUnmount(deletedFiber); + + // The cases in this outer switch modify the stack before they traverse + // into their subtree. There are simpler cases in the inner switch + // that don't modify the stack. + switch (deletedFiber.tag) { + case HostComponent: { + safelyDetachRef(deletedFiber, nearestMountedAncestor); + // Intentional fallthrough to next branch + } + // eslint-disable-next-line-no-fallthrough + case HostText: { + // We only need to remove the nearest host child. Set the host parent + // to `null` on the stack to indicate that nested children don't + // need to be removed. + if (supportsMutation) { + const prevHostParent = hostParent; + const prevHostParentIsContainer = hostParentIsContainer; + hostParent = null; + recursivelyTraverseDeletionEffects( + finishedRoot, + nearestMountedAncestor, + deletedFiber, ); + hostParent = prevHostParent; + hostParentIsContainer = prevHostParentIsContainer; + + if (hostParent !== null) { + // Now that all the child effects have unmounted, we can remove the + // node from the tree. + if (hostParentIsContainer) { + removeChildFromContainer( + ((hostParent: any): Container), + (deletedFiber.stateNode: Instance | TextInstance), + ); + } else { + removeChild( + ((hostParent: any): Instance), + (deletedFiber.stateNode: Instance | TextInstance), + ); + } + } } else { - removeChild( - ((currentParent: any): Instance), - (node.stateNode: Instance | TextInstance), + recursivelyTraverseDeletionEffects( + finishedRoot, + nearestMountedAncestor, + deletedFiber, ); } - // Don't visit children because we already visited them. - } else if ( - enableSuspenseServerRenderer && - node.tag === DehydratedFragment - ) { - if (enableSuspenseCallback) { - const hydrationCallbacks = finishedRoot.hydrationCallbacks; - if (hydrationCallbacks !== null) { - const onDeleted = hydrationCallbacks.onDeleted; - if (onDeleted) { - onDeleted((node.stateNode: SuspenseInstance)); + return; + } + case DehydratedFragment: { + if (enableSuspenseServerRenderer) { + if (enableSuspenseCallback) { + const hydrationCallbacks = finishedRoot.hydrationCallbacks; + if (hydrationCallbacks !== null) { + const onDeleted = hydrationCallbacks.onDeleted; + if (onDeleted) { + onDeleted((deletedFiber.stateNode: SuspenseInstance)); + } } } - } - // Delete the dehydrated suspense boundary and all of its content. - if (currentParentIsContainer) { - clearSuspenseBoundaryFromContainer( - ((currentParent: any): Container), - (node.stateNode: SuspenseInstance), + // Dehydrated fragments don't have any children + + // Delete the dehydrated suspense boundary and all of its content. + if (supportsMutation) { + if (hostParent !== null) { + if (hostParentIsContainer) { + clearSuspenseBoundaryFromContainer( + ((hostParent: any): Container), + (deletedFiber.stateNode: SuspenseInstance), + ); + } else { + clearSuspenseBoundary( + ((hostParent: any): Instance), + (deletedFiber.stateNode: SuspenseInstance), + ); + } + } + } + } + return; + } + case HostPortal: { + if (supportsMutation) { + // When we go into a portal, it becomes the parent to remove from. + const prevHostParent = hostParent; + const prevHostParentIsContainer = hostParentIsContainer; + hostParent = deletedFiber.stateNode.containerInfo; + hostParentIsContainer = true; + recursivelyTraverseDeletionEffects( + finishedRoot, + nearestMountedAncestor, + deletedFiber, ); + hostParent = prevHostParent; + hostParentIsContainer = prevHostParentIsContainer; } else { - clearSuspenseBoundary( - ((currentParent: any): Instance), - (node.stateNode: SuspenseInstance), + emptyPortalContainer(deletedFiber); + + recursivelyTraverseDeletionEffects( + finishedRoot, + nearestMountedAncestor, + deletedFiber, ); } - } else if (node.tag === HostPortal) { - if (node.child !== null) { - // When we go into a portal, it becomes the parent to remove from. - // We will reassign it back when we pop the portal on the way up. - currentParent = node.stateNode.containerInfo; - currentParentIsContainer = true; - // Visit children because portals might contain host components. - node.child.return = node; - node = node.child; - continue; - } - } else { - commitUnmount(finishedRoot, node, nearestMountedAncestor); - // Visit children because we may find more host components below. - if (node.child !== null) { - node.child.return = node; - node = node.child; - continue; - } + return; } - if (node === current) { + case FunctionComponent: + case ForwardRef: + case MemoComponent: + case SimpleMemoComponent: { + const updateQueue: FunctionComponentUpdateQueue | null = (deletedFiber.updateQueue: any); + if (updateQueue !== null) { + const lastEffect = updateQueue.lastEffect; + if (lastEffect !== null) { + const firstEffect = lastEffect.next; + + let effect = firstEffect; + do { + const {destroy, tag} = effect; + if (destroy !== undefined) { + if ((tag & HookInsertion) !== NoHookEffect) { + safelyCallDestroy( + deletedFiber, + nearestMountedAncestor, + destroy, + ); + } else if ((tag & HookLayout) !== NoHookEffect) { + if (enableSchedulingProfiler) { + markComponentLayoutEffectUnmountStarted(deletedFiber); + } + + if ( + enableProfilerTimer && + enableProfilerCommitHooks && + deletedFiber.mode & ProfileMode + ) { + startLayoutEffectTimer(); + safelyCallDestroy( + deletedFiber, + nearestMountedAncestor, + destroy, + ); + recordLayoutEffectDuration(deletedFiber); + } else { + safelyCallDestroy( + deletedFiber, + nearestMountedAncestor, + destroy, + ); + } + + if (enableSchedulingProfiler) { + markComponentLayoutEffectUnmountStopped(); + } + } + } + effect = effect.next; + } while (effect !== firstEffect); + } + } + + recursivelyTraverseDeletionEffects( + finishedRoot, + nearestMountedAncestor, + deletedFiber, + ); return; } - while (node.sibling === null) { - if (node.return === null || node.return === current) { - return; + case ClassComponent: { + safelyDetachRef(deletedFiber, nearestMountedAncestor); + const instance = deletedFiber.stateNode; + if (typeof instance.componentWillUnmount === 'function') { + safelyCallComponentWillUnmount( + deletedFiber, + nearestMountedAncestor, + instance, + ); } - node = node.return; - if (node.tag === HostPortal) { - // When we go out of the portal, we need to restore the parent. - // Since we don't keep a stack of them, we will search for it. - currentParentIsValid = false; + recursivelyTraverseDeletionEffects( + finishedRoot, + nearestMountedAncestor, + deletedFiber, + ); + return; + } + case ScopeComponent: { + if (enableScopeAPI) { + safelyDetachRef(deletedFiber, nearestMountedAncestor); } + recursivelyTraverseDeletionEffects( + finishedRoot, + nearestMountedAncestor, + deletedFiber, + ); + return; + } + default: { + recursivelyTraverseDeletionEffects( + finishedRoot, + nearestMountedAncestor, + deletedFiber, + ); + return; } - node.sibling.return = node.return; - node = node.sibling; - } -} - -function commitDeletion( - finishedRoot: FiberRoot, - current: Fiber, - nearestMountedAncestor: Fiber, -): void { - if (supportsMutation) { - // Recursively delete all host nodes from the parent. - // Detach refs and call componentWillUnmount() on the whole subtree. - unmountHostComponents(finishedRoot, current, nearestMountedAncestor); - } else { - // Detach refs and call componentWillUnmount() on the whole subtree. - commitNestedUnmounts(finishedRoot, current, nearestMountedAncestor); } - - detachFiberMutation(current); } - function commitSuspenseCallback(finishedWork: Fiber) { // TODO: Move this to passive phase const newState: SuspenseState | null = finishedWork.memoizedState; @@ -1907,7 +1915,6 @@ export function commitMutationEffects( ) { inProgressLanes = committedLanes; inProgressRoot = root; - nextEffect = finishedWork; setCurrentDebugFiberInDEV(finishedWork); commitMutationEffectsOnFiber(finishedWork, root, committedLanes); @@ -1929,7 +1936,7 @@ function recursivelyTraverseMutationEffects( for (let i = 0; i < deletions.length; i++) { const childToDelete = deletions[i]; try { - commitDeletion(root, childToDelete, parentFiber); + commitDeletionEffects(root, parentFiber, childToDelete); } catch (error) { captureCommitPhaseError(childToDelete, parentFiber, error); } @@ -3086,7 +3093,6 @@ function invokePassiveEffectUnmountInDEV(fiber: Fiber): void { export { commitPlacement, - commitDeletion, commitAttachRef, commitDetachRef, invokeLayoutEffectMountInDEV, diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.old.js b/packages/react-reconciler/src/ReactFiberCommitWork.old.js index 8c1f0b27242b2..2f900921f099d 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.old.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.old.js @@ -1222,148 +1222,6 @@ function commitDetachRef(current: Fiber) { } } -// User-originating errors (lifecycles and refs) should not interrupt -// deletion, so don't let them throw. Host-originating errors should -// interrupt deletion, so it's okay -function commitUnmount( - finishedRoot: FiberRoot, - current: Fiber, - nearestMountedAncestor: Fiber, -): void { - onCommitUnmount(current); - - switch (current.tag) { - case FunctionComponent: - case ForwardRef: - case MemoComponent: - case SimpleMemoComponent: { - const updateQueue: FunctionComponentUpdateQueue | null = (current.updateQueue: any); - if (updateQueue !== null) { - const lastEffect = updateQueue.lastEffect; - if (lastEffect !== null) { - const firstEffect = lastEffect.next; - - let effect = firstEffect; - do { - const {destroy, tag} = effect; - if (destroy !== undefined) { - if ((tag & HookInsertion) !== NoHookEffect) { - safelyCallDestroy(current, nearestMountedAncestor, destroy); - } else if ((tag & HookLayout) !== NoHookEffect) { - if (enableSchedulingProfiler) { - markComponentLayoutEffectUnmountStarted(current); - } - - if ( - enableProfilerTimer && - enableProfilerCommitHooks && - current.mode & ProfileMode - ) { - startLayoutEffectTimer(); - safelyCallDestroy(current, nearestMountedAncestor, destroy); - recordLayoutEffectDuration(current); - } else { - safelyCallDestroy(current, nearestMountedAncestor, destroy); - } - - if (enableSchedulingProfiler) { - markComponentLayoutEffectUnmountStopped(); - } - } - } - effect = effect.next; - } while (effect !== firstEffect); - } - } - return; - } - case ClassComponent: { - safelyDetachRef(current, nearestMountedAncestor); - const instance = current.stateNode; - if (typeof instance.componentWillUnmount === 'function') { - safelyCallComponentWillUnmount( - current, - nearestMountedAncestor, - instance, - ); - } - return; - } - case HostComponent: { - safelyDetachRef(current, nearestMountedAncestor); - return; - } - case HostPortal: { - // TODO: this is recursive. - // We are also not using this parent because - // the portal will get pushed immediately. - if (supportsMutation) { - unmountHostComponents(finishedRoot, current, nearestMountedAncestor); - } else if (supportsPersistence) { - emptyPortalContainer(current); - } - return; - } - case DehydratedFragment: { - if (enableSuspenseCallback) { - const hydrationCallbacks = finishedRoot.hydrationCallbacks; - if (hydrationCallbacks !== null) { - const onDeleted = hydrationCallbacks.onDeleted; - if (onDeleted) { - onDeleted((current.stateNode: SuspenseInstance)); - } - } - } - return; - } - case ScopeComponent: { - if (enableScopeAPI) { - safelyDetachRef(current, nearestMountedAncestor); - } - return; - } - } -} - -function commitNestedUnmounts( - finishedRoot: FiberRoot, - root: Fiber, - nearestMountedAncestor: Fiber, -): void { - // While we're inside a removed host node we don't want to call - // removeChild on the inner nodes because they're removed by the top - // call anyway. We also want to call componentWillUnmount on all - // composites before this host node is removed from the tree. Therefore - // we do an inner loop while we're still inside the host node. - let node: Fiber = root; - while (true) { - commitUnmount(finishedRoot, node, nearestMountedAncestor); - // Visit children because they may contain more composite or host nodes. - // Skip portals because commitUnmount() currently visits them recursively. - if ( - node.child !== null && - // If we use mutation we drill down into portals using commitUnmount above. - // If we don't use mutation we drill down into portals here instead. - (!supportsMutation || node.tag !== HostPortal) - ) { - node.child.return = node; - node = node.child; - continue; - } - if (node === root) { - return; - } - while (node.sibling === null) { - if (node.return === null || node.return === root) { - return; - } - node = node.return; - } - node.sibling.return = node.return; - node = node.sibling; - } -} - function detachFiberMutation(fiber: Fiber) { // Cut off the return pointer to disconnect it from the tree. // This enables us to detect and warn against state updates on an unmounted component. @@ -1652,152 +1510,302 @@ function insertOrAppendPlacementNode( } } -function unmountHostComponents( - finishedRoot: FiberRoot, - current: Fiber, - nearestMountedAncestor: Fiber, -): void { - // We only have the top Fiber that was deleted but we need to recurse down its - // children to find all the terminal nodes. - let node: Fiber = current; +// These are tracked on the stack as we recursively traverse a +// deleted subtree. +// TODO: Update these during the whole mutation phase, not just during +// a deletion. +let hostParent: Instance | Container | null = null; +let hostParentIsContainer: boolean = false; - // Each iteration, currentParent is populated with node's host parent if not - // currentParentIsValid. - let currentParentIsValid = false; +function commitDeletionEffects( + root: FiberRoot, + returnFiber: Fiber, + deletedFiber: Fiber, +) { + if (supportsMutation) { + // We only have the top Fiber that was deleted but we need to recurse down its + // children to find all the terminal nodes. - // Note: these two variables *must* always be updated together. - let currentParent; - let currentParentIsContainer; - - while (true) { - if (!currentParentIsValid) { - let parent = node.return; - findParent: while (true) { - if (parent === null) { - throw new Error( - 'Expected to find a host parent. This error is likely caused by ' + - 'a bug in React. Please file an issue.', - ); - } + // Recursively delete all host nodes from the parent, detach refs, clean + // up mounted layout effects, and call componentWillUnmount. + + // We only need to remove the topmost host child in each branch. But then we + // still need to keep traversing to unmount effects, refs, and cWU. TODO: We + // could split this into two separate traversals functions, where the second + // one doesn't include any removeChild logic. This is maybe the same + // function as "disappearLayoutEffects" (or whatever that turns into after + // the layout phase is refactored to use recursion). + + // Before starting, find the nearest host parent on the stack so we know + // which instance/container to remove the children from. + // TODO: Instead of searching up the fiber return path on every deletion, we + // can track the nearest host component on the JS stack as we traverse the + // tree during the commit phase. This would make insertions faster, too. + let parent = returnFiber; + findParent: while (parent !== null) { + switch (parent.tag) { + case HostComponent: { + hostParent = parent.stateNode; + hostParentIsContainer = false; + break findParent; + } + case HostRoot: { + hostParent = parent.stateNode.containerInfo; + hostParentIsContainer = true; + break findParent; + } + case HostPortal: { + hostParent = parent.stateNode.containerInfo; + hostParentIsContainer = true; + break findParent; + } + } + parent = parent.return; + } + if (hostParent === null) { + throw new Error( + 'Expected to find a host parent. This error is likely caused by ' + + 'a bug in React. Please file an issue.', + ); + } + commitDeletionEffectsOnFiber(root, returnFiber, deletedFiber); + hostParent = null; + hostParentIsContainer = false; + } else { + // Detach refs and call componentWillUnmount() on the whole subtree. + commitDeletionEffectsOnFiber(root, returnFiber, deletedFiber); + } - const parentStateNode = parent.stateNode; - switch (parent.tag) { - case HostComponent: - currentParent = parentStateNode; - currentParentIsContainer = false; - break findParent; - case HostRoot: - currentParent = parentStateNode.containerInfo; - currentParentIsContainer = true; - break findParent; - case HostPortal: - currentParent = parentStateNode.containerInfo; - currentParentIsContainer = true; - break findParent; - } - parent = parent.return; - } - currentParentIsValid = true; - } - - if (node.tag === HostComponent || node.tag === HostText) { - commitNestedUnmounts(finishedRoot, node, nearestMountedAncestor); - // After all the children have unmounted, it is now safe to remove the - // node from the tree. - if (currentParentIsContainer) { - removeChildFromContainer( - ((currentParent: any): Container), - (node.stateNode: Instance | TextInstance), + detachFiberMutation(deletedFiber); +} + +function recursivelyTraverseDeletionEffects( + finishedRoot, + nearestMountedAncestor, + parent, +) { + // TODO: Use a static flag to skip trees that don't have unmount effects + let child = parent.child; + while (child !== null) { + commitDeletionEffectsOnFiber(finishedRoot, nearestMountedAncestor, child); + child = child.sibling; + } +} + +function commitDeletionEffectsOnFiber( + finishedRoot: FiberRoot, + nearestMountedAncestor: Fiber, + deletedFiber: Fiber, +) { + onCommitUnmount(deletedFiber); + + // The cases in this outer switch modify the stack before they traverse + // into their subtree. There are simpler cases in the inner switch + // that don't modify the stack. + switch (deletedFiber.tag) { + case HostComponent: { + safelyDetachRef(deletedFiber, nearestMountedAncestor); + // Intentional fallthrough to next branch + } + // eslint-disable-next-line-no-fallthrough + case HostText: { + // We only need to remove the nearest host child. Set the host parent + // to `null` on the stack to indicate that nested children don't + // need to be removed. + if (supportsMutation) { + const prevHostParent = hostParent; + const prevHostParentIsContainer = hostParentIsContainer; + hostParent = null; + recursivelyTraverseDeletionEffects( + finishedRoot, + nearestMountedAncestor, + deletedFiber, ); + hostParent = prevHostParent; + hostParentIsContainer = prevHostParentIsContainer; + + if (hostParent !== null) { + // Now that all the child effects have unmounted, we can remove the + // node from the tree. + if (hostParentIsContainer) { + removeChildFromContainer( + ((hostParent: any): Container), + (deletedFiber.stateNode: Instance | TextInstance), + ); + } else { + removeChild( + ((hostParent: any): Instance), + (deletedFiber.stateNode: Instance | TextInstance), + ); + } + } } else { - removeChild( - ((currentParent: any): Instance), - (node.stateNode: Instance | TextInstance), + recursivelyTraverseDeletionEffects( + finishedRoot, + nearestMountedAncestor, + deletedFiber, ); } - // Don't visit children because we already visited them. - } else if ( - enableSuspenseServerRenderer && - node.tag === DehydratedFragment - ) { - if (enableSuspenseCallback) { - const hydrationCallbacks = finishedRoot.hydrationCallbacks; - if (hydrationCallbacks !== null) { - const onDeleted = hydrationCallbacks.onDeleted; - if (onDeleted) { - onDeleted((node.stateNode: SuspenseInstance)); + return; + } + case DehydratedFragment: { + if (enableSuspenseServerRenderer) { + if (enableSuspenseCallback) { + const hydrationCallbacks = finishedRoot.hydrationCallbacks; + if (hydrationCallbacks !== null) { + const onDeleted = hydrationCallbacks.onDeleted; + if (onDeleted) { + onDeleted((deletedFiber.stateNode: SuspenseInstance)); + } } } - } - // Delete the dehydrated suspense boundary and all of its content. - if (currentParentIsContainer) { - clearSuspenseBoundaryFromContainer( - ((currentParent: any): Container), - (node.stateNode: SuspenseInstance), + // Dehydrated fragments don't have any children + + // Delete the dehydrated suspense boundary and all of its content. + if (supportsMutation) { + if (hostParent !== null) { + if (hostParentIsContainer) { + clearSuspenseBoundaryFromContainer( + ((hostParent: any): Container), + (deletedFiber.stateNode: SuspenseInstance), + ); + } else { + clearSuspenseBoundary( + ((hostParent: any): Instance), + (deletedFiber.stateNode: SuspenseInstance), + ); + } + } + } + } + return; + } + case HostPortal: { + if (supportsMutation) { + // When we go into a portal, it becomes the parent to remove from. + const prevHostParent = hostParent; + const prevHostParentIsContainer = hostParentIsContainer; + hostParent = deletedFiber.stateNode.containerInfo; + hostParentIsContainer = true; + recursivelyTraverseDeletionEffects( + finishedRoot, + nearestMountedAncestor, + deletedFiber, ); + hostParent = prevHostParent; + hostParentIsContainer = prevHostParentIsContainer; } else { - clearSuspenseBoundary( - ((currentParent: any): Instance), - (node.stateNode: SuspenseInstance), + emptyPortalContainer(deletedFiber); + + recursivelyTraverseDeletionEffects( + finishedRoot, + nearestMountedAncestor, + deletedFiber, ); } - } else if (node.tag === HostPortal) { - if (node.child !== null) { - // When we go into a portal, it becomes the parent to remove from. - // We will reassign it back when we pop the portal on the way up. - currentParent = node.stateNode.containerInfo; - currentParentIsContainer = true; - // Visit children because portals might contain host components. - node.child.return = node; - node = node.child; - continue; - } - } else { - commitUnmount(finishedRoot, node, nearestMountedAncestor); - // Visit children because we may find more host components below. - if (node.child !== null) { - node.child.return = node; - node = node.child; - continue; - } + return; } - if (node === current) { + case FunctionComponent: + case ForwardRef: + case MemoComponent: + case SimpleMemoComponent: { + const updateQueue: FunctionComponentUpdateQueue | null = (deletedFiber.updateQueue: any); + if (updateQueue !== null) { + const lastEffect = updateQueue.lastEffect; + if (lastEffect !== null) { + const firstEffect = lastEffect.next; + + let effect = firstEffect; + do { + const {destroy, tag} = effect; + if (destroy !== undefined) { + if ((tag & HookInsertion) !== NoHookEffect) { + safelyCallDestroy( + deletedFiber, + nearestMountedAncestor, + destroy, + ); + } else if ((tag & HookLayout) !== NoHookEffect) { + if (enableSchedulingProfiler) { + markComponentLayoutEffectUnmountStarted(deletedFiber); + } + + if ( + enableProfilerTimer && + enableProfilerCommitHooks && + deletedFiber.mode & ProfileMode + ) { + startLayoutEffectTimer(); + safelyCallDestroy( + deletedFiber, + nearestMountedAncestor, + destroy, + ); + recordLayoutEffectDuration(deletedFiber); + } else { + safelyCallDestroy( + deletedFiber, + nearestMountedAncestor, + destroy, + ); + } + + if (enableSchedulingProfiler) { + markComponentLayoutEffectUnmountStopped(); + } + } + } + effect = effect.next; + } while (effect !== firstEffect); + } + } + + recursivelyTraverseDeletionEffects( + finishedRoot, + nearestMountedAncestor, + deletedFiber, + ); return; } - while (node.sibling === null) { - if (node.return === null || node.return === current) { - return; + case ClassComponent: { + safelyDetachRef(deletedFiber, nearestMountedAncestor); + const instance = deletedFiber.stateNode; + if (typeof instance.componentWillUnmount === 'function') { + safelyCallComponentWillUnmount( + deletedFiber, + nearestMountedAncestor, + instance, + ); } - node = node.return; - if (node.tag === HostPortal) { - // When we go out of the portal, we need to restore the parent. - // Since we don't keep a stack of them, we will search for it. - currentParentIsValid = false; + recursivelyTraverseDeletionEffects( + finishedRoot, + nearestMountedAncestor, + deletedFiber, + ); + return; + } + case ScopeComponent: { + if (enableScopeAPI) { + safelyDetachRef(deletedFiber, nearestMountedAncestor); } + recursivelyTraverseDeletionEffects( + finishedRoot, + nearestMountedAncestor, + deletedFiber, + ); + return; + } + default: { + recursivelyTraverseDeletionEffects( + finishedRoot, + nearestMountedAncestor, + deletedFiber, + ); + return; } - node.sibling.return = node.return; - node = node.sibling; - } -} - -function commitDeletion( - finishedRoot: FiberRoot, - current: Fiber, - nearestMountedAncestor: Fiber, -): void { - if (supportsMutation) { - // Recursively delete all host nodes from the parent. - // Detach refs and call componentWillUnmount() on the whole subtree. - unmountHostComponents(finishedRoot, current, nearestMountedAncestor); - } else { - // Detach refs and call componentWillUnmount() on the whole subtree. - commitNestedUnmounts(finishedRoot, current, nearestMountedAncestor); } - - detachFiberMutation(current); } - function commitSuspenseCallback(finishedWork: Fiber) { // TODO: Move this to passive phase const newState: SuspenseState | null = finishedWork.memoizedState; @@ -1907,7 +1915,6 @@ export function commitMutationEffects( ) { inProgressLanes = committedLanes; inProgressRoot = root; - nextEffect = finishedWork; setCurrentDebugFiberInDEV(finishedWork); commitMutationEffectsOnFiber(finishedWork, root, committedLanes); @@ -1929,7 +1936,7 @@ function recursivelyTraverseMutationEffects( for (let i = 0; i < deletions.length; i++) { const childToDelete = deletions[i]; try { - commitDeletion(root, childToDelete, parentFiber); + commitDeletionEffects(root, parentFiber, childToDelete); } catch (error) { captureCommitPhaseError(childToDelete, parentFiber, error); } @@ -3086,7 +3093,6 @@ function invokePassiveEffectUnmountInDEV(fiber: Fiber): void { export { commitPlacement, - commitDeletion, commitAttachRef, commitDetachRef, invokeLayoutEffectMountInDEV, From ec52a5698e2dfea7050a0b015f0b79abfb2d81b7 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 6 Apr 2022 23:47:17 -0400 Subject: [PATCH 10/10] Fix: Don't call cWU if already unmounted When a tree goes offscreen, we unmount all the effects just like we would in a normal deletion. (Conceptually it _is_ a deletion; we keep the fiber around so we can reuse its state if the tree mounts again.) If an offscreen component gets deleted "for real", we shouldn't unmount it again. The fix is to track on the stack whether we're inside a hidden tree. We already had a stack variable for this purpose, called `offscreenSubtreeWasHidden`, in another part of the commit phase, so I reused that variable instead of creating a new one. (The name is a bit confusing: "was" refers to the current tree before this commit. So, the "previous current".) Co-authored-by: dan --- .../src/ReactFiberCommitWork.new.js | 135 +++--- .../src/ReactFiberCommitWork.old.js | 135 +++--- .../ReactSuspenseEffectsSemantics-test.js | 1 - .../ReactSuspenseEffectsSemanticsDOM-test.js | 386 +++++++++++++++++- 4 files changed, 555 insertions(+), 102 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index 9fea320a69ba6..8f7741f74ee1f 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js @@ -1604,7 +1604,9 @@ function commitDeletionEffectsOnFiber( // that don't modify the stack. switch (deletedFiber.tag) { case HostComponent: { - safelyDetachRef(deletedFiber, nearestMountedAncestor); + if (!offscreenSubtreeWasHidden) { + safelyDetachRef(deletedFiber, nearestMountedAncestor); + } // Intentional fallthrough to next branch } // eslint-disable-next-line-no-fallthrough @@ -1710,54 +1712,56 @@ function commitDeletionEffectsOnFiber( case ForwardRef: case MemoComponent: case SimpleMemoComponent: { - const updateQueue: FunctionComponentUpdateQueue | null = (deletedFiber.updateQueue: any); - if (updateQueue !== null) { - const lastEffect = updateQueue.lastEffect; - if (lastEffect !== null) { - const firstEffect = lastEffect.next; - - let effect = firstEffect; - do { - const {destroy, tag} = effect; - if (destroy !== undefined) { - if ((tag & HookInsertion) !== NoHookEffect) { - safelyCallDestroy( - deletedFiber, - nearestMountedAncestor, - destroy, - ); - } else if ((tag & HookLayout) !== NoHookEffect) { - if (enableSchedulingProfiler) { - markComponentLayoutEffectUnmountStarted(deletedFiber); - } - - if ( - enableProfilerTimer && - enableProfilerCommitHooks && - deletedFiber.mode & ProfileMode - ) { - startLayoutEffectTimer(); - safelyCallDestroy( - deletedFiber, - nearestMountedAncestor, - destroy, - ); - recordLayoutEffectDuration(deletedFiber); - } else { + if (!offscreenSubtreeWasHidden) { + const updateQueue: FunctionComponentUpdateQueue | null = (deletedFiber.updateQueue: any); + if (updateQueue !== null) { + const lastEffect = updateQueue.lastEffect; + if (lastEffect !== null) { + const firstEffect = lastEffect.next; + + let effect = firstEffect; + do { + const {destroy, tag} = effect; + if (destroy !== undefined) { + if ((tag & HookInsertion) !== NoHookEffect) { safelyCallDestroy( deletedFiber, nearestMountedAncestor, destroy, ); - } + } else if ((tag & HookLayout) !== NoHookEffect) { + if (enableSchedulingProfiler) { + markComponentLayoutEffectUnmountStarted(deletedFiber); + } - if (enableSchedulingProfiler) { - markComponentLayoutEffectUnmountStopped(); + if ( + enableProfilerTimer && + enableProfilerCommitHooks && + deletedFiber.mode & ProfileMode + ) { + startLayoutEffectTimer(); + safelyCallDestroy( + deletedFiber, + nearestMountedAncestor, + destroy, + ); + recordLayoutEffectDuration(deletedFiber); + } else { + safelyCallDestroy( + deletedFiber, + nearestMountedAncestor, + destroy, + ); + } + + if (enableSchedulingProfiler) { + markComponentLayoutEffectUnmountStopped(); + } } } - } - effect = effect.next; - } while (effect !== firstEffect); + effect = effect.next; + } while (effect !== firstEffect); + } } } @@ -1769,14 +1773,16 @@ function commitDeletionEffectsOnFiber( return; } case ClassComponent: { - safelyDetachRef(deletedFiber, nearestMountedAncestor); - const instance = deletedFiber.stateNode; - if (typeof instance.componentWillUnmount === 'function') { - safelyCallComponentWillUnmount( - deletedFiber, - nearestMountedAncestor, - instance, - ); + if (!offscreenSubtreeWasHidden) { + safelyDetachRef(deletedFiber, nearestMountedAncestor); + const instance = deletedFiber.stateNode; + if (typeof instance.componentWillUnmount === 'function') { + safelyCallComponentWillUnmount( + deletedFiber, + nearestMountedAncestor, + instance, + ); + } } recursivelyTraverseDeletionEffects( finishedRoot, @@ -1796,6 +1802,27 @@ function commitDeletionEffectsOnFiber( ); return; } + case OffscreenComponent: { + // If this offscreen component is hidden, we already unmounted it. Before + // deleting the children, track that it's already unmounted so that we + // don't attempt to unmount the effects again. + // TODO: If the tree is hidden, in most cases we should be able to skip + // over the nested children entirely. An exception is we haven't yet found + // the topmost host node to delete, which we already track on the stack. + // But the other case is portals, which need to be detached no matter how + // deeply they are nested. We should use a subtree flag to track whether a + // subtree includes a nested portal. + const prevOffscreenSubtreeWasHidden = offscreenSubtreeWasHidden; + offscreenSubtreeWasHidden = + prevOffscreenSubtreeWasHidden || deletedFiber.memoizedState !== null; + recursivelyTraverseDeletionEffects( + finishedRoot, + nearestMountedAncestor, + deletedFiber, + ); + offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden; + break; + } default: { recursivelyTraverseDeletionEffects( finishedRoot, @@ -2203,13 +2230,21 @@ function commitMutationEffectsOnFiber( return; } case OffscreenComponent: { + const wasHidden = current !== null && current.memoizedState !== null; + + // Before committing the children, track on the stack whether this + // offscreen subtree was already hidden, so that we don't unmount the + // effects again. + const prevOffscreenSubtreeWasHidden = offscreenSubtreeWasHidden; + offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden || wasHidden; recursivelyTraverseMutationEffects(root, finishedWork, lanes); + offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden; + commitReconciliationEffects(finishedWork); if (flags & Visibility) { const newState: OffscreenState | null = finishedWork.memoizedState; const isHidden = newState !== null; - const wasHidden = current !== null && current.memoizedState !== null; const offscreenBoundary: Fiber = finishedWork; if (supportsMutation) { diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.old.js b/packages/react-reconciler/src/ReactFiberCommitWork.old.js index 2f900921f099d..70b416b3a3224 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.old.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.old.js @@ -1604,7 +1604,9 @@ function commitDeletionEffectsOnFiber( // that don't modify the stack. switch (deletedFiber.tag) { case HostComponent: { - safelyDetachRef(deletedFiber, nearestMountedAncestor); + if (!offscreenSubtreeWasHidden) { + safelyDetachRef(deletedFiber, nearestMountedAncestor); + } // Intentional fallthrough to next branch } // eslint-disable-next-line-no-fallthrough @@ -1710,54 +1712,56 @@ function commitDeletionEffectsOnFiber( case ForwardRef: case MemoComponent: case SimpleMemoComponent: { - const updateQueue: FunctionComponentUpdateQueue | null = (deletedFiber.updateQueue: any); - if (updateQueue !== null) { - const lastEffect = updateQueue.lastEffect; - if (lastEffect !== null) { - const firstEffect = lastEffect.next; - - let effect = firstEffect; - do { - const {destroy, tag} = effect; - if (destroy !== undefined) { - if ((tag & HookInsertion) !== NoHookEffect) { - safelyCallDestroy( - deletedFiber, - nearestMountedAncestor, - destroy, - ); - } else if ((tag & HookLayout) !== NoHookEffect) { - if (enableSchedulingProfiler) { - markComponentLayoutEffectUnmountStarted(deletedFiber); - } - - if ( - enableProfilerTimer && - enableProfilerCommitHooks && - deletedFiber.mode & ProfileMode - ) { - startLayoutEffectTimer(); - safelyCallDestroy( - deletedFiber, - nearestMountedAncestor, - destroy, - ); - recordLayoutEffectDuration(deletedFiber); - } else { + if (!offscreenSubtreeWasHidden) { + const updateQueue: FunctionComponentUpdateQueue | null = (deletedFiber.updateQueue: any); + if (updateQueue !== null) { + const lastEffect = updateQueue.lastEffect; + if (lastEffect !== null) { + const firstEffect = lastEffect.next; + + let effect = firstEffect; + do { + const {destroy, tag} = effect; + if (destroy !== undefined) { + if ((tag & HookInsertion) !== NoHookEffect) { safelyCallDestroy( deletedFiber, nearestMountedAncestor, destroy, ); - } + } else if ((tag & HookLayout) !== NoHookEffect) { + if (enableSchedulingProfiler) { + markComponentLayoutEffectUnmountStarted(deletedFiber); + } - if (enableSchedulingProfiler) { - markComponentLayoutEffectUnmountStopped(); + if ( + enableProfilerTimer && + enableProfilerCommitHooks && + deletedFiber.mode & ProfileMode + ) { + startLayoutEffectTimer(); + safelyCallDestroy( + deletedFiber, + nearestMountedAncestor, + destroy, + ); + recordLayoutEffectDuration(deletedFiber); + } else { + safelyCallDestroy( + deletedFiber, + nearestMountedAncestor, + destroy, + ); + } + + if (enableSchedulingProfiler) { + markComponentLayoutEffectUnmountStopped(); + } } } - } - effect = effect.next; - } while (effect !== firstEffect); + effect = effect.next; + } while (effect !== firstEffect); + } } } @@ -1769,14 +1773,16 @@ function commitDeletionEffectsOnFiber( return; } case ClassComponent: { - safelyDetachRef(deletedFiber, nearestMountedAncestor); - const instance = deletedFiber.stateNode; - if (typeof instance.componentWillUnmount === 'function') { - safelyCallComponentWillUnmount( - deletedFiber, - nearestMountedAncestor, - instance, - ); + if (!offscreenSubtreeWasHidden) { + safelyDetachRef(deletedFiber, nearestMountedAncestor); + const instance = deletedFiber.stateNode; + if (typeof instance.componentWillUnmount === 'function') { + safelyCallComponentWillUnmount( + deletedFiber, + nearestMountedAncestor, + instance, + ); + } } recursivelyTraverseDeletionEffects( finishedRoot, @@ -1796,6 +1802,27 @@ function commitDeletionEffectsOnFiber( ); return; } + case OffscreenComponent: { + // If this offscreen component is hidden, we already unmounted it. Before + // deleting the children, track that it's already unmounted so that we + // don't attempt to unmount the effects again. + // TODO: If the tree is hidden, in most cases we should be able to skip + // over the nested children entirely. An exception is we haven't yet found + // the topmost host node to delete, which we already track on the stack. + // But the other case is portals, which need to be detached no matter how + // deeply they are nested. We should use a subtree flag to track whether a + // subtree includes a nested portal. + const prevOffscreenSubtreeWasHidden = offscreenSubtreeWasHidden; + offscreenSubtreeWasHidden = + prevOffscreenSubtreeWasHidden || deletedFiber.memoizedState !== null; + recursivelyTraverseDeletionEffects( + finishedRoot, + nearestMountedAncestor, + deletedFiber, + ); + offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden; + break; + } default: { recursivelyTraverseDeletionEffects( finishedRoot, @@ -2203,13 +2230,21 @@ function commitMutationEffectsOnFiber( return; } case OffscreenComponent: { + const wasHidden = current !== null && current.memoizedState !== null; + + // Before committing the children, track on the stack whether this + // offscreen subtree was already hidden, so that we don't unmount the + // effects again. + const prevOffscreenSubtreeWasHidden = offscreenSubtreeWasHidden; + offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden || wasHidden; recursivelyTraverseMutationEffects(root, finishedWork, lanes); + offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden; + commitReconciliationEffects(finishedWork); if (flags & Visibility) { const newState: OffscreenState | null = finishedWork.memoizedState; const isHidden = newState !== null; - const wasHidden = current !== null && current.memoizedState !== null; const offscreenBoundary: Fiber = finishedWork; if (supportsMutation) { diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseEffectsSemantics-test.js b/packages/react-reconciler/src/__tests__/ReactSuspenseEffectsSemantics-test.js index bdda4f8313dfa..1825d41fd1da6 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspenseEffectsSemantics-test.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspenseEffectsSemantics-test.js @@ -1980,7 +1980,6 @@ describe('ReactSuspenseEffectsSemantics', () => { // Destroy layout and passive effects in the errored tree. 'App destroy layout', - 'ThrowsInWillUnmount componentWillUnmount', 'Text:Fallback destroy layout', 'Text:Outside destroy layout', 'Text:Inside destroy passive', diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseEffectsSemanticsDOM-test.js b/packages/react-reconciler/src/__tests__/ReactSuspenseEffectsSemanticsDOM-test.js index cb1196baffe6a..00c126bb36450 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspenseEffectsSemanticsDOM-test.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspenseEffectsSemanticsDOM-test.js @@ -10,18 +10,39 @@ 'use strict'; let React; +let ReactDOM; let ReactDOMClient; +let Scheduler; let act; +let container; describe('ReactSuspenseEffectsSemanticsDOM', () => { beforeEach(() => { jest.resetModules(); React = require('react'); + ReactDOM = require('react-dom'); ReactDOMClient = require('react-dom/client'); + Scheduler = require('scheduler'); act = require('jest-react').act; + + container = document.createElement('div'); + document.body.appendChild(container); + }); + + afterEach(() => { + document.body.removeChild(container); }); + async function fakeImport(result) { + return {default: result}; + } + + function Text(props) { + Scheduler.unstable_yieldValue(props.text); + return props.text; + } + it('should not cause a cycle when combined with a render phase update', () => { let scheduleSuspendingUpdate; @@ -63,7 +84,7 @@ describe('ReactSuspenseEffectsSemanticsDOM', () => { } act(() => { - const root = ReactDOMClient.createRoot(document.createElement('div')); + const root = ReactDOMClient.createRoot(container); root.render(); }); @@ -71,4 +92,367 @@ describe('ReactSuspenseEffectsSemanticsDOM', () => { scheduleSuspendingUpdate(); }); }); + + it('does not destroy layout effects twice when hidden child is removed', async () => { + function ChildA({label}) { + React.useLayoutEffect(() => { + Scheduler.unstable_yieldValue('Did mount: ' + label); + return () => { + Scheduler.unstable_yieldValue('Will unmount: ' + label); + }; + }, []); + return ; + } + + function ChildB({label}) { + React.useLayoutEffect(() => { + Scheduler.unstable_yieldValue('Did mount: ' + label); + return () => { + Scheduler.unstable_yieldValue('Will unmount: ' + label); + }; + }, []); + return ; + } + + const LazyChildA = React.lazy(() => fakeImport(ChildA)); + const LazyChildB = React.lazy(() => fakeImport(ChildB)); + + function Parent({swap}) { + return ( + }> + {swap ? : } + + ); + } + + const root = ReactDOMClient.createRoot(container); + act(() => { + root.render(); + }); + expect(Scheduler).toHaveYielded(['Loading...']); + + await LazyChildA; + expect(Scheduler).toFlushAndYield(['A', 'Did mount: A']); + expect(container.innerHTML).toBe('A'); + + // Swap the position of A and B + ReactDOM.flushSync(() => { + root.render(); + }); + expect(Scheduler).toHaveYielded(['Loading...', 'Will unmount: A']); + expect(container.innerHTML).toBe('Loading...'); + + await LazyChildB; + expect(Scheduler).toFlushAndYield(['B', 'Did mount: B']); + expect(container.innerHTML).toBe('B'); + }); + + it('does not destroy ref cleanup twice when hidden child is removed', async () => { + function ChildA({label}) { + return ( + { + if (node) { + Scheduler.unstable_yieldValue('Ref mount: ' + label); + } else { + Scheduler.unstable_yieldValue('Ref unmount: ' + label); + } + }}> + + + ); + } + + function ChildB({label}) { + return ( + { + if (node) { + Scheduler.unstable_yieldValue('Ref mount: ' + label); + } else { + Scheduler.unstable_yieldValue('Ref unmount: ' + label); + } + }}> + + + ); + } + + const LazyChildA = React.lazy(() => fakeImport(ChildA)); + const LazyChildB = React.lazy(() => fakeImport(ChildB)); + + function Parent({swap}) { + return ( + }> + {swap ? : } + + ); + } + + const root = ReactDOMClient.createRoot(container); + act(() => { + root.render(); + }); + expect(Scheduler).toHaveYielded(['Loading...']); + + await LazyChildA; + expect(Scheduler).toFlushAndYield(['A', 'Ref mount: A']); + expect(container.innerHTML).toBe('A'); + + // Swap the position of A and B + ReactDOM.flushSync(() => { + root.render(); + }); + expect(Scheduler).toHaveYielded(['Loading...', 'Ref unmount: A']); + expect(container.innerHTML).toBe( + 'ALoading...', + ); + + await LazyChildB; + expect(Scheduler).toFlushAndYield(['B', 'Ref mount: B']); + expect(container.innerHTML).toBe('B'); + }); + + it('does not call componentWillUnmount twice when hidden child is removed', async () => { + class ChildA extends React.Component { + componentDidMount() { + Scheduler.unstable_yieldValue('Did mount: ' + this.props.label); + } + componentWillUnmount() { + Scheduler.unstable_yieldValue('Will unmount: ' + this.props.label); + } + render() { + return ; + } + } + + class ChildB extends React.Component { + componentDidMount() { + Scheduler.unstable_yieldValue('Did mount: ' + this.props.label); + } + componentWillUnmount() { + Scheduler.unstable_yieldValue('Will unmount: ' + this.props.label); + } + render() { + return ; + } + } + + const LazyChildA = React.lazy(() => fakeImport(ChildA)); + const LazyChildB = React.lazy(() => fakeImport(ChildB)); + + function Parent({swap}) { + return ( + }> + {swap ? : } + + ); + } + + const root = ReactDOMClient.createRoot(container); + act(() => { + root.render(); + }); + expect(Scheduler).toHaveYielded(['Loading...']); + + await LazyChildA; + expect(Scheduler).toFlushAndYield(['A', 'Did mount: A']); + expect(container.innerHTML).toBe('A'); + + // Swap the position of A and B + ReactDOM.flushSync(() => { + root.render(); + }); + expect(Scheduler).toHaveYielded(['Loading...', 'Will unmount: A']); + expect(container.innerHTML).toBe('Loading...'); + + await LazyChildB; + expect(Scheduler).toFlushAndYield(['B', 'Did mount: B']); + expect(container.innerHTML).toBe('B'); + }); + + it('does not destroy layout effects twice when parent suspense is removed', async () => { + function ChildA({label}) { + React.useLayoutEffect(() => { + Scheduler.unstable_yieldValue('Did mount: ' + label); + return () => { + Scheduler.unstable_yieldValue('Will unmount: ' + label); + }; + }, []); + return ; + } + function ChildB({label}) { + React.useLayoutEffect(() => { + Scheduler.unstable_yieldValue('Did mount: ' + label); + return () => { + Scheduler.unstable_yieldValue('Will unmount: ' + label); + }; + }, []); + return ; + } + const LazyChildA = React.lazy(() => fakeImport(ChildA)); + const LazyChildB = React.lazy(() => fakeImport(ChildB)); + + function Parent({swap}) { + return ( + }> + {swap ? : } + + ); + } + + const root = ReactDOMClient.createRoot(container); + act(() => { + root.render(); + }); + expect(Scheduler).toHaveYielded(['Loading...']); + + await LazyChildA; + expect(Scheduler).toFlushAndYield(['A', 'Did mount: A']); + expect(container.innerHTML).toBe('A'); + + // Swap the position of A and B + ReactDOM.flushSync(() => { + root.render(); + }); + expect(Scheduler).toHaveYielded(['Loading...', 'Will unmount: A']); + expect(container.innerHTML).toBe('Loading...'); + + // Destroy the whole tree, including the hidden A + ReactDOM.flushSync(() => { + root.render(

Hello

); + }); + expect(Scheduler).toFlushAndYield([]); + expect(container.innerHTML).toBe('

Hello

'); + }); + + it('does not destroy ref cleanup twice when parent suspense is removed', async () => { + function ChildA({label}) { + return ( + { + if (node) { + Scheduler.unstable_yieldValue('Ref mount: ' + label); + } else { + Scheduler.unstable_yieldValue('Ref unmount: ' + label); + } + }}> + + + ); + } + + function ChildB({label}) { + return ( + { + if (node) { + Scheduler.unstable_yieldValue('Ref mount: ' + label); + } else { + Scheduler.unstable_yieldValue('Ref unmount: ' + label); + } + }}> + + + ); + } + + const LazyChildA = React.lazy(() => fakeImport(ChildA)); + const LazyChildB = React.lazy(() => fakeImport(ChildB)); + + function Parent({swap}) { + return ( + }> + {swap ? : } + + ); + } + + const root = ReactDOMClient.createRoot(container); + act(() => { + root.render(); + }); + expect(Scheduler).toHaveYielded(['Loading...']); + + await LazyChildA; + expect(Scheduler).toFlushAndYield(['A', 'Ref mount: A']); + expect(container.innerHTML).toBe('A'); + + // Swap the position of A and B + ReactDOM.flushSync(() => { + root.render(); + }); + expect(Scheduler).toHaveYielded(['Loading...', 'Ref unmount: A']); + expect(container.innerHTML).toBe( + 'ALoading...', + ); + + // Destroy the whole tree, including the hidden A + ReactDOM.flushSync(() => { + root.render(

Hello

); + }); + expect(Scheduler).toFlushAndYield([]); + expect(container.innerHTML).toBe('

Hello

'); + }); + + it('does not call componentWillUnmount twice when parent suspense is removed', async () => { + class ChildA extends React.Component { + componentDidMount() { + Scheduler.unstable_yieldValue('Did mount: ' + this.props.label); + } + componentWillUnmount() { + Scheduler.unstable_yieldValue('Will unmount: ' + this.props.label); + } + render() { + return ; + } + } + + class ChildB extends React.Component { + componentDidMount() { + Scheduler.unstable_yieldValue('Did mount: ' + this.props.label); + } + componentWillUnmount() { + Scheduler.unstable_yieldValue('Will unmount: ' + this.props.label); + } + render() { + return ; + } + } + + const LazyChildA = React.lazy(() => fakeImport(ChildA)); + const LazyChildB = React.lazy(() => fakeImport(ChildB)); + + function Parent({swap}) { + return ( + }> + {swap ? : } + + ); + } + + const root = ReactDOMClient.createRoot(container); + act(() => { + root.render(); + }); + expect(Scheduler).toHaveYielded(['Loading...']); + + await LazyChildA; + expect(Scheduler).toFlushAndYield(['A', 'Did mount: A']); + expect(container.innerHTML).toBe('A'); + + // Swap the position of A and B + ReactDOM.flushSync(() => { + root.render(); + }); + expect(Scheduler).toHaveYielded(['Loading...', 'Will unmount: A']); + expect(container.innerHTML).toBe('Loading...'); + + // Destroy the whole tree, including the hidden A + ReactDOM.flushSync(() => { + root.render(

Hello

); + }); + expect(Scheduler).toFlushAndYield([]); + expect(container.innerHTML).toBe('

Hello

'); + }); });