Removed the temporary isDeletion hack

Author: Brian Vaughn

packages/react-reconciler/src/ReactFiberCommitWork.new.js

@@ -68,6 +68,7 @@ import {
   Placement,
   Snapshot,
   Update,
+  Passive,
   PassiveUnmountPendingDev,
 } from './ReactSideEffectTags';
 import getComponentName from 'shared/getComponentName';
@@ -879,6 +880,29 @@ function commitUnmount(
             if (destroy !== undefined) {
               if ((tag & HookPassive) !== NoHookEffect) {
                 effect.tag |= HookHasEffect;
+
+                // subtreeTags currently bubble in resetChildLanes which doens't get called for unmounted subtrees.
+                // So we need to bubble this up manually to the root (or to the nearest ancestor that already has it).
+                let ancestor = current.return;
+                while (ancestor !== null) {
+                  if (
+                    (ancestor.subtreeTag & PassiveSubtreeTag) !==
+                    NoSubtreeTag
+                  ) {
+                    break;
+                  }
+
+                  ancestor.subtreeTag |= PassiveSubtreeTag;
+                  const alternate = ancestor.alternate;
+                  if (alternate !== null) {
+                    alternate.subtreeTag |= PassiveSubtreeTag;
+                  }
+
+                  ancestor = ancestor.return;
+                }
+
+                current.effectTag |= Passive;
+
                 if (__DEV__) {
                   // This flag is used to avoid warning about an update to an unmounted component
                   // if the component has a passive unmount scheduled.
@@ -889,6 +913,7 @@ function commitUnmount(
                     alternate.effectTag |= PassiveUnmountPendingDev;
                   }
                 }
+
                 schedulePassiveEffectCallback();
               } else {
                 if (

packages/react-reconciler/src/ReactFiberWorkLoop.new.js

@@ -132,7 +132,6 @@ import {
   Snapshot,
   Callback,
   Passive,
-  // TODO (effects) Maybe get rid of this in favor of new subtreeTag?
   PassiveUnmountPendingDev,
   Incomplete,
   HostEffectMask,
@@ -2181,8 +2180,6 @@ function commitRootImpl(root, renderPriorityLevel) {
           return null;
         });
       }
-    } else {
-      // TODO (effects) Clear deletion arrays
     }
 
     // Tell Scheduler to yield at the end of the frame, so the browser has an
@@ -2216,8 +2213,6 @@ function commitRootImpl(root, renderPriorityLevel) {
     rootWithPendingPassiveEffects = root;
     pendingPassiveEffectsLanes = lanes;
     pendingPassiveEffectsRenderPriority = renderPriorityLevel;
-  } else {
-    // TODO (effects) Detach sibling pointers for deleted Fibers
   }
 
   // Read this again, since an effect might have updated it
@@ -2418,7 +2413,8 @@ function commitMutationEffects(
         renderPriorityLevel,
       );
 
-      // TODO (effects) Clear deletions array if there are no pending passive effects.
+      // TODO (effects) Detach sibling pointers for deleted Fibers
+      // TODO (effects) Clear deletion arrays
     }
 
     if (fiber.child !== null) {
@@ -2773,53 +2769,49 @@ function flushPassiveMountEffectsImpl(fiber: Fiber): void {
   }
 }
 
-function flushPassiveUnmountEffects(
-  firstChild: Fiber,
-  isDeletion: boolean,
-): void {
+function flushPassiveUnmountEffects(firstChild: Fiber): void {
   let fiber = firstChild;
   while (fiber !== null) {
     const deletions = fiber.deletions;
     if (deletions !== null) {
       for (let i = 0; i < deletions.length; i++) {
         const fiberToDelete = deletions[i];
-        // TODO (effects) This probably won't work because effectsTag and subtreeTag won't be set
-        // correctly for nested Fibers; we need to traverse the whole tree then which sucks.
-        // We should update commitUnmount() to handle this when we call enqueuePassive blah.
-        // TODO (effects) This is probably a change in deletion ordering (recursing subtrees this way)
-        // but that presumably doesn't matter so long as we process all deletions before creations?
-        flushPassiveUnmountEffects(fiberToDelete, true);
+        // If this fiber (or anything below it) has passive effects then traverse the subtree.
+        const primaryEffectTag = fiberToDelete.effectTag & (Passive | Update);
+        const primarySubtreeTag = fiberToDelete.subtreeTag & PassiveSubtreeTag;
+        if (
+          primarySubtreeTag !== NoSubtreeTag ||
+          primaryEffectTag !== NoEffect
+        ) {
+          flushPassiveUnmountEffects(fiberToDelete);
+        }
       }
     }
 
     const didBailout =
       fiber.alternate !== null && fiber.alternate.child === fiber.child;
-    if (fiber.child !== null && !didBailout) {
-      // TODO (effects) See above
-      //const primarySubtreeTag = fiber.subtreeTag & Deletion;
-      //if (primarySubtreeTag !== NoEffect) {
-      flushPassiveUnmountEffects(fiber.child, isDeletion);
-      //}
-    }
-    // TODO (effects) Are we traversing extra here? Maybe we won't need this check
-    // here once we stop over traversing.
-    const primarySubtreeTag =
-      fiber.return === null ||
-      (fiber.return.subtreeTag & PassiveSubtreeTag) !== NoSubtreeTag;
-    if (
-      isDeletion ||
-      ((fiber.effectTag & Update) !== NoEffect && primarySubtreeTag)
-    ) {
-      switch (fiber.tag) {
-        case FunctionComponent:
-        case ForwardRef:
-        case SimpleMemoComponent:
-        case Block: {
-          // TODO (effects) See above
-          //const primaryEffectTag = fiber.effectTag & Passive;
-          //if (primaryEffectTag !== NoEffect) {
+    if (!didBailout) {
+      const child = fiber.child;
+      if (child !== null) {
+        // If any children have passive effects then traverse the subtree.
+        // Note that this requires checking subtreeTag of the current Fiber,
+        // rather than the subtreeTag/effectsTag of the first child,
+        // since that would not cover passive effects in siblings.
+        const primarySubtreeTag = fiber.subtreeTag & PassiveSubtreeTag;
+        if (primarySubtreeTag !== NoSubtreeTag) {
+          flushPassiveUnmountEffects(child);
+        }
+      }
+    }
+
+    switch (fiber.tag) {
+      case FunctionComponent:
+      case ForwardRef:
+      case SimpleMemoComponent:
+      case Block: {
+        const primaryEffectTag = fiber.effectTag & (Passive | Update);
+        if (primaryEffectTag !== NoEffect) {
           flushPassiveUnmountEffectsImpl(fiber);
-          //}
         }
       }
     }
@@ -2939,7 +2931,7 @@ function flushPassiveEffectsImpl() {
   // e.g. a destroy function in one component may unintentionally override a ref
   // value set by a create function in another component.
   // Layout effects have the same constraint.
-  flushPassiveUnmountEffects(root.current, false);
+  flushPassiveUnmountEffects(root.current);
   flushPassiveMountEffects(root.current);
 
   // TODO (effects) Detach sibling pointers for deleted Fibers