From 7cf1381537f07f495decad53734cbfe5da8ed407 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 20 Jan 2021 12:24:01 -0600 Subject: [PATCH] Delete remaining references to effect list (#20625) I think that's it! --- .eslintrc.js | 2 +- .../src/ReactChildFiber.new.js | 14 ---- .../react-reconciler/src/ReactFiber.new.js | 20 ++--- .../src/ReactFiberBeginWork.new.js | 36 +-------- .../src/ReactFiberCommitWork.new.js | 3 - .../src/ReactFiberCompleteWork.new.js | 57 ++++++++----- .../src/ReactFiberHydrationContext.new.js | 12 --- .../src/ReactFiberSuspenseComponent.new.js | 3 - .../src/ReactFiberThrow.new.js | 2 - .../src/ReactFiberWorkLoop.new.js | 81 +++++-------------- 10 files changed, 63 insertions(+), 167 deletions(-) diff --git a/.eslintrc.js b/.eslintrc.js index a456964bdbdb0..bed7ff123afd1 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -116,7 +116,7 @@ module.exports = { 'react-internal/no-cross-fork-types': [ ERROR, { - old: [], + old: ['firstEffect', 'nextEffect'], new: [], }, ], diff --git a/packages/react-reconciler/src/ReactChildFiber.new.js b/packages/react-reconciler/src/ReactChildFiber.new.js index a18f845d26ae3..3435694b7ab0a 100644 --- a/packages/react-reconciler/src/ReactChildFiber.new.js +++ b/packages/react-reconciler/src/ReactChildFiber.new.js @@ -263,20 +263,6 @@ function ChildReconciler(shouldTrackSideEffects) { // Noop. return; } - // Deletions are added in reversed order so we add it to the front. - // At this point, the return fiber's effect list is empty except for - // deletions, so we can just append the deletion to the list. The remaining - // effects aren't added until the complete phase. Once we implement - // resuming, this may not be true. - const last = returnFiber.lastEffect; - if (last !== null) { - last.nextEffect = childToDelete; - returnFiber.lastEffect = childToDelete; - } else { - returnFiber.firstEffect = returnFiber.lastEffect = childToDelete; - } - childToDelete.nextEffect = null; - const deletions = returnFiber.deletions; if (deletions === null) { returnFiber.deletions = [childToDelete]; diff --git a/packages/react-reconciler/src/ReactFiber.new.js b/packages/react-reconciler/src/ReactFiber.new.js index 3b3d9d4bc2e7b..d6c28749f26da 100644 --- a/packages/react-reconciler/src/ReactFiber.new.js +++ b/packages/react-reconciler/src/ReactFiber.new.js @@ -144,10 +144,6 @@ function FiberNode( // Effects this.flags = NoFlags; - this.nextEffect = null; - - this.firstEffect = null; - this.lastEffect = null; this.subtreeFlags = NoFlags; this.deletions = null; @@ -285,10 +281,7 @@ export function createWorkInProgress(current: Fiber, pendingProps: any): Fiber { // Reset the effect tag. workInProgress.flags = NoFlags; - // The effect list is no longer valid. - workInProgress.nextEffect = null; - workInProgress.firstEffect = null; - workInProgress.lastEffect = null; + // The effects are no longer valid. workInProgress.subtreeFlags = NoFlags; workInProgress.deletions = null; @@ -370,10 +363,7 @@ export function resetWorkInProgress(workInProgress: Fiber, renderLanes: Lanes) { // that child fiber is setting, not the reconciliation. workInProgress.flags &= StaticMask | Placement; - // The effect list is no longer valid. - workInProgress.nextEffect = null; - workInProgress.firstEffect = null; - workInProgress.lastEffect = null; + // The effects are no longer valid. const current = workInProgress.alternate; if (current === null) { @@ -403,6 +393,9 @@ export function resetWorkInProgress(workInProgress: Fiber, renderLanes: Lanes) { workInProgress.lanes = current.lanes; workInProgress.child = current.child; + // TODO: `subtreeFlags` should be reset to NoFlags, like we do in + // `createWorkInProgress`. Nothing reads this until the complete phase, + // currently, but it might in the future, and we should be consistent. workInProgress.subtreeFlags = current.subtreeFlags; workInProgress.deletions = null; workInProgress.memoizedProps = current.memoizedProps; @@ -847,9 +840,6 @@ export function assignFiberPropertiesInDEV( target.dependencies = source.dependencies; target.mode = source.mode; target.flags = source.flags; - target.nextEffect = source.nextEffect; - target.firstEffect = source.firstEffect; - target.lastEffect = source.lastEffect; target.subtreeFlags = source.subtreeFlags; target.deletions = source.deletions; target.lanes = source.lanes; diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.new.js b/packages/react-reconciler/src/ReactFiberBeginWork.new.js index f447ee0e3545b..2239906d43e4b 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.new.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.new.js @@ -2198,8 +2198,6 @@ function updateSuspensePrimaryChildren( primaryChildFragment.sibling = null; if (currentFallbackChildFragment !== null) { // Delete the fallback child fragment - currentFallbackChildFragment.nextEffect = null; - workInProgress.firstEffect = workInProgress.lastEffect = currentFallbackChildFragment; const deletions = workInProgress.deletions; if (deletions === null) { workInProgress.deletions = [currentFallbackChildFragment]; @@ -2261,22 +2259,9 @@ function updateSuspenseFallbackChildren( currentPrimaryChildFragment.treeBaseDuration; } - if (currentFallbackChildFragment !== null) { - // The fallback fiber was added as a deletion effect during the first - // pass. However, since we're going to remain on the fallback, we no - // longer want to delete it. So we need to remove it from the list. - // Deletions are stored on the same list as effects, and are always added - // to the front. So we know that the first effect must be the fallback - // deletion effect, and everything after that is from the primary free. - const firstPrimaryTreeEffect = currentFallbackChildFragment.nextEffect; - if (firstPrimaryTreeEffect !== null) { - workInProgress.firstEffect = firstPrimaryTreeEffect; - } else { - // TODO: Reset this somewhere else? Lol legacy mode is so weird. - workInProgress.firstEffect = workInProgress.lastEffect = null; - } - } - + // The fallback fiber was added as a deletion during the first pass. + // However, since we're going to remain on the fallback, we no longer want + // to delete it. workInProgress.deletions = null; } else { primaryChildFragment = createWorkInProgressOffscreenFiber( @@ -2773,7 +2758,6 @@ function initSuspenseListRenderState( tail: null | Fiber, lastContentRow: null | Fiber, tailMode: SuspenseListTailMode, - lastEffectBeforeRendering: null | Fiber, ): void { const renderState: null | SuspenseListRenderState = workInProgress.memoizedState; @@ -2785,7 +2769,6 @@ function initSuspenseListRenderState( last: lastContentRow, tail: tail, tailMode: tailMode, - lastEffect: lastEffectBeforeRendering, }: SuspenseListRenderState); } else { // We can reuse the existing object from previous renders. @@ -2795,7 +2778,6 @@ function initSuspenseListRenderState( renderState.last = lastContentRow; renderState.tail = tail; renderState.tailMode = tailMode; - renderState.lastEffect = lastEffectBeforeRendering; } } @@ -2877,7 +2859,6 @@ function updateSuspenseListComponent( tail, lastContentRow, tailMode, - workInProgress.lastEffect, ); break; } @@ -2909,7 +2890,6 @@ function updateSuspenseListComponent( tail, null, // last tailMode, - workInProgress.lastEffect, ); break; } @@ -2920,7 +2900,6 @@ function updateSuspenseListComponent( null, // tail null, // last undefined, - workInProgress.lastEffect, ); break; } @@ -3180,15 +3159,6 @@ function remountFiber( // Delete the old fiber and place the new one. // Since the old fiber is disconnected, we have to schedule it manually. - const last = returnFiber.lastEffect; - if (last !== null) { - last.nextEffect = current; - returnFiber.lastEffect = current; - } else { - returnFiber.firstEffect = returnFiber.lastEffect = current; - } - current.nextEffect = null; - const deletions = returnFiber.deletions; if (deletions === null) { returnFiber.deletions = [current]; diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index 714c77e477ae9..371991abda353 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js @@ -1213,9 +1213,6 @@ export function detachFiberAfterEffects(fiber: Fiber): void { fiber.sibling = null; fiber.stateNode = null; fiber.updateQueue = null; - fiber.nextEffect = null; - fiber.firstEffect = null; - fiber.lastEffect = null; if (__DEV__) { fiber._debugOwner = null; diff --git a/packages/react-reconciler/src/ReactFiberCompleteWork.new.js b/packages/react-reconciler/src/ReactFiberCompleteWork.new.js index 92fa3eeacb603..cd7ed4c4c180e 100644 --- a/packages/react-reconciler/src/ReactFiberCompleteWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCompleteWork.new.js @@ -72,7 +72,9 @@ import { NoFlags, DidCapture, Snapshot, + ChildDeletion, StaticMask, + MutationMask, } from './ReactFiberFlags'; import invariant from 'shared/invariant'; @@ -173,6 +175,31 @@ function markRef(workInProgress: Fiber) { workInProgress.flags |= Ref; } +function hadNoMutationsEffects(current: null | Fiber, completedWork: Fiber) { + const didBailout = current !== null && current.child === completedWork.child; + if (didBailout) { + return true; + } + + if ((completedWork.flags & ChildDeletion) !== NoFlags) { + return false; + } + + // TODO: If we move the `hadNoMutationsEffects` call after `bubbleProperties` + // then we only have to check the `completedWork.subtreeFlags`. + let child = completedWork.child; + while (child !== null) { + if ( + (child.flags & MutationMask) !== NoFlags || + (child.subtreeFlags & MutationMask) !== NoFlags + ) { + return false; + } + child = child.sibling; + } + return true; +} + let appendAllChildren; let updateHostContainer; let updateHostComponent; @@ -217,7 +244,7 @@ if (supportsMutation) { } }; - updateHostContainer = function(workInProgress: Fiber) { + updateHostContainer = function(current: null | Fiber, workInProgress: Fiber) { // Noop }; updateHostComponent = function( @@ -461,13 +488,13 @@ if (supportsMutation) { node = node.sibling; } }; - updateHostContainer = function(workInProgress: Fiber) { + updateHostContainer = function(current: null | Fiber, workInProgress: Fiber) { const portalOrRoot: { containerInfo: Container, pendingChildren: ChildSet, ... } = workInProgress.stateNode; - const childrenUnchanged = workInProgress.firstEffect === null; + const childrenUnchanged = hadNoMutationsEffects(current, workInProgress); if (childrenUnchanged) { // No changes, just reuse the existing instance. } else { @@ -492,7 +519,7 @@ if (supportsMutation) { const oldProps = current.memoizedProps; // If there are no effects associated with this node, then none of our children had any updates. // This guarantees that we can reuse all of them. - const childrenUnchanged = workInProgress.firstEffect === null; + const childrenUnchanged = hadNoMutationsEffects(current, workInProgress); if (childrenUnchanged && oldProps === newProps) { // No changes, just reuse the existing instance. // Note that this might release a previous clone. @@ -575,7 +602,7 @@ if (supportsMutation) { }; } else { // No host operations - updateHostContainer = function(workInProgress: Fiber) { + updateHostContainer = function(current: null | Fiber, workInProgress: Fiber) { // Noop }; updateHostComponent = function( @@ -847,7 +874,7 @@ function completeWork( workInProgress.flags |= Snapshot; } } - updateHostContainer(workInProgress); + updateHostContainer(current, workInProgress); bubbleProperties(workInProgress); return null; } @@ -1142,7 +1169,7 @@ function completeWork( } case HostPortal: popHostContainer(workInProgress); - updateHostContainer(workInProgress); + updateHostContainer(current, workInProgress); if (current === null) { preparePortalMount(workInProgress.stateNode.containerInfo); } @@ -1226,11 +1253,7 @@ function completeWork( // Rerender the whole list, but this time, we'll force fallbacks // to stay in place. - // Reset the effect list before doing the second pass since that's now invalid. - if (renderState.lastEffect === null) { - workInProgress.firstEffect = null; - } - workInProgress.lastEffect = renderState.lastEffect; + // Reset the effect flags before doing the second pass since that's now invalid. // Reset the child fibers to their original state. workInProgress.subtreeFlags = NoFlags; resetChildFibers(workInProgress, renderLanes); @@ -1301,15 +1324,6 @@ function completeWork( !renderedTail.alternate && !getIsHydrating() // We don't cut it if we're hydrating. ) { - // We need to delete the row we just rendered. - // Reset the effect list to what it was before we rendered this - // child. The nested children have already appended themselves. - const lastEffect = (workInProgress.lastEffect = - renderState.lastEffect); - // Remove any effects that were appended after this point. - if (lastEffect !== null) { - lastEffect.nextEffect = null; - } // We're done. bubbleProperties(workInProgress); return null; @@ -1369,7 +1383,6 @@ function completeWork( const next = renderState.tail; renderState.rendering = next; renderState.tail = next.sibling; - renderState.lastEffect = workInProgress.lastEffect; renderState.renderingStartTime = now(); next.sibling = null; diff --git a/packages/react-reconciler/src/ReactFiberHydrationContext.new.js b/packages/react-reconciler/src/ReactFiberHydrationContext.new.js index 087dd818b9a9d..d91901f519764 100644 --- a/packages/react-reconciler/src/ReactFiberHydrationContext.new.js +++ b/packages/react-reconciler/src/ReactFiberHydrationContext.new.js @@ -125,18 +125,6 @@ function deleteHydratableInstance( childToDelete.stateNode = instance; childToDelete.return = returnFiber; - // This might seem like it belongs on progressedFirstDeletion. However, - // these children are not part of the reconciliation list of children. - // Even if we abort and rereconcile the children, that will try to hydrate - // again and the nodes are still in the host tree so these will be - // recreated. - if (returnFiber.lastEffect !== null) { - returnFiber.lastEffect.nextEffect = childToDelete; - returnFiber.lastEffect = childToDelete; - } else { - returnFiber.firstEffect = returnFiber.lastEffect = childToDelete; - } - const deletions = returnFiber.deletions; if (deletions === null) { returnFiber.deletions = [childToDelete]; diff --git a/packages/react-reconciler/src/ReactFiberSuspenseComponent.new.js b/packages/react-reconciler/src/ReactFiberSuspenseComponent.new.js index 9692c89a65911..b1077092d46a0 100644 --- a/packages/react-reconciler/src/ReactFiberSuspenseComponent.new.js +++ b/packages/react-reconciler/src/ReactFiberSuspenseComponent.new.js @@ -60,9 +60,6 @@ export type SuspenseListRenderState = {| tail: null | Fiber, // Tail insertions setting. tailMode: SuspenseListTailMode, - // Last Effect before we rendered the "rendering" item. - // Used to remove new effects added by the rendered item. - lastEffect: null | Fiber, |}; export function shouldCaptureSuspense( diff --git a/packages/react-reconciler/src/ReactFiberThrow.new.js b/packages/react-reconciler/src/ReactFiberThrow.new.js index ea670b69f070a..27baacda7daed 100644 --- a/packages/react-reconciler/src/ReactFiberThrow.new.js +++ b/packages/react-reconciler/src/ReactFiberThrow.new.js @@ -188,8 +188,6 @@ function throwException( ) { // The source fiber did not complete. sourceFiber.flags |= Incomplete; - // Its effect list is no longer valid. - sourceFiber.firstEffect = sourceFiber.lastEffect = null; if ( value !== null && diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index ed3c4ce8882c3..cb4c4dd366673 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -117,14 +117,15 @@ import { import {LegacyRoot} from './ReactRootTags'; import { NoFlags, - PerformedWork, Placement, PassiveStatic, Incomplete, HostEffectMask, Hydrating, + BeforeMutationMask, + MutationMask, + LayoutMask, PassiveMask, - StaticMask, } from './ReactFiberFlags'; import { NoLanePriority, @@ -1716,45 +1717,6 @@ function completeUnitOfWork(unitOfWork: Fiber): void { workInProgress = next; return; } - - if ( - returnFiber !== null && - // Do not append effects to parents if a sibling failed to complete - (returnFiber.flags & Incomplete) === NoFlags - ) { - // Append all the effects of the subtree and this fiber onto the effect - // list of the parent. The completion order of the children affects the - // side-effect order. - if (returnFiber.firstEffect === null) { - returnFiber.firstEffect = completedWork.firstEffect; - } - if (completedWork.lastEffect !== null) { - if (returnFiber.lastEffect !== null) { - returnFiber.lastEffect.nextEffect = completedWork.firstEffect; - } - returnFiber.lastEffect = completedWork.lastEffect; - } - - // If this fiber had side-effects, we append it AFTER the children's - // side-effects. We can perform certain side-effects earlier if needed, - // by doing multiple passes over the effect list. We don't want to - // schedule our own side-effect on our own list because if end up - // reusing children we'll schedule this effect onto itself since we're - // at the end. - const flags = completedWork.flags; - - // Skip both NoWork and PerformedWork tags when creating the effect - // list. PerformedWork effect is read by React DevTools but shouldn't be - // committed. - if ((flags & ~StaticMask) > PerformedWork) { - if (returnFiber.lastEffect !== null) { - returnFiber.lastEffect.nextEffect = completedWork; - } else { - returnFiber.firstEffect = completedWork; - } - returnFiber.lastEffect = completedWork; - } - } } else { // This fiber did not complete because something threw. Pop values off // the stack without entering the complete phase. If this is a boundary, @@ -1791,8 +1753,7 @@ function completeUnitOfWork(unitOfWork: Fiber): void { } if (returnFiber !== null) { - // Mark the parent fiber as incomplete and clear its effect list. - returnFiber.firstEffect = returnFiber.lastEffect = null; + // Mark the parent fiber as incomplete and clear its subtree flags. returnFiber.flags |= Incomplete; returnFiber.subtreeFlags = NoFlags; returnFiber.deletions = null; @@ -1910,24 +1871,6 @@ function commitRootImpl(root, renderPriorityLevel) { // times out. } - // Get the list of effects. - let firstEffect; - if (finishedWork.flags > PerformedWork) { - // A fiber's effect list consists only of its children, not itself. So if - // the root has an effect, we need to add it to the end of the list. The - // resulting list is the set that would belong to the root's parent, if it - // had one; that is, all the effects in the tree including the root. - if (finishedWork.lastEffect !== null) { - finishedWork.lastEffect.nextEffect = finishedWork; - firstEffect = finishedWork.firstEffect; - } else { - firstEffect = finishedWork; - } - } else { - // There is no effect on the root. - firstEffect = finishedWork.firstEffect; - } - // If there are pending passive effects, schedule a callback to process them. // Do this as early as possible, so it is queued before anything else that // might get scheduled in the commit phase. (See #16714.) @@ -1946,7 +1889,21 @@ function commitRootImpl(root, renderPriorityLevel) { } } - if (firstEffect !== null) { + // Check if there are any effects in the whole tree. + // TODO: This is left over from the effect list implementation, where we had + // to check for the existence of `firstEffect` to satsify Flow. I think the + // only other reason this optimization exists is because it affects profiling. + // Reconsider whether this is necessary. + const subtreeHasEffects = + (finishedWork.subtreeFlags & + (BeforeMutationMask | MutationMask | LayoutMask | PassiveMask)) !== + NoFlags; + const rootHasEffect = + (finishedWork.flags & + (BeforeMutationMask | MutationMask | LayoutMask | PassiveMask)) !== + NoFlags; + + if (subtreeHasEffects || rootHasEffect) { let previousLanePriority; if (decoupleUpdatePriorityFromScheduler) { previousLanePriority = getCurrentUpdateLanePriority();