Append deletions to the effect list

First clear any progressed deletions for any case where we start
over with the "current" set of children.

Once we've performed a new reconciliation we need to add the
deletions to the side-effect list (which we know is empty because
we just emptied it).

For other effects, instead of just adding a fiber to an effect
list we need to mark it with an update. Then after completion
we add it to the the effect list if it had any effects at all.

This means that we lose the opportunity to control if a fiber
gets added before or after its children but that was already
flawed since we want certain side-effects to happen before others
on a global level.

Instead, we'll do multiple passes through the effect list.

Author: sebmarkbage

src/renderers/shared/fiber/ReactChildFiber.js

@@ -58,7 +58,6 @@ const {
 } = ReactPriorityLevel;
 
 const {
-  NoEffect,
   Placement,
   Deletion,
 } = ReactTypeOfSideEffect;
@@ -582,10 +581,6 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {
       existingChildren.forEach(child => deleteChild(returnFiber, child));
     }
 
-    // TODO: Add deletions and insert/moves to the side-effect list.
-    // TODO: Clear the deletion list when we don't reconcile in place. When
-    // progressedChild isn't reused.
-
     return resultingFirstChild;
   }
 

src/renderers/shared/fiber/ReactFiberBeginWork.js

@@ -66,6 +66,18 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>, g
     }
   }
 
+  function clearDeletions(workInProgress) {
+    workInProgress.progressedFirstDeletion =
+      workInProgress.progressedLastDeletion =
+        null;
+  }
+
+  function transferDeletions(workInProgress) {
+    // Any deletions get added first into the effect list.
+    workInProgress.firstEffect = workInProgress.progressedFirstDeletion;
+    workInProgress.lastEffect = workInProgress.progressedLastDeletion;
+  }
+
   function reconcileChildren(current, workInProgress, nextChildren) {
     const priorityLevel = workInProgress.pendingWorkPriority;
     reconcileChildrenAtPriority(current, workInProgress, nextChildren, priorityLevel);
@@ -90,23 +102,31 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>, g
       // If the current child is the same as the work in progress, it means that
       // we haven't yet started any work on these children. Therefore, we use
       // the clone algorithm to create a copy of all the current children.
+
+      // If we had any progressed work already, that is invalid at this point so
+      // let's throw it out.
+      clearDeletions(workInProgress);
+
       workInProgress.child = reconcileChildFibers(
         workInProgress,
         workInProgress.child,
         nextChildren,
         priorityLevel
       );
+
+      transferDeletions(workInProgress);
     } else {
-      // If, on the other hand, we don't have a current fiber or if it is
-      // already using a clone, that means we've already begun some work on this
-      // tree and we can continue where we left off by reconciling against the
-      // existing children.
+      // If, on the other hand, it is already using a clone, that means we've
+      // already begun some work on this tree and we can continue where we left
+      // off by reconciling against the existing children.
       workInProgress.child = reconcileChildFibersInPlace(
         workInProgress,
         workInProgress.child,
         nextChildren,
         priorityLevel
       );
+
+      transferDeletions(workInProgress);
     }
     markChildAsProgressed(current, workInProgress, priorityLevel);
   }
@@ -353,6 +373,12 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>, g
     //   return null;
     // }
 
+    if (current && workInProgress.child === current.child) {
+      // If we had any progressed work already, that is invalid at this point so
+      // let's throw it out.
+      clearDeletions(workInProgress);
+    }
+
     cloneChildFibers(current, workInProgress);
     markChildAsProgressed(current, workInProgress, priorityLevel);
     return workInProgress.child;

src/renderers/shared/fiber/ReactFiberCompleteWork.js

@@ -19,6 +19,7 @@ import type { ReifiedYield } from 'ReactReifiedYield';
 
 var { reconcileChildFibers } = require('ReactChildFiber');
 var ReactTypeOfWork = require('ReactTypeOfWork');
