From 2d01fb40661fdab8d70cbd6c7fb33c97979a25cc Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Mon, 6 Jul 2020 13:21:03 -0400 Subject: [PATCH] Effects list rewrite --- ...DOMServerPartialHydration-test.internal.js | 9 +- ...ReactNativeComponentClass-test.internal.js | 8 +- .../src/ReactChildFiber.new.js | 2 + .../react-reconciler/src/ReactFiber.new.js | 6 + .../src/ReactFiberBeginWork.new.js | 4 + .../src/ReactFiberCompleteWork.new.js | 9 + .../src/ReactFiberHydrationContext.new.js | 3 +- .../src/ReactFiberWorkLoop.new.js | 506 +++++++++++------- .../src/ReactInternalTypes.js | 2 + .../ReactSuspenseWithNoopRenderer-test.js | 33 +- 10 files changed, 381 insertions(+), 201 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js b/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js index 43c8c2de6caaf..7b4dc925001e2 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js @@ -367,7 +367,14 @@ describe('ReactDOMServerPartialHydration', () => { const span2 = container.getElementsByTagName('span')[0]; // This is a new node. expect(span).not.toBe(span2); - expect(ref.current).toBe(span2); + + if (gate(flags => flags.new)) { + // The effects list refactor causes this to be null because the Suspense Offscreen's child + // is null. However, since we can't hydrate Suspense in legacy this change in behavior is ok + expect(ref.current).toBe(null); + } else { + expect(ref.current).toBe(span2); + } // Resolving the promise should render the final content. suspend = false; diff --git a/packages/react-native-renderer/src/__tests__/createReactNativeComponentClass-test.internal.js b/packages/react-native-renderer/src/__tests__/createReactNativeComponentClass-test.internal.js index e0e5ad6b4e791..3aab90ee0735f 100644 --- a/packages/react-native-renderer/src/__tests__/createReactNativeComponentClass-test.internal.js +++ b/packages/react-native-renderer/src/__tests__/createReactNativeComponentClass-test.internal.js @@ -45,8 +45,12 @@ describe('createReactNativeComponentClass', () => { expect(Text).not.toBe(View); - ReactNative.render(, 1); - ReactNative.render(, 1); + ReactNative.render( + + + , + 1, + ); }); it('should not allow viewConfigs with duplicate uiViewClassNames to be registered', () => { diff --git a/packages/react-reconciler/src/ReactChildFiber.new.js b/packages/react-reconciler/src/ReactChildFiber.new.js index 0406a362db5b2..66a99eaa204d3 100644 --- a/packages/react-reconciler/src/ReactChildFiber.new.js +++ b/packages/react-reconciler/src/ReactChildFiber.new.js @@ -280,6 +280,7 @@ function ChildReconciler(shouldTrackSideEffects) { // 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. + // TODO (effects) Get rid of effects list update here. const last = returnFiber.lastEffect; if (last !== null) { last.nextEffect = childToDelete; @@ -287,6 +288,7 @@ function ChildReconciler(shouldTrackSideEffects) { } else { returnFiber.firstEffect = returnFiber.lastEffect = childToDelete; } + returnFiber.deletions.push(childToDelete); childToDelete.nextEffect = null; childToDelete.effectTag = Deletion; } diff --git a/packages/react-reconciler/src/ReactFiber.new.js b/packages/react-reconciler/src/ReactFiber.new.js index fd95292f1b472..0240483e40157 100644 --- a/packages/react-reconciler/src/ReactFiber.new.js +++ b/packages/react-reconciler/src/ReactFiber.new.js @@ -144,6 +144,8 @@ function FiberNode( // Effects this.effectTag = NoEffect; + this.subtreeTag = NoEffect; + this.deletions = []; this.nextEffect = null; this.firstEffect = null; @@ -287,6 +289,8 @@ export function createWorkInProgress(current: Fiber, pendingProps: any): Fiber { // We already have an alternate. // Reset the effect tag. workInProgress.effectTag = NoEffect; + workInProgress.subtreeTag = NoEffect; + workInProgress.deletions = []; // The effect list is no longer valid. workInProgress.nextEffect = null; @@ -826,6 +830,8 @@ export function assignFiberPropertiesInDEV( target.dependencies = source.dependencies; target.mode = source.mode; target.effectTag = source.effectTag; + target.subtreeTag = source.subtreeTag; + target.deletions = source.deletions; target.nextEffect = source.nextEffect; target.firstEffect = source.firstEffect; target.lastEffect = source.lastEffect; diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.new.js b/packages/react-reconciler/src/ReactFiberBeginWork.new.js index 42ab9c0d064e2..a2ab46d48eb7e 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.new.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.new.js @@ -2066,6 +2066,7 @@ function updateSuspensePrimaryChildren( currentFallbackChildFragment.nextEffect = null; currentFallbackChildFragment.effectTag = Deletion; workInProgress.firstEffect = workInProgress.lastEffect = currentFallbackChildFragment; + workInProgress.deletions.push(currentFallbackChildFragment); } workInProgress.child = primaryChildFragment; @@ -2131,9 +2132,11 @@ function updateSuspenseFallbackChildren( workInProgress.firstEffect = primaryChildFragment.firstEffect; workInProgress.lastEffect = progressedLastEffect; progressedLastEffect.nextEffect = null; + workInProgress.deletions = []; } else { // TODO: Reset this somewhere else? Lol legacy mode is so weird. workInProgress.firstEffect = workInProgress.lastEffect = null; + workInProgress.deletions = []; } } else { primaryChildFragment = createWorkInProgressOffscreenFiber( @@ -3040,6 +3043,7 @@ function remountFiber( } else { returnFiber.firstEffect = returnFiber.lastEffect = current; } + returnFiber.deletions.push(current); current.nextEffect = null; current.effectTag = Deletion; diff --git a/packages/react-reconciler/src/ReactFiberCompleteWork.new.js b/packages/react-reconciler/src/ReactFiberCompleteWork.new.js index b820a690811ca..f6a83fb226fe8 100644 --- a/packages/react-reconciler/src/ReactFiberCompleteWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCompleteWork.new.js @@ -1062,6 +1062,15 @@ function completeWork( // Reset the effect list before doing the second pass since that's now invalid. if (renderState.lastEffect === null) { workInProgress.firstEffect = null; + workInProgress.subtreeTag = NoEffect; + // TODO (effects) This probably isn't the best approach. Discuss with Brian + let child = workInProgress.child; + while (child !== null) { + if (child.deletions.length > 0) { + child.deletions = []; + } + child = child.sibling; + } } workInProgress.lastEffect = renderState.lastEffect; // Reset the child fibers to their original state. diff --git a/packages/react-reconciler/src/ReactFiberHydrationContext.new.js b/packages/react-reconciler/src/ReactFiberHydrationContext.new.js index 3f4c83da829ba..2f5a9cb3000f9 100644 --- a/packages/react-reconciler/src/ReactFiberHydrationContext.new.js +++ b/packages/react-reconciler/src/ReactFiberHydrationContext.new.js @@ -24,7 +24,7 @@ import { HostRoot, SuspenseComponent, } from './ReactWorkTags'; -import {Deletion, Placement, Hydrating} from './ReactSideEffectTags'; +import {Deletion, Hydrating, Placement} from './ReactSideEffectTags'; import invariant from 'shared/invariant'; import { @@ -125,6 +125,7 @@ function deleteHydratableInstance( childToDelete.stateNode = instance; childToDelete.return = returnFiber; childToDelete.effectTag = Deletion; + returnFiber.deletions.push(childToDelete); // This might seem like it belongs on progressedFirstDeletion. However, // these children are not part of the reconciliation list of children. diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index d3ee13fda5fd2..06e1218383e92 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -1679,6 +1679,8 @@ function completeUnitOfWork(unitOfWork: Fiber): void { // Mark the parent fiber as incomplete and clear its effect list. returnFiber.firstEffect = returnFiber.lastEffect = null; returnFiber.effectTag |= Incomplete; + returnFiber.subtreeTag = NoEffect; + returnFiber.deletions = []; } } @@ -1716,6 +1718,12 @@ function resetChildLanes(completedWork: Fiber) { } let newChildLanes = NoLanes; + let subtreeTag = NoEffect; + let childrenDidNotComplete = false; + + const wasFiberCloned = + completedWork.alternate === null || + completedWork.child !== completedWork.alternate.child; // Bubble up the earliest expiration time. if (enableProfilerTimer && (completedWork.mode & ProfileMode) !== NoMode) { @@ -1724,24 +1732,31 @@ function resetChildLanes(completedWork: Fiber) { let actualDuration = completedWork.actualDuration; let treeBaseDuration = ((completedWork.selfBaseDuration: any): number); - // When a fiber is cloned, its actualDuration is reset to 0. This value will - // only be updated if work is done on the fiber (i.e. it doesn't bailout). - // When work is done, it should bubble to the parent's actualDuration. If - // the fiber has not been cloned though, (meaning no work was done), then - // this value will reflect the amount of time spent working on a previous - // render. In that case it should not bubble. We determine whether it was - // cloned by comparing the child pointer. - const shouldBubbleActualDurations = - completedWork.alternate === null || - completedWork.child !== completedWork.alternate.child; - let child = completedWork.child; while (child !== null) { newChildLanes = mergeLanes( newChildLanes, mergeLanes(child.lanes, child.childLanes), ); - if (shouldBubbleActualDurations) { + + subtreeTag |= child.subtreeTag; + subtreeTag |= child.effectTag & HostEffectMask; + if (child.deletions.length > 0) { + subtreeTag |= Deletion; + } + + if ((child.effectTag & Incomplete) !== NoEffect) { + childrenDidNotComplete = true; + } + + // When a fiber is cloned, its actualDuration is reset to 0. This value will + // only be updated if work is done on the fiber (i.e. it doesn't bailout). + // When work is done, it should bubble to the parent's actualDuration. If + // the fiber has not been cloned though, (meaning no work was done), then + // this value will reflect the amount of time spent working on a previous + // render. In that case it should not bubble. We determine whether it was + // cloned by comparing the child pointer. + if (wasFiberCloned) { actualDuration += child.actualDuration; } treeBaseDuration += child.treeBaseDuration; @@ -1768,11 +1783,27 @@ function resetChildLanes(completedWork: Fiber) { newChildLanes, mergeLanes(child.lanes, child.childLanes), ); + + subtreeTag |= child.subtreeTag; + subtreeTag |= child.effectTag & HostEffectMask; + if (child.deletions.length > 0) { + subtreeTag |= Deletion; + } + + if ((child.effectTag & Incomplete) !== NoEffect) { + childrenDidNotComplete = true; + } + child = child.sibling; } } completedWork.childLanes = newChildLanes; + + // TODO (effects) is it appropriate to check wasFiberCloned for this? + if (!childrenDidNotComplete && wasFiberCloned) { + completedWork.subtreeTag |= subtreeTag; + } } function commitRoot(root) { @@ -1884,26 +1915,7 @@ function commitRootImpl(root, renderPriorityLevel) { focusedInstanceHandle = prepareForCommit(root.containerInfo); shouldFireAfterActiveInstanceBlur = false; - nextEffect = firstEffect; - do { - if (__DEV__) { - invokeGuardedCallback(null, commitBeforeMutationEffects, null); - if (hasCaughtError()) { - invariant(nextEffect !== null, 'Should be working on an effect.'); - const error = clearCaughtError(); - captureCommitPhaseError(nextEffect, error); - nextEffect = nextEffect.nextEffect; - } - } else { - try { - commitBeforeMutationEffects(); - } catch (error) { - invariant(nextEffect !== null, 'Should be working on an effect.'); - captureCommitPhaseError(nextEffect, error); - nextEffect = nextEffect.nextEffect; - } - } - } while (nextEffect !== null); + commitBeforeMutationEffects(finishedWork); // We no longer need to track the active instance fiber focusedInstanceHandle = null; @@ -1915,32 +1927,7 @@ function commitRootImpl(root, renderPriorityLevel) { } // The next phase is the mutation phase, where we mutate the host tree. - nextEffect = firstEffect; - do { - if (__DEV__) { - invokeGuardedCallback( - null, - commitMutationEffects, - null, - root, - renderPriorityLevel, - ); - if (hasCaughtError()) { - invariant(nextEffect !== null, 'Should be working on an effect.'); - const error = clearCaughtError(); - captureCommitPhaseError(nextEffect, error); - nextEffect = nextEffect.nextEffect; - } - } else { - try { - commitMutationEffects(root, renderPriorityLevel); - } catch (error) { - invariant(nextEffect !== null, 'Should be working on an effect.'); - captureCommitPhaseError(nextEffect, error); - nextEffect = nextEffect.nextEffect; - } - } - } while (nextEffect !== null); + commitMutationEffects(finishedWork, root, renderPriorityLevel); if (shouldFireAfterActiveInstanceBlur) { afterActiveInstanceBlur(); @@ -1956,26 +1943,7 @@ function commitRootImpl(root, renderPriorityLevel) { // The next phase is the layout phase, where we call effects that read // the host tree after it's been mutated. The idiomatic use case for this is // layout, but class component lifecycles also fire here for legacy reasons. - nextEffect = firstEffect; - do { - if (__DEV__) { - invokeGuardedCallback(null, commitLayoutEffects, null, root, lanes); - if (hasCaughtError()) { - invariant(nextEffect !== null, 'Should be working on an effect.'); - const error = clearCaughtError(); - captureCommitPhaseError(nextEffect, error); - nextEffect = nextEffect.nextEffect; - } - } else { - try { - commitLayoutEffects(root, lanes); - } catch (error) { - invariant(nextEffect !== null, 'Should be working on an effect.'); - captureCommitPhaseError(nextEffect, error); - nextEffect = nextEffect.nextEffect; - } - } - } while (nextEffect !== null); + commitLayoutEffects(finishedWork, root, lanes); nextEffect = null; @@ -2011,6 +1979,7 @@ function commitRootImpl(root, renderPriorityLevel) { // We are done with the effect chain at this point so let's clear the // nextEffect pointers to assist with GC. If we have passive effects, we'll // clear this in flushPassiveEffects. + // TODO (effects) Traverse with subtreeTag Deletion and detatch deletions array only nextEffect = firstEffect; while (nextEffect !== null) { const nextNextEffect = nextEffect.nextEffect; @@ -2101,145 +2070,305 @@ function commitRootImpl(root, renderPriorityLevel) { return null; } -function commitBeforeMutationEffects() { - while (nextEffect !== null) { - const current = nextEffect.alternate; +function commitBeforeMutationEffects(fiber: Fiber) { + if (fiber.child !== null) { + const primarySubtreeTag = + fiber.subtreeTag & (Deletion | Snapshot | Passive | Placement); + if (primarySubtreeTag !== NoEffect) { + commitBeforeMutationEffects(fiber.child); + } + } - if (!shouldFireAfterActiveInstanceBlur && focusedInstanceHandle !== null) { - if ((nextEffect.effectTag & Deletion) !== NoEffect) { - if (doesFiberContain(nextEffect, focusedInstanceHandle)) { - shouldFireAfterActiveInstanceBlur = true; - beforeActiveInstanceBlur(); - } - } else { - // TODO: Move this out of the hot path using a dedicated effect tag. - if ( - nextEffect.tag === SuspenseComponent && - isSuspenseBoundaryBeingHidden(current, nextEffect) && - doesFiberContain(nextEffect, focusedInstanceHandle) - ) { - shouldFireAfterActiveInstanceBlur = true; - beforeActiveInstanceBlur(); - } - } + if (__DEV__) { + setCurrentDebugFiberInDEV(fiber); + invokeGuardedCallback(null, commitBeforeMutationEffectsImpl, null, fiber); + if (hasCaughtError()) { + const error = clearCaughtError(); + captureCommitPhaseError(fiber, error); + } + resetCurrentDebugFiberInDEV(); + } else { + try { + commitBeforeMutationEffectsImpl(fiber); + } catch (error) { + captureCommitPhaseError(fiber, error); } + } - const effectTag = nextEffect.effectTag; - if ((effectTag & Snapshot) !== NoEffect) { - setCurrentDebugFiberInDEV(nextEffect); + if (fiber.sibling !== null) { + commitBeforeMutationEffects(fiber.sibling); + } +} - commitBeforeMutationEffectOnFiber(current, nextEffect); +function commitBeforeMutationEffectsImpl(fiber: Fiber) { + const current = fiber.alternate; + const effectTag = fiber.effectTag; - resetCurrentDebugFiberInDEV(); + if (!shouldFireAfterActiveInstanceBlur && focusedInstanceHandle !== null) { + // The "deletions" array on a Fiber holds previous children that were marked for deletion. + // However the overall commit sequence relies on child deletions being processed before parent's effects, + // so to satisfy that we also process the parent's "deletions" array (the deletion of siblings). + commitBeforeMutationEffectsDeletions(fiber.deletions); + const parent = fiber.return; + if (parent) { + commitBeforeMutationEffectsDeletions(parent.deletions); } - if ((effectTag & Passive) !== NoEffect) { - // If there are passive effects, schedule a callback to flush at - // the earliest opportunity. - if (!rootDoesHavePassiveEffects) { - rootDoesHavePassiveEffects = true; - scheduleCallback(NormalSchedulerPriority, () => { - flushPassiveEffects(); - return null; - }); - } + + // Check to see if the focused element was inside of a hidden (Suspense) subtree. + // TODO: Move this out of the hot path using a dedicated effect tag. + if ( + fiber.tag === SuspenseComponent && + isSuspenseBoundaryBeingHidden(current, fiber) && + doesFiberContain(fiber, focusedInstanceHandle) + ) { + shouldFireAfterActiveInstanceBlur = true; + beforeActiveInstanceBlur(); + } + } + + if ((effectTag & Snapshot) !== NoEffect) { + setCurrentDebugFiberInDEV(fiber); + commitBeforeMutationEffectOnFiber(current, fiber); + resetCurrentDebugFiberInDEV(); + } + + if ((effectTag & Passive) !== NoEffect) { + // If there are passive effects, schedule a callback to flush at + // the earliest opportunity. + if (!rootDoesHavePassiveEffects) { + rootDoesHavePassiveEffects = true; + scheduleCallback(NormalSchedulerPriority, () => { + flushPassiveEffects(); + return null; + }); } - nextEffect = nextEffect.nextEffect; } } -function commitMutationEffects(root: FiberRoot, renderPriorityLevel) { - // TODO: Should probably move the bulk of this function to commitWork. - while (nextEffect !== null) { - setCurrentDebugFiberInDEV(nextEffect); +function commitBeforeMutationEffectsDeletions(deletions: Array) { + for (let i = 0; i < deletions.length; i++) { + const fiber = deletions[i]; - const effectTag = nextEffect.effectTag; + // TODO (effects) It would be nice to avoid calling doesFiberContain() + // Maybe we can repurpose one of the subtreeTag positions for this instead? + // Use it to store which part of the tree the focused instance is in? + // This assumes we can safely determine that instance during the "render" phase. - if (effectTag & ContentReset) { - commitResetTextContent(nextEffect); + if (doesFiberContain(fiber, ((focusedInstanceHandle: any): Fiber))) { + shouldFireAfterActiveInstanceBlur = true; + beforeActiveInstanceBlur(); } + } +} - if (effectTag & Ref) { - const current = nextEffect.alternate; - if (current !== null) { - commitDetachRef(current); - } +function commitMutationEffects( + fiber: Fiber, + root: FiberRoot, + renderPriorityLevel, +) { + if (fiber.child !== null) { + const primarySubtreeTag = + fiber.subtreeTag & + (ContentReset | Deletion | Hydrating | Placement | Ref | Update); + if (primarySubtreeTag !== NoEffect) { + commitMutationEffects(fiber.child, root, renderPriorityLevel); } + } - // 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 primaryEffectTag = - effectTag & (Placement | Update | Deletion | Hydrating); - switch (primaryEffectTag) { - case Placement: { - commitPlacement(nextEffect); - // 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. - nextEffect.effectTag &= ~Placement; - break; - } - case PlacementAndUpdate: { - // Placement - commitPlacement(nextEffect); - // Clear the "placement" from effect tag so that we know that this is - // inserted, before any life-cycles like componentDidMount gets called. - nextEffect.effectTag &= ~Placement; - - // Update - const current = nextEffect.alternate; - commitWork(current, nextEffect); - break; - } - case Hydrating: { - nextEffect.effectTag &= ~Hydrating; - break; - } - case HydratingAndUpdate: { - nextEffect.effectTag &= ~Hydrating; - - // Update - const current = nextEffect.alternate; - commitWork(current, nextEffect); - break; - } - case Update: { - const current = nextEffect.alternate; - commitWork(current, nextEffect); - break; - } - case Deletion: { - commitDeletion(root, nextEffect, renderPriorityLevel); - break; - } + if (__DEV__) { + setCurrentDebugFiberInDEV(fiber); + invokeGuardedCallback( + null, + commitMutationEffectsImpl, + null, + fiber, + root, + renderPriorityLevel, + ); + if (hasCaughtError()) { + const error = clearCaughtError(); + captureCommitPhaseError(fiber, error); } - resetCurrentDebugFiberInDEV(); - nextEffect = nextEffect.nextEffect; + } else { + try { + commitMutationEffectsImpl(fiber, root, renderPriorityLevel); + } catch (error) { + captureCommitPhaseError(fiber, error); + } + } + + if (fiber.sibling !== null) { + commitMutationEffects(fiber.sibling, root, renderPriorityLevel); } } -function commitLayoutEffects(root: FiberRoot, committedLanes: Lanes) { - // TODO: Should probably move the bulk of this function to commitWork. - while (nextEffect !== null) { - setCurrentDebugFiberInDEV(nextEffect); +function commitMutationEffectsImpl( + fiber: Fiber, + root: FiberRoot, + renderPriorityLevel, +) { + // The "deletions" array on a Fiber holds previous children that were marked for deletion. + // However the overall commit sequence relies on child deletions being processed before parent's effects, + // so to satisfy that we also process the parent's "deletions" array (the deletion of siblings). + commitMutationEffectsDeletions(fiber.deletions, root, renderPriorityLevel); + const parent = fiber.return; + if (parent) { + commitMutationEffectsDeletions(parent.deletions, root, renderPriorityLevel); + } + + const effectTag = fiber.effectTag; + if (effectTag & ContentReset) { + commitResetTextContent(fiber); + } + + if (effectTag & Ref) { + const current = fiber.alternate; + if (current !== null) { + commitDetachRef(current); + } + } + + // 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 primaryEffectTag = effectTag & (Placement | Update | Hydrating); + switch (primaryEffectTag) { + case Placement: { + commitPlacement(fiber); + // 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. + fiber.effectTag &= ~Placement; + break; + } + case PlacementAndUpdate: { + // Placement + commitPlacement(fiber); + // Clear the "placement" from effect tag so that we know that this is + // inserted, before any life-cycles like componentDidMount gets called. + fiber.effectTag &= ~Placement; + + // Update + const current = fiber.alternate; + commitWork(current, fiber); + break; + } + case Hydrating: { + fiber.effectTag &= ~Hydrating; + break; + } + case HydratingAndUpdate: { + fiber.effectTag &= ~Hydrating; - const effectTag = nextEffect.effectTag; + // Update + const current = fiber.alternate; + commitWork(current, fiber); + break; + } + case Update: { + const current = fiber.alternate; + commitWork(current, fiber); + break; + } + } +} - if (effectTag & (Update | Callback)) { - const current = nextEffect.alternate; - commitLayoutEffectOnFiber(root, current, nextEffect, committedLanes); +function commitMutationEffectsDeletions( + deletions: Array, + root: FiberRoot, + renderPriorityLevel, +) { + for (let i = 0; i < deletions.length; i++) { + const childToDelete = deletions[i]; + if (__DEV__) { + invokeGuardedCallback( + null, + commitDeletion, + null, + root, + childToDelete, + renderPriorityLevel, + ); + if (hasCaughtError()) { + const error = clearCaughtError(); + captureCommitPhaseError(childToDelete, error); + } + } else { + try { + commitDeletion(root, childToDelete, renderPriorityLevel); + } catch (error) { + captureCommitPhaseError(childToDelete, error); + } } + // Don't clear the Deletion effect yet; we also use it to know when we need to detach refs later. + } + + // TODO (effects) Don't clear this yet; we may need to cleanup passive effects + deletions.splice(0); +} - if (effectTag & Ref) { - commitAttachRef(nextEffect); +function commitLayoutEffects( + fiber: Fiber, + root: FiberRoot, + committedLanes: Lanes, +) { + if (fiber.child !== null) { + const primarySubtreeTag = fiber.subtreeTag & (Update | Callback | Ref); + if (primarySubtreeTag !== NoEffect) { + commitLayoutEffects(fiber.child, root, committedLanes); } + } + if (__DEV__) { + setCurrentDebugFiberInDEV(fiber); + invokeGuardedCallback( + null, + commitLayoutEffectsImpl, + null, + fiber, + root, + committedLanes, + ); + if (hasCaughtError()) { + const error = clearCaughtError(); + captureCommitPhaseError(fiber, error); + } resetCurrentDebugFiberInDEV(); - nextEffect = nextEffect.nextEffect; + } else { + try { + commitLayoutEffectsImpl(fiber, root, committedLanes); + } catch (error) { + captureCommitPhaseError(fiber, error); + } + } + + if (fiber.sibling !== null) { + commitLayoutEffects(fiber.sibling, root, committedLanes); + } +} + +function commitLayoutEffectsImpl( + fiber: Fiber, + root: FiberRoot, + committedLanes: Lanes, +) { + const effectTag = fiber.effectTag; + + setCurrentDebugFiberInDEV(fiber); + + if (effectTag & (Update | Callback)) { + const current = fiber.alternate; + commitLayoutEffectOnFiber(root, current, fiber, committedLanes); + } + + if (effectTag & Ref) { + commitAttachRef(fiber); } + + resetCurrentDebugFiberInDEV(); } export function flushPassiveEffects() { @@ -2448,6 +2577,7 @@ function flushPassiveEffectsImpl() { // Note: This currently assumes there are no passive effects on the root fiber // because the root is not part of its own effect list. // This could change in the future. + // TODO (effects) Traverse with subtreeTag Deletion and detatch deletions array only let effect = root.current.firstEffect; while (effect !== null) { const nextNextEffect = effect.nextEffect; diff --git a/packages/react-reconciler/src/ReactInternalTypes.js b/packages/react-reconciler/src/ReactInternalTypes.js index 648b3d537af3b..e75ead706ae60 100644 --- a/packages/react-reconciler/src/ReactInternalTypes.js +++ b/packages/react-reconciler/src/ReactInternalTypes.js @@ -126,6 +126,8 @@ export type Fiber = {| // Effect effectTag: SideEffectTag, + subtreeTag: SideEffectTag, + deletions: Array, // Singly linked list fast path to the next fiber with side-effects. nextEffect: Fiber | null, diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js index a8ffa822960a4..12e2d600c6e26 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js @@ -1733,15 +1733,30 @@ describe('ReactSuspenseWithNoopRenderer', () => { expect(Scheduler).toHaveYielded(['Promise resolved [B2]']); - expect(Scheduler).toFlushAndYield([ - 'B2', - 'Destroy Layout Effect [Loading...]', - 'Destroy Layout Effect [B]', - 'Layout Effect [B2]', - 'Destroy Effect [Loading...]', - 'Destroy Effect [B]', - 'Effect [B2]', - ]); + // Slight sequencing difference between old and new reconcilers, + // because walking the tree during the commit phase means processing deletions + // as part of processing the parent rather than the child. + gate(flags => + flags.new + ? expect(Scheduler).toFlushAndYield([ + 'B2', + 'Destroy Layout Effect [B]', + 'Destroy Layout Effect [Loading...]', + 'Layout Effect [B2]', + 'Destroy Effect [Loading...]', + 'Destroy Effect [B]', + 'Effect [B2]', + ]) + : expect(Scheduler).toFlushAndYield([ + 'B2', + 'Destroy Layout Effect [Loading...]', + 'Destroy Layout Effect [B]', + 'Layout Effect [B2]', + 'Destroy Effect [Loading...]', + 'Destroy Effect [B]', + 'Effect [B2]', + ]), + ); }); it('suspends for longer if something took a long (CPU bound) time to render', async () => {