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', + ]); + + }); });