Address feedback

facebook · May 21, 2020 · eb3dc35 · eb3dc35
1 parent f3396bf
commit eb3dc35
Show file tree

Hide file tree

Showing 4 changed files with 22 additions and 12 deletions.
diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js
@@ -1124,15 +1124,16 @@ function commitNestedUnmounts(
   }
 }
 
-function detachFiber(fiber: Fiber) {
+function detachFiberMutation(fiber: Fiber) {
   // Cut off the return pointers to disconnect it from the tree. Ideally, we
   // should clear the child pointer of the parent alternate to let this
   // get GC:ed but we don't know which for sure which parent is the current
   // one so we'll settle for GC:ing the subtree of this child. This child
   // itself will be GC:ed when the parent updates the next time.
   // Note: we cannot null out sibling here, otherwise it can cause issues
   // with findDOMNode and how it requires the sibling field to carry out
-  // traversal in a later effect. See PR #16820.
+  // traversal in a later effect. See PR #16820. We now clear the sibling
+  // field after effects, see: detachFiberAfterEffects.
   fiber.alternate = null;
   fiber.child = null;
   fiber.dependencies_new = null;
@@ -1543,9 +1544,9 @@ function commitDeletion(
     commitNestedUnmounts(finishedRoot, current, renderPriorityLevel);
   }
   const alternate = current.alternate;
-  detachFiber(current);
+  detachFiberMutation(current);
   if (alternate !== null) {
-    detachFiber(alternate);
+    detachFiberMutation(alternate);
   }
 }
 

diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.old.js b/packages/react-reconciler/src/ReactFiberCommitWork.old.js
@@ -1122,15 +1122,16 @@ function commitNestedUnmounts(
   }
 }
 
-function detachFiber(fiber: Fiber) {
+function detachFiberMutation(fiber: Fiber) {
   // Cut off the return pointers to disconnect it from the tree. Ideally, we
   // should clear the child pointer of the parent alternate to let this
   // get GC:ed but we don't know which for sure which parent is the current
   // one so we'll settle for GC:ing the subtree of this child. This child
   // itself will be GC:ed when the parent updates the next time.
   // Note: we cannot null out sibling here, otherwise it can cause issues
   // with findDOMNode and how it requires the sibling field to carry out
-  // traversal in a later effect. See PR #16820.
+  // traversal in a later effect. See PR #16820. We now clear the sibling
+  // field after effects, see: detachFiberAfterEffects.
   fiber.alternate = null;
   fiber.child = null;
   fiber.dependencies_old = null;
@@ -1541,9 +1542,9 @@ function commitDeletion(
     commitNestedUnmounts(finishedRoot, current, renderPriorityLevel);
   }
   const alternate = current.alternate;
-  detachFiber(current);
+  detachFiberMutation(current);
   if (alternate !== null) {
-    detachFiber(alternate);
+    detachFiberMutation(alternate);
   }
 }
 

diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js
@@ -1994,7 +1994,7 @@ function commitRootImpl(root, renderPriorityLevel) {
       const nextNextEffect = nextEffect.nextEffect;
       nextEffect.nextEffect = null;
       if (nextEffect.effectTag & Deletion) {
-        nextEffect.sibling = null;
+        detachFiberAfterEffects(nextEffect);
       }
       nextEffect = nextNextEffect;
     }
@@ -2451,7 +2451,7 @@ function flushPassiveEffectsImpl() {
     // Remove nextEffect pointer to assist GC
     effect.nextEffect = null;
     if (effect.effectTag & Deletion) {
-      effect.sibling = null;
+      detachFiberAfterEffects(effect);
     }
     effect = nextNextEffect;
   }
@@ -3555,3 +3555,7 @@ export function act(callback: () => Thenable<mixed>): Thenable<void> {
     };
   }
 }
+
+function detachFiberAfterEffects(fiber: Fiber): void {
+  fiber.sibling = null;
+}
diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js
@@ -2102,7 +2102,7 @@ function commitRootImpl(root, renderPriorityLevel) {
       const nextNextEffect = nextEffect.nextEffect;
       nextEffect.nextEffect = null;
       if (nextEffect.effectTag & Deletion) {
-        nextEffect.sibling = null;
+        detachFiberAfterEffects(nextEffect);
       }
       nextEffect = nextNextEffect;
     }
@@ -2599,7 +2599,7 @@ function flushPassiveEffectsImpl() {
     // Remove nextEffect pointer to assist GC
     effect.nextEffect = null;
     if (effect.effectTag & Deletion) {
-      effect.sibling = null;
+      detachFiberAfterEffects(effect);
     }
     effect = nextNextEffect;
   }
@@ -3718,3 +3718,7 @@ export function act(callback: () => Thenable<mixed>): Thenable<void> {
     };
   }
 }
+
+function detachFiberAfterEffects(fiber: Fiber): void {
+  fiber.sibling = null;
+}