From ce5c42152c307f8b73e7abdb253e7f3c01f5b637 Mon Sep 17 00:00:00 2001 From: Luna Date: Mon, 22 Aug 2022 11:12:06 +0100 Subject: [PATCH] Refactor code --- .../src/ReactFiberBeginWork.new.js | 9 +- .../src/ReactFiberCommitWork.new.js | 355 ++++++++++-------- .../src/ReactFiberCompleteWork.new.js | 10 - .../ReactFiberTracingMarkerComponent.new.js | 52 ++- .../src/ReactInternalTypes.js | 2 +- 5 files changed, 241 insertions(+), 187 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.new.js b/packages/react-reconciler/src/ReactFiberBeginWork.new.js index b62c3b042174e..fa417e08114c9 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.new.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.new.js @@ -89,6 +89,7 @@ import { StaticMask, ShouldCapture, ForceClientRender, + Passive, } from './ReactFiberFlags'; import ReactSharedInternals from 'shared/ReactSharedInternals'; import { @@ -979,11 +980,17 @@ function updateTracingMarkerComponent( const markerInstance: TracingMarkerInstance = { tag: TransitionTracingMarker, transitions: new Set(currentTransitions), - pendingBoundaries: new Map(), + pendingBoundaries: null, name: workInProgress.pendingProps.name, aborts: null, }; workInProgress.stateNode = markerInstance; + + // We call the marker complete callback when all child suspense boundaries resolve. + // We do this in the commit phase on Offscreen. If the marker has no child suspense + // boundaries, we need to schedule a passive effect to make sure we call the marker + // complete callback. + workInProgress.flags |= Passive; } } else { if (__DEV__) { diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index d3e3c47378b9f..4e921de5c0ee9 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js @@ -1140,96 +1140,136 @@ function commitLayoutEffectOnFiber( } } -function abortParentMarkerTransitions( - deletedFiber: Fiber, - nearestMountedAncestor: Fiber, +function abortRootTransitions( + root: FiberRoot, abort: TransitionAbort, + deletedTransitions: Set, + deletedOffscreenInstance: OffscreenInstance | null, + isInDeletedTree: boolean, ) { - const deletedFiberInstance: OffscreenInstance = deletedFiber.stateNode; - - let fiber = deletedFiber; - let isInDeletedTree = true; - while (fiber !== null) { - switch (fiber.tag) { - case TracingMarkerComponent: - const transitions = deletedFiberInstance.transitions; + if (enableTransitionTracing) { + const rootTransitions = root.incompleteTransitions; + deletedTransitions.forEach(transition => { + if (rootTransitions.has(transition)) { + const transitionInstance: TracingMarkerInstance = (rootTransitions.get( + transition, + ): any); + if (transitionInstance.aborts === null) { + transitionInstance.aborts = []; + } + transitionInstance.aborts.push(abort); - const markerInstance = fiber.stateNode; - const markerTransitions = markerInstance.transitions; - if (markerTransitions !== null && transitions !== null) { - let abortMarker = false; - transitions.forEach(transition => { - if (markerTransitions.has(transition)) { - abortMarker = true; - } - }); + if (deletedOffscreenInstance !== null) { + if ( + transitionInstance.pendingBoundaries !== null && + transitionInstance.pendingBoundaries.has(deletedOffscreenInstance) + ) { + transitionInstance.pendingBoundaries.delete( + deletedOffscreenInstance, + ); + } + } + } + }); + } +} - if (abortMarker) { +function abortTracingMarkerTransitions( + abortedFiber: Fiber, + abort: TransitionAbort, + deletedTransitions: Set, + deletedOffscreenInstance: OffscreenInstance | null, + isInDeletedTree: boolean, +) { + if (enableTransitionTracing) { + const markerInstance: TracingMarkerInstance = abortedFiber.stateNode; + const markerTransitions = markerInstance.transitions; + const pendingBoundaries = markerInstance.pendingBoundaries; + if (markerTransitions !== null) { + // TODO: Refactor this code. Is there a way to move this code to + // the deletions phase instead of calculating it here while making sure + // complete is called appropriately? + deletedTransitions.forEach(transition => { + // If one of the transitions on the tracing marker is a transition + // that was in an aborted subtree, we will abort that tracing marker + if ( + abortedFiber !== null && + markerTransitions.has(transition) && + (markerInstance.aborts === null || + !markerInstance.aborts.includes(abort)) + ) { + if (markerInstance.transitions !== null) { if (markerInstance.aborts === null) { - markerInstance.aborts = new Set(); + markerInstance.aborts = [abort]; + addMarkerIncompleteCallbackToPendingTransition( + abortedFiber.memoizedProps.name, + markerInstance.transitions, + markerInstance.aborts, + ); + } else { + markerInstance.aborts.push(abort); } - markerInstance.aborts.add(abort); - addMarkerIncompleteCallbackToPendingTransition( - fiber.memoizedProps.name, - transitions, - markerInstance.aborts, - ); - // We only want to call onTransitionProgress when the marker hasn't been // deleted if ( + deletedOffscreenInstance !== null && !isInDeletedTree && - markerInstance.pendingBoundaries !== null && - markerInstance.pendingBoundaries.has(deletedFiberInstance) + pendingBoundaries !== null && + pendingBoundaries.has(deletedOffscreenInstance) ) { - markerInstance.pendingBoundaries.delete(deletedFiberInstance); + pendingBoundaries.delete(deletedOffscreenInstance); addMarkerProgressCallbackToPendingTransition( - fiber.memoizedProps.name, - transitions, - markerInstance.pendingBoundaries, + abortedFiber.memoizedProps.name, + deletedTransitions, + pendingBoundaries, ); } } } - break; - case HostRoot: - const root = fiber.stateNode; - const incompleteTransitions = root.incompleteTransitions; - - if (deletedFiberInstance.transitions !== null) { - deletedFiberInstance.transitions.forEach(transition => { - if (incompleteTransitions.has(transition)) { - const transitionInstance = incompleteTransitions.get(transition); - if (transitionInstance.aborts === null) { - transitionInstance.aborts = []; - } - transitionInstance.aborts.push(abort); - - if ( - transitionInstance.pendingBoundaries !== null && - transitionInstance.pendingBoundaries.has(deletedFiberInstance) - ) { - transitionInstance.pendingBoundaries.delete( - deletedFiberInstance, - ); - } - } - }); - } - break; - default: - break; + }); } + } +} + +function abortParentMarkerTransitionsForDeletedFiber( + abortedFiber: Fiber, + abort: TransitionAbort, + deletedTransitions: Set, + deletedOffscreenInstance: OffscreenInstance | null, + isInDeletedTree: boolean, +) { + if (enableTransitionTracing) { + // Find all pending markers that are waiting on child suspense boundaries in the + // aborted subtree and cancels them + let fiber = abortedFiber; + while (fiber !== null) { + switch (fiber.tag) { + case TracingMarkerComponent: + abortTracingMarkerTransitions( + fiber, + abort, + deletedTransitions, + deletedOffscreenInstance, + isInDeletedTree, + ); + break; + case HostRoot: + const root = fiber.stateNode; + abortRootTransitions( + root, + abort, + deletedTransitions, + deletedOffscreenInstance, + isInDeletedTree, + ); + + break; + default: + break; + } - if ( - nearestMountedAncestor.deletions !== null && - nearestMountedAncestor.deletions.includes(fiber) - ) { - isInDeletedTree = false; - fiber = nearestMountedAncestor; - } else { fiber = fiber.return; } } @@ -1280,6 +1320,7 @@ function commitTransitionProgress(offscreenFiber: Fiber) { pendingMarkers.forEach(markerInstance => { const pendingBoundaries = markerInstance.pendingBoundaries; const transitions = markerInstance.transitions; + const markerName = markerInstance.name; if ( pendingBoundaries !== null && !pendingBoundaries.has(offscreenInstance) @@ -1290,10 +1331,10 @@ function commitTransitionProgress(offscreenFiber: Fiber) { if (transitions !== null) { if ( markerInstance.tag === TransitionTracingMarker && - markerInstance.name !== undefined + markerName !== null ) { addMarkerProgressCallbackToPendingTransition( - markerInstance.name, + markerName, transitions, pendingBoundaries, ); @@ -1317,6 +1358,7 @@ function commitTransitionProgress(offscreenFiber: Fiber) { pendingMarkers.forEach(markerInstance => { const pendingBoundaries = markerInstance.pendingBoundaries; const transitions = markerInstance.transitions; + const markerName = markerInstance.name; if ( pendingBoundaries !== null && pendingBoundaries.has(offscreenInstance) @@ -1325,13 +1367,27 @@ function commitTransitionProgress(offscreenFiber: Fiber) { if (transitions !== null) { if ( markerInstance.tag === TransitionTracingMarker && - markerInstance.name !== undefined + markerName !== null ) { addMarkerProgressCallbackToPendingTransition( - markerInstance.name, + markerName, transitions, pendingBoundaries, ); + + // If there are no more unresolved suspense boundaries, the interaction + // is considered finished + if (pendingBoundaries.size === 0) { + if (markerInstance.aborts === null) { + addMarkerCompleteCallbackToPendingTransition( + markerName, + transitions, + ); + } + markerInstance.transitions = null; + markerInstance.pendingBoundaries = null; + markerInstance.aborts = null; + } } else if (markerInstance.tag === TransitionRoot) { transitions.forEach(transition => { addTransitionProgressCallbackToPendingTransition( @@ -1850,6 +1906,7 @@ function commitDeletionEffects( 'a bug in React. Please file an issue.', ); } + commitDeletionEffectsOnFiber(root, returnFiber, deletedFiber); hostParent = null; hostParentIsContainer = false; @@ -2097,28 +2154,6 @@ function commitDeletionEffectsOnFiber( offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden || deletedFiber.memoizedState !== null; - if (enableTransitionTracing) { - // We need to mark this fiber's parents as deleted - const instance: OffscreenInstance = deletedFiber.stateNode; - const transitions = instance.transitions; - if (transitions !== null) { - let name = null; - const parent = deletedFiber.return; - if ( - parent !== null && - parent.tag === SuspenseComponent && - parent.memoizedProps.unstable_name - ) { - name = parent.memoizedProps.unstable_name; - } - - abortParentMarkerTransitions(deletedFiber, nearestMountedAncestor, { - reason: 'suspense', - name, - }); - } - } - recursivelyTraverseDeletionEffects( finishedRoot, nearestMountedAncestor, @@ -2134,30 +2169,6 @@ function commitDeletionEffectsOnFiber( } break; } - case TracingMarkerComponent: { - if (enableTransitionTracing) { - // We need to mark this fiber's parents as deleted - const instance: TracingMarkerInstance = deletedFiber.stateNode; - const transitions = instance.transitions; - if (transitions !== null) { - const abort = { - reason: 'marker', - name: deletedFiber.memoizedProps.name, - }; - abortParentMarkerTransitions( - deletedFiber, - nearestMountedAncestor, - abort, - ); - } - } - recursivelyTraverseDeletionEffects( - finishedRoot, - nearestMountedAncestor, - deletedFiber, - ); - return; - } default: { recursivelyTraverseDeletionEffects( finishedRoot, @@ -3134,6 +3145,7 @@ function commitOffscreenPassiveMountEffects( commitTransitionProgress(finishedWork); + // TODO: Refactor this into an if/else branch if (!isHidden) { instance.transitions = null; instance.pendingMarkers = null; @@ -3168,18 +3180,14 @@ function commitCachePassiveMountEffect( function commitTracingMarkerPassiveMountEffect(finishedWork: Fiber) { // Get the transitions that were initiatized during the render // and add a start transition callback for each of them + // We will only call this on initial mount of the tracing marker + // only if there are no suspense children const instance = finishedWork.stateNode; - if ( - instance.transitions !== null && - (instance.pendingBoundaries === null || - instance.pendingBoundaries.size === 0) - ) { - if (instance.aborts === null) { - addMarkerCompleteCallbackToPendingTransition( - finishedWork.memoizedProps.name, - instance.transitions, - ); - } + if (instance.transitions !== null && instance.pendingBoundaries === null) { + addMarkerCompleteCallbackToPendingTransition( + finishedWork.memoizedProps.name, + instance.transitions, + ); instance.transitions = null; instance.pendingBoundaries = null; instance.aborts = null; @@ -3285,7 +3293,7 @@ function commitPassiveMountOnFiber( if (enableTransitionTracing) { // Get the transitions that were initiatized during the render // and add a start transition callback for each of them - const root = finishedWork.stateNode; + const root: FiberRoot = finishedWork.stateNode; const incompleteTransitions = root.incompleteTransitions; // Initial render if (committedTransitions !== null) { @@ -3674,21 +3682,6 @@ function commitAtomicPassiveEffects( } break; } - case TracingMarkerComponent: { - if (enableTransitionTracing) { - recursivelyTraverseAtomicPassiveEffects( - finishedRoot, - finishedWork, - committedLanes, - committedTransitions, - ); - if (flags & Passive) { - commitTracingMarkerPassiveMountEffect(finishedWork); - } - break; - } - // Intentional fallthrough to next branch - } // eslint-disable-next-line-no-fallthrough default: { recursivelyTraverseAtomicPassiveEffects( @@ -4016,6 +4009,43 @@ function commitPassiveUnmountInsideDeletedTreeOnFiber( } break; } + case SuspenseComponent: { + if (enableTransitionTracing) { + // We need to mark this fiber's parents as deleted + const offscreenFiber: Fiber = (current.child: any); + const instance: OffscreenInstance = offscreenFiber.stateNode; + const transitions = instance.transitions; + if (transitions !== null) { + const abortReason = { + reason: 'suspense', + name: current.memoizedProps.unstable_name || null, + }; + if ( + current.memoizedState === null || + current.memoizedState.dehydrated === null + ) { + abortParentMarkerTransitionsForDeletedFiber( + offscreenFiber, + abortReason, + transitions, + instance, + true, + ); + + if (nearestMountedAncestor !== null) { + abortParentMarkerTransitionsForDeletedFiber( + nearestMountedAncestor, + abortReason, + transitions, + instance, + false, + ); + } + } + } + } + break; + } case CacheComponent: { if (enableCache) { const cache = current.memoizedState.cache; @@ -4023,6 +4053,37 @@ function commitPassiveUnmountInsideDeletedTreeOnFiber( } break; } + case TracingMarkerComponent: { + if (enableTransitionTracing) { + // We need to mark this fiber's parents as deleted + const instance: TracingMarkerInstance = current.stateNode; + const transitions = instance.transitions; + if (transitions !== null) { + const abortReason = { + reason: 'marker', + name: current.memoizedProps.name, + }; + abortParentMarkerTransitionsForDeletedFiber( + current, + abortReason, + transitions, + null, + true, + ); + + if (nearestMountedAncestor !== null) { + abortParentMarkerTransitionsForDeletedFiber( + nearestMountedAncestor, + abortReason, + transitions, + null, + false, + ); + } + } + } + break; + } } } diff --git a/packages/react-reconciler/src/ReactFiberCompleteWork.new.js b/packages/react-reconciler/src/ReactFiberCompleteWork.new.js index 7a8843941eee5..135e9290f549f 100644 --- a/packages/react-reconciler/src/ReactFiberCompleteWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCompleteWork.new.js @@ -1589,16 +1589,6 @@ function completeWork( popMarkerInstance(workInProgress); } bubbleProperties(workInProgress); - - if ( - current === null || - (workInProgress.subtreeFlags & Visibility) !== NoFlags - ) { - // If any of our suspense children toggle visibility, this means that - // the pending boundaries array needs to be updated, which we only - // do in the passive phase. - workInProgress.flags |= Passive; - } } return null; } diff --git a/packages/react-reconciler/src/ReactFiberTracingMarkerComponent.new.js b/packages/react-reconciler/src/ReactFiberTracingMarkerComponent.new.js index 044418c13ea5c..724f9bdbd5944 100644 --- a/packages/react-reconciler/src/ReactFiberTracingMarkerComponent.new.js +++ b/packages/react-reconciler/src/ReactFiberTracingMarkerComponent.new.js @@ -7,7 +7,7 @@ * @flow */ -import type {TransitionTracingCallbacks, Fiber} from './ReactInternalTypes'; +import type {TransitionTracingCallbacks, Fiber, FiberRoot} from './ReactInternalTypes'; import type {OffscreenInstance} from './ReactFiberOffscreenComponent'; import type {StackCursor} from './ReactFiberStack.new'; @@ -43,6 +43,7 @@ export type BatchConfigTransition = { _updatedFibers?: Set, }; +// TODO: Is there a way to not include the tag or name here? export type TracingMarkerInstance = {| tag?: TracingMarkerTag, transitions: Set | null, @@ -121,10 +122,27 @@ export function processTransitionCallbacks( markerIncomplete.forEach(({transitions, aborts}, markerName) => { transitions.forEach(transition => { const filteredAborts = []; - aborts.forEach(deletion => { - const filteredDeletion = getFilteredDeletion(deletion, endTime); - if (filteredDeletion !== null) { - filteredAborts.push(filteredDeletion); + aborts.forEach(abort => { + switch (abort.reason) { + case 'marker': { + filteredAborts.push({ + type: 'marker', + name: abort.name, + endTime, + }); + break; + } + case 'suspense': { + filteredAborts.push({ + type: 'suspense', + name: abort.name, + endTime, + }); + break; + } + default: { + break; + } } }); @@ -164,28 +182,6 @@ export function processTransitionCallbacks( } } -function getFilteredDeletion(abort: TransitionAbort, endTime: number) { - switch (abort.reason) { - case 'marker': { - return { - type: 'marker', - name: abort.name, - endTime, - }; - } - case 'suspense': { - return { - type: 'suspense', - name: abort.name, - endTime, - }; - } - default: { - return null; - } - } -} - // For every tracing marker, store a pointer to it. We will later access it // to get the set of suspense boundaries that need to resolve before the // tracing marker can be logged as complete @@ -206,7 +202,7 @@ export function pushRootMarkerInstance(workInProgress: Fiber): void { // transitions map. Each entry in this map functions like a tracing // marker does, so we can push it onto the marker instance stack const transitions = getWorkInProgressTransitions(); - const root = workInProgress.stateNode; + const root: FiberRoot = workInProgress.stateNode; if (transitions !== null) { transitions.forEach(transition => { diff --git a/packages/react-reconciler/src/ReactInternalTypes.js b/packages/react-reconciler/src/ReactInternalTypes.js index d5ed83d34a077..141f7a8f59638 100644 --- a/packages/react-reconciler/src/ReactInternalTypes.js +++ b/packages/react-reconciler/src/ReactInternalTypes.js @@ -335,7 +335,7 @@ type TransitionTracingOnlyFiberRootProperties = {| // are considered complete when the pending suspense boundaries set is // empty. We can represent this as a Map of transitions to suspense // boundary sets - incompleteTransitions: Map, TracingMarkerInstance>, + incompleteTransitions: Map, |}; // Exported FiberRoot type includes all properties,