[Fiber] Remove output field (#8406)

* Remove output field

The idea was originally that each fiber has a return value. In practice
most of what we're modelling here are void functions and we track side
effects instead of return values.

We do use this for coroutines to short cut access to terminal yields.
However, since this can be nested fragments we end up searching the tree
anyway. We also have to manage this in transferOutput so it ends up being
as expensive. Maybe we can save some traversal for updates when coroutine
branches bail out but meh.

* Unmount child from the first phase of a coroutine

Author: sebmarkbage

scripts/fiber/tests-passing.txt

@@ -1031,6 +1031,7 @@ src/renderers/shared/__tests__/ReactPerf-test.js
 
 src/renderers/shared/fiber/__tests__/ReactCoroutine-test.js
 * should render a coroutine
+* should unmount a composite in a coroutine
 
 src/renderers/shared/fiber/__tests__/ReactIncremental-test.js
 * should render a simple component

src/renderers/shared/fiber/ReactChildFiber.js

@@ -294,14 +294,14 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {
       // Insert
       const reifiedYield = createReifiedYield(yieldNode);
       const created = createFiberFromYield(yieldNode, priority);
-      created.output = reifiedYield;
+      created.type = reifiedYield;
       created.return = returnFiber;
       return created;
     } else {
       // Move based on index
       const existing = useFiber(current, priority);
-      existing.output = createUpdatedReifiedYield(
-        current.output,
+      existing.type = createUpdatedReifiedYield(
+        current.type,
         yieldNode
       );
       existing.return = returnFiber;
@@ -386,7 +386,7 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {
         case REACT_YIELD_TYPE: {
           const reifiedYield = createReifiedYield(newChild);
           const created = createFiberFromYield(newChild, priority);
-          created.output = reifiedYield;
+          created.type = reifiedYield;
           created.return = returnFiber;
           return created;
         }
@@ -790,8 +790,8 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {
         if (child.tag === YieldComponent) {
           deleteRemainingChildren(returnFiber, child.sibling);
           const existing = useFiber(child, priority);
-          existing.output = createUpdatedReifiedYield(
-            child.output,
+          existing.type = createUpdatedReifiedYield(
+            child.type,
             yieldNode
           );
           existing.return = returnFiber;
@@ -808,7 +808,7 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {
 
     const reifiedYield = createReifiedYield(yieldNode);
     const created = createFiberFromYield(yieldNode, priority);
-    created.output = reifiedYield;
+    created.type = reifiedYield;
     created.return = returnFiber;
     return created;
   }

src/renderers/shared/fiber/ReactFiber.js

@@ -97,9 +97,6 @@ export type Fiber = {
   memoizedState: any,
   // Linked list of callbacks to call after updates are committed.
   callbackList: ?UpdateQueue,
-  // Output is the return value of this fiber, or a linked list of return values
-  // if this returns multiple values. Such as a fragment.
-  output: any, // This type will be more specific once we overload the tag.
 
   // Effect
   effectTag: TypeOfSideEffect,
@@ -190,7 +187,6 @@ var createFiber = function(tag : TypeOfWork, key : null | string) : Fiber {
     updateQueue: null,
     memoizedState: null,
     callbackList: null,
-    output: null,
 
     effectTag: NoEffect,
     nextEffect: null,
@@ -267,7 +263,6 @@ exports.cloneFiber = function(fiber : Fiber, priorityLevel : PriorityLevel) : Fi
 
   alt.memoizedProps = fiber.memoizedProps;
   alt.memoizedState = fiber.memoizedState;
-  alt.output = fiber.output;
 
   return alt;
 };

src/renderers/shared/fiber/ReactFiberCommitWork.js

@@ -21,6 +21,7 @@ var {
   HostContainer,
   HostComponent,
   HostText,
+  CoroutineComponent,
   Portal,
 } = ReactTypeOfWork;
 var { callCallbacks } = require('ReactFiberUpdateQueue');
@@ -278,6 +279,10 @@ module.exports = function<T, P, I, TI, C>(
         detachRef(current);
         return;
       }
+      case CoroutineComponent: {
+        commitNestedUnmounts(current.stateNode);
+        return;
+      }
     }
   }
 

src/renderers/shared/fiber/ReactFiberCompleteWork.js

@@ -62,28 +62,31 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>) {
     workInProgress.effectTag |= Callback;
   }
 
-  function transferOutput(child : ?Fiber, returnFiber : Fiber) {
-    // If we have a single result, we just pass that through as the output to
-    // avoid unnecessary traversal. When we have multiple output, we just pass
-    // the linked list of fibers that has the individual output values.
-    returnFiber.output = (child && !child.sibling) ? child.output : child;
-    returnFiber.memoizedProps = returnFiber.pendingProps;
-  }
-
-  function recursivelyFillYields(yields, output : ?Fiber | ?ReifiedYield) {
-    if (!output) {
-      // Ignore nulls etc.
-    } else if (output.tag !== undefined) { // TODO: Fix this fragile duck test.
-      // Detect if this is a fiber, if so it is a fragment result.
-      // $FlowFixMe: Refinement issue.
-      var item = (output : Fiber);
-      do {
-        recursivelyFillYields(yields, item.output);
-        item = item.sibling;
-      } while (item);
-    } else {
-      // $FlowFixMe: Refinement issue. If it is not a Fiber or null, it is a yield
-      yields.push(output);
+  function appendAllYields(yields : Array<ReifiedYield>, workInProgress : Fiber) {
+    let node = workInProgress.child;
+    while (node) {
+      if (node.tag === HostComponent || node.tag === HostText ||
+          node.tag === Portal) {
+        throw new Error('A coroutine cannot have host component children.');
+      } else if (node.tag === YieldComponent) {
+        yields.push(node.type);
+      } else if (node.child) {
+        // TODO: Coroutines need to visit the stateNode.
+        node.child.return = node;
+        node = node.child;
+        continue;
+      }
+      if (node === workInProgress) {
+        return;
+      }
+      while (!node.sibling) {
+        if (!node.return || node.return === workInProgress) {
+          return;
+        }
+        node = node.return;
+      }
+      node.sibling.return = node.return;
+      node = node.sibling;
     }
   }
 
@@ -105,11 +108,7 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>) {
     // Build up the yields.
     // TODO: Compare this to a generator or opaque helpers like Children.
     var yields : Array<ReifiedYield> = [];
-    var child = workInProgress.child;
-    while (child) {
-      recursivelyFillYields(yields, child.output);
-      child = child.sibling;
-    }
+    appendAllYields(yields, workInProgress);
     var fn = coroutine.handler;
     var props = coroutine.props;
     var nextChildren = fn(props, yields);
@@ -158,10 +157,9 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>) {
   function completeWork(current : ?Fiber, workInProgress : Fiber) : ?Fiber {
     switch (workInProgress.tag) {
       case FunctionalComponent:
-        transferOutput(workInProgress.child, workInProgress);
+        workInProgress.memoizedProps = workInProgress.pendingProps;
         return null;
       case ClassComponent:
-        transferOutput(workInProgress.child, workInProgress);
         // We are leaving this subtree, so pop context if any.
         if (isContextProvider(workInProgress)) {
           popContextProvider();
@@ -191,7 +189,7 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>) {
         }
         return null;
       case HostContainer: {
-        transferOutput(workInProgress.child, workInProgress);
+        workInProgress.memoizedProps = workInProgress.pendingProps;
         popContextProvider();
         const fiberRoot = (workInProgress.stateNode : FiberRoot);
         if (fiberRoot.pendingContext) {
@@ -221,8 +219,6 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>) {
             // This returns true if there was something to update.
             markUpdate(workInProgress);
           }
-          // TODO: Is this actually ever going to change? Why set it every time?
-          workInProgress.output = instance;
         } else {
           if (!newProps) {
             if (workInProgress.stateNode === null) {
@@ -242,9 +238,7 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>) {
           appendAllChildren(instance, workInProgress);
           finalizeInitialChildren(instance, workInProgress.type, newProps);
 
-          // TODO: This seems like unnecessary duplication.
           workInProgress.stateNode = instance;
-          workInProgress.output = instance;
           if (workInProgress.ref) {
             // If there is a ref on a host node we need to schedule a callback
             markUpdate(workInProgress);
@@ -268,29 +262,26 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>) {
             }
           }
           const textInstance = createTextInstance(newText, workInProgress);
-          // TODO: This seems like unnecessary duplication.
           workInProgress.stateNode = textInstance;
-          workInProgress.output = textInstance;
         }
         workInProgress.memoizedProps = newText;
         return null;
       case CoroutineComponent:
         return moveCoroutineToHandlerPhase(current, workInProgress);
       case CoroutineHandlerPhase:
-        transferOutput(workInProgress.stateNode, workInProgress);
+        workInProgress.memoizedProps = workInProgress.pendingProps;
         // Reset the tag to now be a first phase coroutine.
         workInProgress.tag = CoroutineComponent;
         return null;
       case YieldComponent:
         // Does nothing.
         return null;
       case Fragment:
-        transferOutput(workInProgress.child, workInProgress);
+        workInProgress.memoizedProps = workInProgress.pendingProps;
         return null;
       case Portal:
         // TODO: Only mark this as an update if we have any pending callbacks.
         markUpdate(workInProgress);
-        workInProgress.output = null;
         workInProgress.memoizedProps = workInProgress.pendingProps;
         return null;
 

src/renderers/shared/fiber/ReactFiberReconciler.js

@@ -14,7 +14,6 @@
 
 import type { Fiber } from 'ReactFiber';
 import type { FiberRoot } from 'ReactFiberRoot';
-import type { TypeOfWork } from 'ReactTypeOfWork';
 import type { PriorityLevel } from 'ReactPriorityLevel';
 
 var {
@@ -39,10 +38,6 @@ export type Deadline = {
   timeRemaining : () => number
 };
 
-type HostChildNode<I> = { tag: TypeOfWork, output: HostChildren<I>, sibling: any };
-
-export type HostChildren<I> = null | void | I | HostChildNode<I>;
-
 type OpaqueNode = Fiber;
 
 export type HostConfig<T, P, I, TI, C> = {

src/renderers/shared/fiber/ReactFiberScheduler.js

@@ -286,6 +286,12 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>) {
       workInProgress.pendingProps = null;
       workInProgress.updateQueue = null;
 
+      if (next) {
+        // If completing this work spawned new work, do that next. We'll come
+        // back here again.
+        return next;
+      }
+
       const returnFiber = workInProgress.return;
 
       if (returnFiber) {
@@ -318,10 +324,7 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>) {
         }
       }
 
-      if (next) {
-        // If completing this work spawned new work, do that next.
-        return next;
-      } else if (workInProgress.sibling) {
+      if (workInProgress.sibling) {
         // If there is more work to do in this returnFiber, do that next.
         return workInProgress.sibling;
       } else if (returnFiber) {

src/renderers/shared/fiber/__tests__/ReactCoroutine-test.js

@@ -24,10 +24,8 @@ describe('ReactCoroutine', () => {
   });
 
   it('should render a coroutine', () => {
-
     var ops = [];
 
-
     function Continuation({ isSame }) {
       ops.push(['Continuation', isSame]);
       return <span>{isSame ? 'foo==bar' : 'foo!=bar'}</span>;
@@ -84,7 +82,72 @@ describe('ReactCoroutine', () => {
       ['Continuation', true],
       ['Continuation', false],
     ]);
-
   });
 
+  it('should unmount a composite in a coroutine', () => {
+    var ops = [];
+
+    class Continuation extends React.Component {
+      render() {
+        ops.push('Continuation');
+        return <div />;
+      }
+      componentWillUnmount() {
+        ops.push('Unmount Continuation');
+      }
+    }
+
+    class Child extends React.Component {
+      render() {
+        ops.push('Child');
+        return ReactCoroutine.createYield({}, Continuation, null);
+      }
+      componentWillUnmount() {
+        ops.push('Unmount Child');
+      }
+    }
+
+    function HandleYields(props, yields) {
+      ops.push('HandleYields');
+      return yields.map(y => <y.continuation />);
+    }
+
+    class Parent extends React.Component {
+      render() {
+        ops.push('Parent');
+        return ReactCoroutine.createCoroutine(
+          this.props.children,
+          HandleYields,
+          this.props
+        );
+      }
+      componentWillUnmount() {
+        ops.push('Unmount Parent');
+      }
+    }
+
+    ReactNoop.render(<Parent><Child /></Parent>);
+    ReactNoop.flush();
+
+    expect(ops).toEqual([
+      'Parent',
+      'Child',
+      'HandleYields',
+      'Continuation',
+    ]);
+
+    ops = [];
+
+    ReactNoop.render(<div />);
+    ReactNoop.flush();
+
+    expect(ops).toEqual([
+      'Unmount Parent',
+      // TODO: This should happen in the order Child, Continuation which it
+      // will once we swap stateNode and child positions of these.
+      'Unmount Continuation',
+      'Unmount Child',
+    ]);
+
+  });
 });