Track deletions using an array on the parent
Adds back the `deletions` array and uses it in the commit phase. We use a trick where the first time we hit a deletion effect, we commit all the deletion effects that belong to that parent. This is an incremental step away from using the effect list and toward a DFS + subtreeFlags traversal. This will help determine whether the regression is caused by, say, pushing the same fiber into the deletions array multiple times.
Andrew Clark committed
Nov 16, 2020 at 16:01 UTC
de75315d7ed388493f6d42fccd15ae3638adebe0
9 files changed
+203
-39
packages/react-reconciler/src/ReactChildFiber.new.js
+18
-1
@@ -13,7 +13,7 @@ import type {Fiber} from './ReactInternalTypes';
13
import type {Lanes} from './ReactFiberLane.new';
14
15
import getComponentName from 'shared/getComponentName';
16
-import {Placement, Deletion} from './ReactFiberFlags';
16
+import {Deletion, ChildDeletion, Placement} from './ReactFiberFlags';
17
import {
18
getIteratorFn,
19
REACT_ELEMENT_TYPE,
@@ -276,6 +276,23 @@ function ChildReconciler(shouldTrackSideEffects) {
276
}
277
childToDelete.nextEffect = null;
278
childToDelete.flags = Deletion;
279
+
280
+ let deletions = returnFiber.deletions;
281
+ if (deletions === null) {
282
+ deletions = returnFiber.deletions = [childToDelete];
283
+ returnFiber.flags |= ChildDeletion;
284
+ } else {
285
+ deletions.push(childToDelete);
286
+ }
287
+ // Stash a reference to the return fiber's deletion array on each of the
288
+ // deleted children. This is really weird, but it's a temporary workaround
289
+ // while we're still using the effect list to traverse effect fibers. A
290
+ // better workaround would be to follow the `.return` pointer in the commit
291
+ // phase, but unfortunately we can't assume that `.return` points to the
292
+ // correct fiber, even in the commit phase, because `findDOMNode` might
293
+ // mutate it.
294
+ // TODO: Remove this line.
295
+ childToDelete.deletions = deletions;
296
}
297
298
function deleteRemainingChildren(
packages/react-reconciler/src/ReactChildFiber.old.js
+19
-1
@@ -13,7 +13,7 @@ import type {Fiber} from './ReactInternalTypes';
13
import type {Lanes} from './ReactFiberLane.old';
14
15
import getComponentName from 'shared/getComponentName';
16
-import {Placement, Deletion} from './ReactFiberFlags';
16
+import {Deletion, ChildDeletion, Placement} from './ReactFiberFlags';
17
import {
18
getIteratorFn,
19
REACT_ELEMENT_TYPE,
@@ -276,6 +276,23 @@ function ChildReconciler(shouldTrackSideEffects) {
276
}
277
childToDelete.nextEffect = null;
278
childToDelete.flags = Deletion;
279
+
280
+ let deletions = returnFiber.deletions;
281
+ if (deletions === null) {
282
+ deletions = returnFiber.deletions = [childToDelete];
283
+ returnFiber.flags |= ChildDeletion;
284
+ } else {
285
+ deletions.push(childToDelete);
286
+ }
287
+ // Stash a reference to the return fiber's deletion array on each of the
288
+ // deleted children. This is really weird, but it's a temporary workaround
289
+ // while we're still using the effect list to traverse effect fibers. A
290
+ // better workaround would be to follow the `.return` pointer in the commit
291
+ // phase, but unfortunately we can't assume that `.return` points to the
292
+ // correct fiber, even in the commit phase, because `findDOMNode` might
293
+ // mutate it.
294
+ // TODO: Remove this line.
295
+ childToDelete.deletions = deletions;
296
}
297
298
function deleteRemainingChildren(
@@ -1125,6 +1142,7 @@ function ChildReconciler(shouldTrackSideEffects) {
1142
} else {
1143
if (
1144
child.elementType === elementType ||
1145
+ // Keep this check inline so it only runs on the false path:
1146
(__DEV__
1147
? isCompatibleFamilyForHotReloading(child, element)
1148
: false) ||
packages/react-reconciler/src/ReactFiberBeginWork.new.js
+34
-14
@@ -61,6 +61,7 @@ import {
61
Update,
62
Ref,
63
Deletion,
64
+ ChildDeletion,
65
ForceUpdateForLegacySuspense,
66
} from './ReactFiberFlags';
67
import ReactSharedInternals from 'shared/ReactSharedInternals';
@@ -2007,6 +2008,14 @@ function updateSuspensePrimaryChildren(
2008
currentFallbackChildFragment.nextEffect = null;
2009
currentFallbackChildFragment.flags = Deletion;
2010
workInProgress.firstEffect = workInProgress.lastEffect = currentFallbackChildFragment;
2011
+ let deletions = workInProgress.deletions;
2012
+ if (deletions === null) {
2013
+ deletions = workInProgress.deletions = [currentFallbackChildFragment];
2014
+ workInProgress.flags |= ChildDeletion;
2015
+ } else {
2016
+ deletions.push(currentFallbackChildFragment);
2017
+ }
2018
+ currentFallbackChildFragment.deletions = deletions;
2019
}
2020
2021
workInProgress.child = primaryChildFragment;
@@ -2061,21 +2070,23 @@ function updateSuspenseFallbackChildren(
2070
currentPrimaryChildFragment.treeBaseDuration;
2071
}
2072
2064
- // The fallback fiber was added as a deletion effect during the first pass.
2065
- // However, since we're going to remain on the fallback, we no longer want
2066
- // to delete it. So we need to remove it from the list. Deletions are stored
2067
- // on the same list as effects. We want to keep the effects from the primary
2068
- // tree. So we copy the primary child fragment's effect list, which does not
2069
- // include the fallback deletion effect.
2070
- const progressedLastEffect = primaryChildFragment.lastEffect;
2071
- if (progressedLastEffect !== null) {
2072
- workInProgress.firstEffect = primaryChildFragment.firstEffect;
2073
- workInProgress.lastEffect = progressedLastEffect;
2074
- progressedLastEffect.nextEffect = null;
2075
- } else {
2076
- // TODO: Reset this somewhere else? Lol legacy mode is so weird.
2077
- workInProgress.firstEffect = workInProgress.lastEffect = null;
2073
+ if (currentFallbackChildFragment !== null) {
2074
+ // The fallback fiber was added as a deletion effect during the first
2075
+ // pass. However, since we're going to remain on the fallback, we no
2076
+ // longer want to delete it. So we need to remove it from the list.
2077
+ // Deletions are stored on the same list as effects, and are always added
2078
+ // to the front. So we know that the first effect must be the fallback
2079
+ // deletion effect, and everything after that is from the primary free.
2080
+ const firstPrimaryTreeEffect = currentFallbackChildFragment.nextEffect;
2081
+ if (firstPrimaryTreeEffect !== null) {
2082
+ workInProgress.firstEffect = firstPrimaryTreeEffect;
2083
+ } else {
2084
+ // TODO: Reset this somewhere else? Lol legacy mode is so weird.
2085
+ workInProgress.firstEffect = workInProgress.lastEffect = null;
2086
+ }
2087
}
2088
+
2089
+ workInProgress.deletions = null;
2090
} else {
2091
primaryChildFragment = createWorkInProgressOffscreenFiber(
2092
currentPrimaryChildFragment,
@@ -2982,6 +2993,15 @@ function remountFiber(
2993
current.nextEffect = null;
2994
current.flags = Deletion;
2995
2996
+ let deletions = returnFiber.deletions;
2997
+ if (deletions === null) {
2998
+ deletions = returnFiber.deletions = [current];
2999
+ returnFiber.flags |= ChildDeletion;
3000
+ } else {
3001
+ deletions.push(current);
3002
+ }
3003
+ current.deletions = deletions;
3004
+
3005
newWorkInProgress.flags |= Placement;
3006
3007
// Restart work from the new fiber.
packages/react-reconciler/src/ReactFiberBeginWork.old.js
+34
-14
@@ -61,6 +61,7 @@ import {
61
Update,
62
Ref,
63
Deletion,
64
+ ChildDeletion,
65
ForceUpdateForLegacySuspense,
66
} from './ReactFiberFlags';
67
import ReactSharedInternals from 'shared/ReactSharedInternals';
@@ -2007,6 +2008,14 @@ function updateSuspensePrimaryChildren(
2008
currentFallbackChildFragment.nextEffect = null;
2009
currentFallbackChildFragment.flags = Deletion;
2010
workInProgress.firstEffect = workInProgress.lastEffect = currentFallbackChildFragment;
2011
+ let deletions = workInProgress.deletions;
2012
+ if (deletions === null) {
2013
+ deletions = workInProgress.deletions = [currentFallbackChildFragment];
2014
+ workInProgress.flags |= ChildDeletion;
2015
+ } else {
2016
+ deletions.push(currentFallbackChildFragment);
2017
+ }
2018
+ currentFallbackChildFragment.deletions = deletions;
2019
}
2020
2021
workInProgress.child = primaryChildFragment;
@@ -2061,21 +2070,23 @@ function updateSuspenseFallbackChildren(
2070
currentPrimaryChildFragment.treeBaseDuration;
2071
}
2072
2064
- // The fallback fiber was added as a deletion effect during the first pass.
2065
- // However, since we're going to remain on the fallback, we no longer want
2066
- // to delete it. So we need to remove it from the list. Deletions are stored
2067
- // on the same list as effects. We want to keep the effects from the primary
2068
- // tree. So we copy the primary child fragment's effect list, which does not
2069
- // include the fallback deletion effect.
2070
- const progressedLastEffect = primaryChildFragment.lastEffect;
2071
- if (progressedLastEffect !== null) {
2072
- workInProgress.firstEffect = primaryChildFragment.firstEffect;
2073
- workInProgress.lastEffect = progressedLastEffect;
2074
- progressedLastEffect.nextEffect = null;
2075
- } else {
2076
- // TODO: Reset this somewhere else? Lol legacy mode is so weird.
2077
- workInProgress.firstEffect = workInProgress.lastEffect = null;
2073
+ if (currentFallbackChildFragment !== null) {
2074
+ // The fallback fiber was added as a deletion effect during the first
2075
+ // pass. However, since we're going to remain on the fallback, we no
2076
+ // longer want to delete it. So we need to remove it from the list.
2077
+ // Deletions are stored on the same list as effects, and are always added
2078
+ // to the front. So we know that the first effect must be the fallback
2079
+ // deletion effect, and everything after that is from the primary free.
2080
+ const firstPrimaryTreeEffect = currentFallbackChildFragment.nextEffect;
2081
+ if (firstPrimaryTreeEffect !== null) {
2082
+ workInProgress.firstEffect = firstPrimaryTreeEffect;
2083
+ } else {
2084
+ // TODO: Reset this somewhere else? Lol legacy mode is so weird.
2085
+ workInProgress.firstEffect = workInProgress.lastEffect = null;
2086
+ }
2087
}
2088
+
2089
+ workInProgress.deletions = null;
2090
} else {
2091
primaryChildFragment = createWorkInProgressOffscreenFiber(
2092
currentPrimaryChildFragment,
@@ -2982,6 +2993,15 @@ function remountFiber(
2993
current.nextEffect = null;
2994
current.flags = Deletion;
2995
2996
+ let deletions = returnFiber.deletions;
2997
+ if (deletions === null) {
2998
+ deletions = returnFiber.deletions = [current];
2999
+ returnFiber.flags |= ChildDeletion;
3000
+ } else {
3001
+ deletions.push(current);
3002
+ }
3003
+ current.deletions = deletions;
3004
+
3005
newWorkInProgress.flags |= Placement;
3006
3007
// Restart work from the new fiber.
packages/react-reconciler/src/ReactFiberHooks.old.js
-3
@@ -1864,9 +1864,6 @@ const HooksDispatcherOnMount: Dispatcher = {
1864
1865
unstable_isNewReconciler: enableNewReconciler,
1866
};
1867
-if (enableCache) {
1868
- (HooksDispatcherOnMount: Dispatcher).getCacheForType = getCacheForType;
1869
-}
1867
1868
const HooksDispatcherOnUpdate: Dispatcher = {
1869
readContext,
packages/react-reconciler/src/ReactFiberHydrationContext.new.js
+10
-1
@@ -24,7 +24,7 @@ import {
24
HostRoot,
25
SuspenseComponent,
26
} from './ReactWorkTags';
27
-import {Deletion, Placement, Hydrating} from './ReactFiberFlags';
27
+import {Deletion, ChildDeletion, Placement, Hydrating} from './ReactFiberFlags';
28
import invariant from 'shared/invariant';
29
30
import {
@@ -137,6 +137,15 @@ function deleteHydratableInstance(
137
} else {
138
returnFiber.firstEffect = returnFiber.lastEffect = childToDelete;
139
}
140
+
141
+ let deletions = returnFiber.deletions;
142
+ if (deletions === null) {
143
+ deletions = returnFiber.deletions = [childToDelete];
144
+ returnFiber.flags |= ChildDeletion;
145
+ } else {
146
+ deletions.push(childToDelete);
147
+ }
148
+ childToDelete.deletions = deletions;
149
}
150
151
function insertNonHydratedInstance(returnFiber: Fiber, fiber: Fiber) {
packages/react-reconciler/src/ReactFiberHydrationContext.old.js
+10
-1
@@ -24,7 +24,7 @@ import {
24
HostRoot,
25
SuspenseComponent,
26
} from './ReactWorkTags';
27
-import {Deletion, Placement, Hydrating} from './ReactFiberFlags';
27
+import {Deletion, ChildDeletion, Placement, Hydrating} from './ReactFiberFlags';
28
import invariant from 'shared/invariant';
29
30
import {
@@ -137,6 +137,15 @@ function deleteHydratableInstance(
137
} else {
138
returnFiber.firstEffect = returnFiber.lastEffect = childToDelete;
139
}
140
+
141
+ let deletions = returnFiber.deletions;
142
+ if (deletions === null) {
143
+ deletions = returnFiber.deletions = [childToDelete];
144
+ returnFiber.flags |= ChildDeletion;
145
+ } else {
146
+ deletions.push(childToDelete);
147
+ }
148
+ childToDelete.deletions = deletions;
149
}
150
151
function insertNonHydratedInstance(returnFiber: Fiber, fiber: Fiber) {
packages/react-reconciler/src/ReactFiberWorkLoop.new.js
+31
-2
@@ -1821,6 +1821,7 @@ function completeUnitOfWork(unitOfWork: Fiber): void {
1821
// Mark the parent fiber as incomplete and clear its effect list.
1822
returnFiber.firstEffect = returnFiber.lastEffect = null;
1823
returnFiber.flags |= Incomplete;
1824
+ returnFiber.deletions = null;
1825
}
1826
}
1827
@@ -2384,7 +2385,7 @@ function commitMutationEffects(root: FiberRoot, renderPriorityLevel) {
2385
// bitmap value, we remove the secondary effects from the effect tag and
2386
// switch on that value.
2387
const primaryFlags = flags & (Placement | Update | Deletion | Hydrating);
2387
- switch (primaryFlags) {
2388
+ outer: switch (primaryFlags) {
2389
case Placement: {
2390
commitPlacement(nextEffect);
2391
// Clear the "placement" from effect tag so that we know that this is
@@ -2424,7 +2425,35 @@ function commitMutationEffects(root: FiberRoot, renderPriorityLevel) {
2425
break;
2426
}
2427
case Deletion: {
2427
- commitDeletion(root, nextEffect, renderPriorityLevel);
2428
+ // Reached a deletion effect. Instead of commit this effect like we
2429
+ // normally do, we're going to use the `deletions` array of the parent.
2430
+ // However, because the effect list is sorted in depth-first order, we
2431
+ // can't wait until we reach the parent node, because the child effects
2432
+ // will have run in the meantime.
2433
+ //
2434
+ // So instead, we use a trick where the first time we hit a deletion
2435
+ // effect, we commit all the deletion effects that belong to that parent.
2436
+ //
2437
+ // This is an incremental step away from using the effect list and
2438
+ // toward a DFS + subtreeFlags traversal.
2439
+ //
2440
+ // A reference to the deletion array of the parent is also stored on
2441
+ // each of the deletions. This is really weird. It would be better to
2442
+ // follow the `.return` pointer, but unfortunately we can't assume that
2443
+ // `.return` points to the correct fiber, even in the commit phase,
2444
+ // because `findDOMNode` might mutate it.
2445
+ const deletedChild = nextEffect;
2446
+ const deletions = deletedChild.deletions;
2447
+ if (deletions !== null) {
2448
+ for (let i = 0; i < deletions.length; i++) {
2449
+ const deletion = deletions[i];
2450
+ // Clear the deletion effect so that we don't delete this node more
2451
+ // than once.
2452
+ deletion.flags &= ~Deletion;
2453
+ deletion.deletions = null;
2454
+ commitDeletion(root, deletion, renderPriorityLevel);
2455
+ }
2456
+ }
2457
break;
2458
}
2459
}
packages/react-reconciler/src/ReactFiberWorkLoop.old.js
+47
-2
@@ -1821,6 +1821,7 @@ function completeUnitOfWork(unitOfWork: Fiber): void {
1821
// Mark the parent fiber as incomplete and clear its effect list.
1822
returnFiber.firstEffect = returnFiber.lastEffect = null;
1823
returnFiber.flags |= Incomplete;
1824
+ returnFiber.deletions = null;
1825
}
1826
}
1827
@@ -2384,7 +2385,7 @@ function commitMutationEffects(root: FiberRoot, renderPriorityLevel) {
2385
// bitmap value, we remove the secondary effects from the effect tag and
2386
// switch on that value.
2387
const primaryFlags = flags & (Placement | Update | Deletion | Hydrating);
2387
- switch (primaryFlags) {
2388
+ outer: switch (primaryFlags) {
2389
case Placement: {
2390
commitPlacement(nextEffect);
2391
// Clear the "placement" from effect tag so that we know that this is
@@ -2424,7 +2425,35 @@ function commitMutationEffects(root: FiberRoot, renderPriorityLevel) {
2425
break;
2426
}
2427
case Deletion: {
2427
- commitDeletion(root, nextEffect, renderPriorityLevel);
2428
+ // Reached a deletion effect. Instead of commit this effect like we
2429
+ // normally do, we're going to use the `deletions` array of the parent.
2430
+ // However, because the effect list is sorted in depth-first order, we
2431
+ // can't wait until we reach the parent node, because the child effects
2432
+ // will have run in the meantime.
2433
+ //
2434
+ // So instead, we use a trick where the first time we hit a deletion
2435
+ // effect, we commit all the deletion effects that belong to that parent.
2436
+ //
2437
+ // This is an incremental step away from using the effect list and
2438
+ // toward a DFS + subtreeFlags traversal.
2439
+ //
2440
+ // A reference to the deletion array of the parent is also stored on
2441
+ // each of the deletions. This is really weird. It would be better to
2442
+ // follow the `.return` pointer, but unfortunately we can't assume that
2443
+ // `.return` points to the correct fiber, even in the commit phase,
2444
+ // because `findDOMNode` might mutate it.
2445
+ const deletedChild = nextEffect;
2446
+ const deletions = deletedChild.deletions;
2447
+ if (deletions !== null) {
2448
+ for (let i = 0; i < deletions.length; i++) {
2449
+ const deletion = deletions[i];
2450
+ // Clear the deletion effect so that we don't delete this node more
2451
+ // than once.
2452
+ deletion.flags &= ~Deletion;
2453
+ deletion.deletions = null;
2454
+ commitDeletion(root, deletion, renderPriorityLevel);
2455
+ }
2456
+ }
2457
break;
2458
}
2459
}
@@ -2844,6 +2873,22 @@ export function captureCommitPhaseError(sourceFiber: Fiber, error: mixed) {
2873
}
2874
fiber = fiber.return;
2875
}
2876
+
2877
+ if (__DEV__) {
2878
+ // TODO: Until we re-land skipUnmountedBoundaries (see #20147), this warning
2879
+ // will fire for errors that are thrown by destroy functions inside deleted
2880
+ // trees. What it should instead do is propagate the error to the parent of
2881
+ // the deleted tree. In the meantime, do not add this warning to the
2882
+ // allowlist; this is only for our internal use.
2883
+ console.error(
2884
+ 'Internal React error: Attempted to capture a commit phase error ' +
2885
+ 'inside a detached tree. This indicates a bug in React. Likely ' +
2886
+ 'causes include deleting the same fiber more than once, committing an ' +
2887
+ 'already-finished tree, or an inconsistent return pointer.\n\n' +
2888
+ 'Error message:\n\n%s',
2889
+ error,
2890
+ );
2891
+ }
2892
}
2893
2894
export function pingSuspendedRoot(