Improved deletions approach

Process deletions as we traverse the tree during commit, before we process other effects. This has the result of better mimicking the previous sequencing.
facebook · Jul 7, 2020 · 1033552 · 1033552
1 parent 5cdbbba
commit 1033552
Show file tree

Hide file tree

Showing 2 changed files with 13 additions and 43 deletions.
diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js
@@ -2125,6 +2125,8 @@ function commitRootImpl(root, renderPriorityLevel) {
 }
 
 function commitBeforeMutationEffects(fiber: Fiber) {
+  commitBeforeMutationEffectsDeletions(fiber.deletions);
+
   if (fiber.child !== null) {
     const primarySubtreeTag =
       fiber.subtreeTag & (Deletion | Snapshot | Passive | Placement);
@@ -2159,15 +2161,6 @@ function commitBeforeMutationEffectsImpl(fiber: Fiber) {
   const effectTag = fiber.effectTag;
 
   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);
-    }
-
     // 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 (
@@ -2220,6 +2213,8 @@ function commitMutationEffects(
   root: FiberRoot,
   renderPriorityLevel,
 ) {
+  commitMutationEffectsDeletions(fiber.deletions, root, renderPriorityLevel);
+
   if (fiber.child !== null) {
     const primarySubtreeTag =
       fiber.subtreeTag &
@@ -2262,15 +2257,6 @@ function commitMutationEffectsImpl(
   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);

diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js
@@ -1732,31 +1732,15 @@ describe('ReactSuspenseWithNoopRenderer', () => {
     await resolveText('B2');
 
     expect(Scheduler).toHaveYielded(['Promise resolved [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]',
-          ]),
-    );
+    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 () => {