From 0b997a65ba20a9886b1ec8ad1869668ec140d2f4 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Wed, 23 Nov 2016 19:02:37 -0800 Subject: [PATCH 1/2] 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. --- src/renderers/shared/fiber/ReactChildFiber.js | 14 ++-- src/renderers/shared/fiber/ReactFiber.js | 5 -- .../shared/fiber/ReactFiberCompleteWork.js | 69 ++++++++----------- .../shared/fiber/ReactFiberReconciler.js | 5 -- 4 files changed, 37 insertions(+), 56 deletions(-) diff --git a/src/renderers/shared/fiber/ReactChildFiber.js b/src/renderers/shared/fiber/ReactChildFiber.js index fad819060145..6a094f8250b6 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 349826a4e3cb..9c3604a5f861 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/ReactFiberCompleteWork.js b/src/renderers/shared/fiber/ReactFiberCompleteWork.js index d62a2926b762..dcbdfcafe69b 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 a6b54e85a93b..ca2fbb7660b0 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 = { From 4910b16197480c264f5356b6acfcf172e597a2cf Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Thu, 24 Nov 2016 10:01:26 -0800 Subject: [PATCH 2/2] Unmount child from the first phase of a coroutine --- scripts/fiber/tests-passing.txt | 1 + .../shared/fiber/ReactFiberCommitWork.js | 5 ++ .../shared/fiber/ReactFiberScheduler.js | 11 +-- .../fiber/__tests__/ReactCoroutine-test.js | 69 ++++++++++++++++++- 4 files changed, 79 insertions(+), 7 deletions(-) diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index 88ab2992a782..5867da6a6ad6 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/ReactFiberCommitWork.js b/src/renderers/shared/fiber/ReactFiberCommitWork.js index de09d2af44d2..31a62138fa8a 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/ReactFiberScheduler.js b/src/renderers/shared/fiber/ReactFiberScheduler.js index a78364de782f..58fc5445fc49 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 99f3177c0706..a5d54c8041a2 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', + ]); + + }); });