From 0ba8434a33a23c12e3ad0e8a0ccfce72d4c4ddd4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Thu, 24 Nov 2016 10:03:14 -0800 Subject: [PATCH] [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 --- scripts/fiber/tests-passing.txt | 1 + src/renderers/shared/fiber/ReactChildFiber.js | 14 ++-- src/renderers/shared/fiber/ReactFiber.js | 5 -- .../shared/fiber/ReactFiberCommitWork.js | 5 ++ .../shared/fiber/ReactFiberCompleteWork.js | 69 ++++++++----------- .../shared/fiber/ReactFiberReconciler.js | 5 -- .../shared/fiber/ReactFiberScheduler.js | 11 +-- .../fiber/__tests__/ReactCoroutine-test.js | 69 ++++++++++++++++++- 8 files changed, 116 insertions(+), 63 deletions(-) diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index 88ab2992a7821..5867da6a6ad61 100644 --- a/scripts/fiber/tests-passing.txt +++ b/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 diff --git a/src/renderers/shared/fiber/ReactChildFiber.js b/src/renderers/shared/fiber/ReactChildFiber.js index fad819060145e..6a094f8250b67 100644 --- a/src/renderers/shared/fiber/ReactChildFiber.js +++ b/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; } diff --git a/src/renderers/shared/fiber/ReactFiber.js b/src/renderers/shared/fiber/ReactFiber.js index 349826a4e3cbb..9c3604a5f861e 100644 --- a/src/renderers/shared/fiber/ReactFiber.js +++ b/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; }; diff --git a/src/renderers/shared/fiber/ReactFiberCommitWork.js b/src/renderers/shared/fiber/ReactFiberCommitWork.js index de09d2af44d28..31a62138fa8a9 100644 --- a/src/renderers/shared/fiber/ReactFiberCommitWork.js +++ b/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( detachRef(current); return; } + case CoroutineComponent: { + commitNestedUnmounts(current.stateNode); + return; + } } } diff --git a/src/renderers/shared/fiber/ReactFiberCompleteWork.js b/src/renderers/shared/fiber/ReactFiberCompleteWork.js index d62a2926b7621..dcbdfcafe69b8 100644 --- a/src/renderers/shared/fiber/ReactFiberCompleteWork.js +++ b/src/renderers/shared/fiber/ReactFiberCompleteWork.js @@ -62,28 +62,31 @@ module.exports = function(config : HostConfig) { 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, 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(config : HostConfig) { // Build up the yields. // TODO: Compare this to a generator or opaque helpers like Children. var yields : Array = []; - 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(config : HostConfig) { 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(config : HostConfig) { } 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(config : HostConfig) { // 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(config : HostConfig) { 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,16 +262,14 @@ module.exports = function(config : HostConfig) { } } 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; @@ -285,12 +277,11 @@ module.exports = function(config : HostConfig) { // 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; diff --git a/src/renderers/shared/fiber/ReactFiberReconciler.js b/src/renderers/shared/fiber/ReactFiberReconciler.js index a6b54e85a93bf..ca2fbb7660b06 100644 --- a/src/renderers/shared/fiber/ReactFiberReconciler.js +++ b/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 = { tag: TypeOfWork, output: HostChildren, sibling: any }; - -export type HostChildren = null | void | I | HostChildNode; - type OpaqueNode = Fiber; export type HostConfig = { diff --git a/src/renderers/shared/fiber/ReactFiberScheduler.js b/src/renderers/shared/fiber/ReactFiberScheduler.js index a78364de782fa..58fc5445fc49d 100644 --- a/src/renderers/shared/fiber/ReactFiberScheduler.js +++ b/src/renderers/shared/fiber/ReactFiberScheduler.js @@ -286,6 +286,12 @@ module.exports = function(config : HostConfig) { 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(config : HostConfig) { } } - 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) { diff --git a/src/renderers/shared/fiber/__tests__/ReactCoroutine-test.js b/src/renderers/shared/fiber/__tests__/ReactCoroutine-test.js index 99f3177c07068..a5d54c8041a22 100644 --- a/src/renderers/shared/fiber/__tests__/ReactCoroutine-test.js +++ b/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 {isSame ? 'foo==bar' : 'foo!=bar'}; @@ -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
; + } + 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 => ); + } + + 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(); + ReactNoop.flush(); + + expect(ops).toEqual([ + 'Parent', + 'Child', + 'HandleYields', + 'Continuation', + ]); + + ops = []; + + ReactNoop.render(
); + 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', + ]); + + }); });