+var ReactTypeOfSideEffect = require('ReactTypeOfSideEffect');
 var {
   IndeterminateComponent,
   FunctionalComponent,
@@ -31,35 +32,20 @@ var {
   YieldComponent,
   Fragment,
 } = ReactTypeOfWork;
+var {
+  Update,
+} = ReactTypeOfSideEffect;
 
 module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>) {
 
   const createInstance = config.createInstance;
   const createTextInstance = config.createTextInstance;
   const prepareUpdate = config.prepareUpdate;
 
-  function markForPreEffect(workInProgress : Fiber) {
-    // Schedule a side-effect on this fiber, BEFORE the children's side-effects.
-    if (workInProgress.firstEffect) {
-      workInProgress.nextEffect = workInProgress.firstEffect;
-      workInProgress.firstEffect = workInProgress;
-    } else {
-      workInProgress.firstEffect = workInProgress;
-      workInProgress.lastEffect = workInProgress;
-    }
-  }
-
-  // TODO: It's possible this will create layout thrash issues because mutations
-  // of the DOM and life-cycles are interleaved. E.g. if a componentDidMount
-  // of a sibling reads, then the next sibling updates and reads etc.
-  function markForPostEffect(workInProgress : Fiber) {
-    // Schedule a side-effect on this fiber, AFTER the children's side-effects.
-    if (workInProgress.lastEffect) {
-      workInProgress.lastEffect.nextEffect = workInProgress;
-    } else {
-      workInProgress.firstEffect = workInProgress;
-    }
-    workInProgress.lastEffect = workInProgress;
+  function markUpdate(workInProgress : Fiber) {
+    // Tag the fiber with an update effect. This turns a Placement into
+    // an UpdateAndPlacement.
+    workInProgress.effectTag |= Update;
   }
 
   function transferOutput(child : ?Fiber, returnFiber : Fiber) {
@@ -143,7 +129,7 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>) {
         // Transfer update queue to callbackList field so callbacks can be
         // called during commit phase.
         workInProgress.callbackList = workInProgress.updateQueue;
-        markForPostEffect(workInProgress);
+        markUpdate(workInProgress);
         return null;
       case HostContainer:
         transferOutput(workInProgress.child, workInProgress);
@@ -152,7 +138,7 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>) {
         // all the other side-effects in the subtree. We need to schedule it
         // before so that the entire tree is up-to-date before the life-cycles
         // are invoked.
-        markForPreEffect(workInProgress);
+        markUpdate(workInProgress);
         return null;
       case HostComponent:
         let newProps = workInProgress.pendingProps;
@@ -172,7 +158,7 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>) {
           const instance : I = workInProgress.stateNode;
           if (prepareUpdate(instance, oldProps, newProps, children)) {
             // This returns true if there was something to update.
-            markForPreEffect(workInProgress);
+            markUpdate(workInProgress);
           }
           // TODO: Is this actually ever going to change? Why set it every time?
           workInProgress.output = instance;
@@ -197,7 +183,7 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>) {
         if (current && workInProgress.stateNode != null) {
           // If we have an alternate, that means this is an update and we need to
           // schedule a side-effect to do the updates.
-          markForPreEffect(workInProgress);
+          markUpdate(workInProgress);
         } else {
           if (typeof newText !== 'string') {
             if (workInProgress.stateNode === null) {

src/renderers/shared/fiber/ReactFiberScheduler.js

@@ -30,6 +30,12 @@ var {
   SynchronousPriority,
 } = require('ReactPriorityLevel');
 
+var {
+  NoEffect,
+  Update,
+  PlacementAndUpdate,
+} = require('ReactTypeOfSideEffect');
+
 var timeHeuristicForUnitOfWork = 1;
 
 export type Scheduler = {
@@ -106,7 +112,10 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>) {
     let effectfulFiber = finishedWork.firstEffect;
     while (effectfulFiber) {
       const current = effectfulFiber.alternate;
-      commitWork(current, effectfulFiber);
+      if (effectfulFiber.effectTag === Update ||
+          effectfulFiber.effectTag === PlacementAndUpdate) {
+        commitWork(current, effectfulFiber);
+      }
       const next = effectfulFiber.nextEffect;
       // Ensure that we clean these up so that we don't accidentally keep them.
       // I'm not actually sure this matters because we can't reset firstEffect
@@ -151,12 +160,26 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>) {
       workInProgress.pendingProps = null;
       workInProgress.updateQueue = null;
 
+      // If this fiber had side-effects, we append it to the end of its own
+      // effect list.
+      if (workInProgress.effectTag !== NoEffect) {
+        // Schedule a side-effect on this fiber, AFTER the children's
+        // side-effects. We can perform certain side-effects earlier if
+        // needed, by doing multiple passes over the effect list.
+        if (workInProgress.lastEffect) {
+          workInProgress.lastEffect.nextEffect = workInProgress;
+        } else {
+          workInProgress.firstEffect = workInProgress;
+        }
+        workInProgress.lastEffect = workInProgress;
+      }
+
       const returnFiber = workInProgress.return;
 
       if (returnFiber) {
-        // Ensure that the first and last effect of the parent corresponds
-        // to the children's first and last effect. This probably relies on
-        // children completing in order.
+        // Append all the effects of the subtree and this fiber onto the effect
+        // list of the parent. The completion order of the children affects the
+        // side-effect order.
         if (!returnFiber.firstEffect) {
           returnFiber.firstEffect = workInProgress.firstEffect;
         }