Skip to content

Commit

Permalink
[DevTools] Unmount by walking previous nodes no longer in the new tree (
Browse files Browse the repository at this point in the history 
#30644)

This no longer uses the handleCommitFiberUnmount hook to track unmounts.
Instead, we can just unmount the DevToolsInstances that we didn't reuse.
This doesn't account for cleaning up instances that were unnecessarily
created when they weren't in the tree. I have a separate follow up for
that.

This also removes the queuing of untracking. This was added in #21523
but I don't believe it has been needed for a while because the mentioned
flushPendingErrorsAndWarningsAfterDelay hasn't called untrackFiberID for
a while so the race condition doesn't exist. It's hard to tell though
because from the issues there weren't really any repros submitted.
  • Loading branch information
@sebmarkbage
sebmarkbage authored Aug 12, 2024
1 parent 8d74e8c commit 68dbd84
Showing 1 changed file with 51 additions and 92 deletions.
143 changes: 51 additions & 92 deletions packages/react-devtools-shared/src/backend/fiber/renderer.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1405,81 +1405,37 @@ export function attach(

// Removes a Fiber (and its alternate) from the Maps used to track their id.
// This method should always be called when a Fiber is unmounting.
function untrackFiberID(fiber: Fiber) {
function untrackFiber(fiberInstance: FiberInstance) {
if (__DEBUG__) {
debug('untrackFiberID()', fiber, null, 'schedule after delay');
debug('untrackFiber()', fiberInstance.data, null);
}

// Untrack Fibers after a slight delay in order to support a Fast Refresh edge case:
// 1. Component type is updated and Fast Refresh schedules an update+remount.
// 2. flushPendingErrorsAndWarningsAfterDelay() runs, sees the old Fiber is no longer mounted
// (it's been disconnected by Fast Refresh), and calls untrackFiberID() to clear it from the Map.
// 3. React flushes pending passive effects before it runs the next render,
// which logs an error or warning, which causes a new ID to be generated for this Fiber.
// 4. DevTools now tries to unmount the old Component with the new ID.
//
// The underlying problem here is the premature clearing of the Fiber ID,
// but DevTools has no way to detect that a given Fiber has been scheduled for Fast Refresh.
// (The "_debugNeedsRemount" flag won't necessarily be set.)
//
// The best we can do is to delay untracking by a small amount,
// and give React time to process the Fast Refresh delay.

untrackFibersSet.add(fiber);
idToDevToolsInstanceMap.delete(fiberInstance.id);

// React may detach alternate pointers during unmount;
// Since our untracking code is async, we should explicily track the pending alternate here as well.
const alternate = fiber.alternate;
if (alternate !== null) {
untrackFibersSet.add(alternate);
// Also clear any errors/warnings associated with this fiber.
clearErrorsForElementID(fiberInstance.id);
clearWarningsForElementID(fiberInstance.id);
if (fiberInstance.flags & FORCE_ERROR) {
fiberInstance.flags &= ~FORCE_ERROR;
forceErrorCount--;
if (forceErrorCount === 0 && setErrorHandler != null) {
setErrorHandler(shouldErrorFiberAlwaysNull);
}
}

if (untrackFibersTimeoutID === null) {
untrackFibersTimeoutID = setTimeout(untrackFibers, 1000);
if (fiberInstance.flags & FORCE_SUSPENSE_FALLBACK) {
fiberInstance.flags &= ~FORCE_SUSPENSE_FALLBACK;
forceFallbackCount--;
if (forceFallbackCount === 0 && setSuspenseHandler != null) {
setSuspenseHandler(shouldSuspendFiberAlwaysFalse);
}
}
}

const untrackFibersSet: Set<Fiber> = new Set();
let untrackFibersTimeoutID: TimeoutID | null = null;

function untrackFibers() {
if (untrackFibersTimeoutID !== null) {
clearTimeout(untrackFibersTimeoutID);
untrackFibersTimeoutID = null;
const fiber = fiberInstance.data;
fiberToFiberInstanceMap.delete(fiber);
const {alternate} = fiber;
if (alternate !== null) {
fiberToFiberInstanceMap.delete(alternate);
}

untrackFibersSet.forEach(fiber => {
const fiberInstance = fiberToFiberInstanceMap.get(fiber);
if (fiberInstance !== undefined) {
idToDevToolsInstanceMap.delete(fiberInstance.id);

// Also clear any errors/warnings associated with this fiber.
clearErrorsForElementID(fiberInstance.id);
clearWarningsForElementID(fiberInstance.id);
if (fiberInstance.flags & FORCE_ERROR) {
fiberInstance.flags &= ~FORCE_ERROR;
forceErrorCount--;
if (forceErrorCount === 0 && setErrorHandler != null) {
setErrorHandler(shouldErrorFiberAlwaysNull);
}
}
if (fiberInstance.flags & FORCE_SUSPENSE_FALLBACK) {
fiberInstance.flags &= ~FORCE_SUSPENSE_FALLBACK;
forceFallbackCount--;
if (forceFallbackCount === 0 && setSuspenseHandler != null) {
setSuspenseHandler(shouldSuspendFiberAlwaysFalse);
}
}
}

fiberToFiberInstanceMap.delete(fiber);

const {alternate} = fiber;
if (alternate !== null) {
fiberToFiberInstanceMap.delete(alternate);
}
});
untrackFibersSet.clear();
}

function getChangeDescription(
Expand Down Expand Up @@ -2054,7 +2010,7 @@ export function attach(
// Fill in the real unmounts in the reverse order.
// They were inserted parents-first by React, but we want children-first.
// So we traverse our array backwards.
for (let j = pendingRealUnmountedIDs.length - 1; j >= 0; j--) {
for (let j = 0; j < pendingRealUnmountedIDs.length; j++) {
operations[i++] = pendingRealUnmountedIDs[j];
}
// Fill in the simulated unmounts (hidden Suspense subtrees) in their order.
Expand Down Expand Up @@ -2282,7 +2238,7 @@ export function attach(
}

if (!fiber._debugNeedsRemount) {
untrackFiberID(fiber);
untrackFiber(fiberInstance);

const isProfilingSupported = fiber.hasOwnProperty('treeBaseDuration');
if (isProfilingSupported) {
Expand Down Expand Up @@ -2363,6 +2319,17 @@ export function attach(
instance.parent = null;
}

function unmountRemainingChildren() {
let child = remainingReconcilingChildren;
while (child !== null) {
if (child.kind === FIBER_INSTANCE) {
unmountFiberRecursively(child.data, false);
}
removeChild(child);
child = remainingReconcilingChildren;
}
}

function mountChildrenRecursively(
firstChild: Fiber,
traceNearestHostComponentUpdate: boolean,
Expand Down Expand Up @@ -2484,8 +2451,8 @@ export function attach(
}

// We use this to simulate unmounting for Suspense trees
// when we switch from primary to fallback.
function unmountFiberRecursively(fiber: Fiber) {
// when we switch from primary to fallback, or deleting a subtree.
function unmountFiberRecursively(fiber: Fiber, isSimulated: boolean) {
if (__DEBUG__) {
debug('unmountFiberRecursively()', fiber, null);
}
Expand Down Expand Up @@ -2526,7 +2493,7 @@ export function attach(
child = fallbackChildFragment ? fallbackChildFragment.child : null;
}

unmountChildrenRecursively(child);
unmountChildrenRecursively(child, isSimulated);
} finally {
if (shouldIncludeInTree) {
reconcilingParent = stashedParent;
Expand All @@ -2535,19 +2502,20 @@ export function attach(
}
}
if (fiberInstance !== null) {
recordUnmount(fiber, true);
recordUnmount(fiber, isSimulated);
removeChild(fiberInstance);
}
}

function unmountChildrenRecursively(firstChild: null | Fiber) {
function unmountChildrenRecursively(
firstChild: null | Fiber,
isSimulated: boolean,
) {
let child: null | Fiber = firstChild;
while (child !== null) {
// Record simulated unmounts children-first.
// We skip nodes without return because those are real unmounts.
if (child.return !== null) {
unmountFiberRecursively(child);
}
unmountFiberRecursively(child, isSimulated);
child = child.sibling;
}
}
Expand Down Expand Up @@ -2890,7 +2858,7 @@ export function attach(
// 1. Hide primary set
// This is not a real unmount, so it won't get reported by React.
// We need to manually walk the previous tree and record unmounts.
unmountChildrenRecursively(prevFiber.child);
unmountChildrenRecursively(prevFiber.child, true);
// 2. Mount fallback set
const nextFiberChild = nextFiber.child;
const nextFallbackChildSet = nextFiberChild
Expand Down Expand Up @@ -2922,6 +2890,7 @@ export function attach(
// All the remaining children will be children of this same fiber so we can just reuse them.
// I.e. we just restore them by undoing what we did above.
fiberInstance.firstChild = remainingReconcilingChildren;
remainingReconcilingChildren = null;
} else {
// If this fiber is filtered there might be changes to this set elsewhere so we have
// to visit each child to place it back in the set. We let the child bail out instead.
Expand Down Expand Up @@ -2984,6 +2953,7 @@ export function attach(
}
} finally {
if (shouldIncludeInTree) {
unmountRemainingChildren();
reconcilingParent = stashedParent;
previouslyReconciledSibling = stashedPrevious;
remainingReconcilingChildren = stashedRemaining;
Expand Down Expand Up @@ -3068,15 +3038,8 @@ export function attach(
}

function handleCommitFiberUnmount(fiber: any) {
// If the untrackFiberSet already has the unmounted Fiber, this means we've already
// recordedUnmount, so we don't need to do it again. If we don't do this, we might
// end up double-deleting Fibers in some cases (like Legacy Suspense).
if (!untrackFibersSet.has(fiber)) {
// This is not recursive.
// We can't traverse fibers after unmounting so instead
// we rely on React telling us about each unmount.
recordUnmount(fiber, false);
}
// This Hook is no longer used. After having shipped DevTools everywhere it is
// safe to stop calling it from Fiber.
}

function handlePostCommitFiberRoot(root: any) {
Expand All @@ -3097,10 +3060,6 @@ export function attach(
const current = root.current;
const alternate = current.alternate;

// Flush any pending Fibers that we are untracking before processing the new commit.
// If we don't do this, we might end up double-deleting Fibers in some cases (like Legacy Suspense).
untrackFibers();

currentRootID = getOrGenerateFiberInstance(current).id;

// Before the traversals, remember to start tracking
Expand Down Expand Up @@ -3158,7 +3117,7 @@ export function attach(
} else if (wasMounted && !isMounted) {
// Unmount an existing root.
removeRootPseudoKey(currentRootID);
recordUnmount(current, false);
unmountFiberRecursively(alternate, false);
}
} else {
// Mount a new root.
Expand Down

0 comments on commit 68dbd84

Please sign in to comment.