From 648d6e190cf44870ab62a0e8a65189975ef44ee6 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Fri, 20 Jan 2017 22:21:58 -0800 Subject: [PATCH 1/5] Swap .child and .stateNode in coroutines It is slightly more useful this way because when we want to find host nodes we typically want to do so in the second phase. That's the real tree where as the first phase is more of a virtual part of the tree. --- .../shared/fiber/ReactFiberBeginWork.js | 42 ++++++++++++++++++- .../shared/fiber/ReactFiberCommitWork.js | 4 -- .../shared/fiber/ReactFiberCompleteWork.js | 23 +++++++--- .../shared/fiber/ReactFiberScheduler.js | 2 + .../shared/fiber/ReactFiberTreeReflection.js | 1 - .../fiber/__tests__/ReactCoroutine-test.js | 4 +- src/test/ReactTestUtils.js | 1 - 7 files changed, 60 insertions(+), 17 deletions(-) diff --git a/src/renderers/shared/fiber/ReactFiberBeginWork.js b/src/renderers/shared/fiber/ReactFiberBeginWork.js index dbbb91466956a..67caa03ad5191 100644 --- a/src/renderers/shared/fiber/ReactFiberBeginWork.js +++ b/src/renderers/shared/fiber/ReactFiberBeginWork.js @@ -518,11 +518,49 @@ module.exports = function( } else if (nextCoroutine === null || workInProgress.memoizedProps === nextCoroutine) { return bailoutOnAlreadyFinishedWork(current, workInProgress); } - reconcileChildren(current, workInProgress, nextCoroutine.children); + + const nextChildren = nextCoroutine.children; + const priorityLevel = workInProgress.pendingWorkPriority; + + // The following is a fork of reconcileChildrenAtPriority but using + // stateNode to store the child. + + // At this point any memoization is no longer valid since we'll have changed + // the children. + workInProgress.memoizedProps = null; + if (!current) { + workInProgress.stateNode = mountChildFibersInPlace( + workInProgress, + workInProgress.stateNode, + nextChildren, + priorityLevel + ); + } else if (current.child === workInProgress.child) { + clearDeletions(workInProgress); + + workInProgress.stateNode = reconcileChildFibers( + workInProgress, + workInProgress.stateNode, + nextChildren, + priorityLevel + ); + + transferDeletions(workInProgress); + } else { + workInProgress.stateNode = reconcileChildFibersInPlace( + workInProgress, + workInProgress.stateNode, + nextChildren, + priorityLevel + ); + + transferDeletions(workInProgress); + } + memoizeProps(workInProgress, nextCoroutine); // This doesn't take arbitrary time so we could synchronously just begin // eagerly do the work of workInProgress.child as an optimization. - return workInProgress.child; + return workInProgress.stateNode; } function updatePortalComponent(current, workInProgress) { diff --git a/src/renderers/shared/fiber/ReactFiberCommitWork.js b/src/renderers/shared/fiber/ReactFiberCommitWork.js index 37bef63964c2b..0dd429cdda195 100644 --- a/src/renderers/shared/fiber/ReactFiberCommitWork.js +++ b/src/renderers/shared/fiber/ReactFiberCommitWork.js @@ -136,7 +136,6 @@ module.exports = function( while (node.tag !== HostComponent && node.tag !== HostText) { // If it is not host node and, we might have a host node inside it. // Try to search down until we find one. - // TODO: For coroutines, this will have to search the stateNode. if (node.effectTag & Placement) { // If we don't have a child, try the siblings instead. continue siblings; @@ -198,7 +197,6 @@ module.exports = function( // down its children. Instead, we'll get insertions from each child in // the portal directly. } else if (node.child) { - // TODO: Coroutines need to visit the stateNode. node.child.return = node; node = node.child; continue; @@ -229,7 +227,6 @@ module.exports = function( // Visit children because they may contain more composite or host nodes. // Skip portals because commitUnmount() currently visits them recursively. if (node.child && node.tag !== HostPortal) { - // TODO: Coroutines need to visit the stateNode. node.child.return = node; node = node.child; continue; @@ -273,7 +270,6 @@ module.exports = function( commitUnmount(node); // Visit children because we may find more host components below. if (node.child) { - // TODO: Coroutines need to visit the stateNode. node.child.return = node; node = node.child; continue; diff --git a/src/renderers/shared/fiber/ReactFiberCompleteWork.js b/src/renderers/shared/fiber/ReactFiberCompleteWork.js index 776db30ecc018..08644759ca000 100644 --- a/src/renderers/shared/fiber/ReactFiberCompleteWork.js +++ b/src/renderers/shared/fiber/ReactFiberCompleteWork.js @@ -66,6 +66,18 @@ module.exports = function( popHostContainer, } = hostContext; + function markChildAsProgressed(current, workInProgress, priorityLevel) { + // We now have clones. Let's store them as the currently progressed work. + workInProgress.progressedChild = workInProgress.child; + workInProgress.progressedPriority = priorityLevel; + if (current) { + // We also store it on the current. When the alternate swaps in we can + // continue from this point. + current.progressedChild = workInProgress.progressedChild; + current.progressedPriority = workInProgress.progressedPriority; + } + } + function markUpdate(workInProgress : Fiber) { // Tag the fiber with an update effect. This turns a Placement into // an UpdateAndPlacement. @@ -73,7 +85,7 @@ module.exports = function( } function appendAllYields(yields : Array, workInProgress : Fiber) { - let node = workInProgress.child; + let node = workInProgress.stateNode; while (node) { if (node.tag === HostComponent || node.tag === HostText || node.tag === HostPortal) { @@ -81,7 +93,6 @@ module.exports = function( } 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; @@ -123,16 +134,17 @@ module.exports = function( var props = coroutine.props; var nextChildren = fn(props, yields); - var currentFirstChild = current ? current.stateNode : null; + var currentFirstChild = current ? current.child : null; // Inherit the priority of the returnFiber. const priority = workInProgress.pendingWorkPriority; - workInProgress.stateNode = reconcileChildFibers( + workInProgress.child = reconcileChildFibers( workInProgress, currentFirstChild, nextChildren, priority ); - return workInProgress.stateNode; + markChildAsProgressed(current, workInProgress, priority); + return workInProgress.child; } function appendAllChildren(parent : I, workInProgress : Fiber) { @@ -147,7 +159,6 @@ module.exports = function( // down its children. Instead, we'll get insertions from each child in // the portal directly. } else if (node.child) { - // TODO: Coroutines need to visit the stateNode. node = node.child; continue; } diff --git a/src/renderers/shared/fiber/ReactFiberScheduler.js b/src/renderers/shared/fiber/ReactFiberScheduler.js index 1b2fc170048ad..7cdf1b917d15b 100644 --- a/src/renderers/shared/fiber/ReactFiberScheduler.js +++ b/src/renderers/shared/fiber/ReactFiberScheduler.js @@ -435,6 +435,8 @@ module.exports = function(config : HostConfig { 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', + 'Unmount Continuation', ]); }); diff --git a/src/test/ReactTestUtils.js b/src/test/ReactTestUtils.js index 1a393d426b243..7a01f13cb7c38 100644 --- a/src/test/ReactTestUtils.js +++ b/src/test/ReactTestUtils.js @@ -97,7 +97,6 @@ function findAllInRenderedFiberTreeInternal(fiber, test) { } } if (node.child) { - // TODO: Coroutines need to visit the stateNode. node.child.return = node; node = node.child; continue; From fd8d5f7a8acadda78238c612311c9bd7bde4cf66 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Fri, 20 Jan 2017 23:17:51 -0800 Subject: [PATCH 2/5] Simplify coroutines by making yields stateless Coroutines was kind of broken because it tried to do reparenting and enabling state preservation to be passed along the coroutine. However, since we couldn't determine which Fiber was "current" on a reified yield this was kind of broken. This removes the "continuation" part of yields so they're basically just return values. It is still possible to do continuations by just passing simple functions or classes as part of the return value but they're not stateful. This means that we won't have reparenting, but I actually don't think we need it. There's another way to structure this by doing all the state in the first phase and then yielding a stateless representation of the result. This stateless representation of the tree can then be rendered in different (or even multiple) locations. Because we no longer have a stateful continuation, you may have noticed that this really no longer represent the "coroutine" concept. I will rename it in a follow up commit. --- .../classic/element/ReactElementValidator.js | 3 +- src/renderers/shared/fiber/ReactChildFiber.js | 62 ++++++------------- src/renderers/shared/fiber/ReactFiber.js | 3 +- .../shared/fiber/ReactFiberCompleteWork.js | 5 +- .../shared/fiber/ReactReifiedYield.js | 47 -------------- .../fiber/__tests__/ReactCoroutine-test.js | 11 ++-- .../shared/fiber/isomorphic/ReactCoroutine.js | 32 +++++----- 7 files changed, 45 insertions(+), 118 deletions(-) delete mode 100644 src/renderers/shared/fiber/ReactReifiedYield.js diff --git a/src/isomorphic/classic/element/ReactElementValidator.js b/src/isomorphic/classic/element/ReactElementValidator.js index f3910590dca5f..cc60aee305d51 100644 --- a/src/isomorphic/classic/element/ReactElementValidator.js +++ b/src/isomorphic/classic/element/ReactElementValidator.js @@ -200,8 +200,7 @@ var ReactElementValidator = { createElement: function(type, props, children) { var validType = typeof type === 'string' || - typeof type === 'function' || - type !== null && typeof type === 'object' && typeof type.tag === 'number'; + typeof type === 'function'; // We warn in this case but don't throw. We expect the element creation to // succeed and there will likely be errors in render. if (!validType) { diff --git a/src/renderers/shared/fiber/ReactChildFiber.js b/src/renderers/shared/fiber/ReactChildFiber.js index 95cea880cf906..0505ae07e34cd 100644 --- a/src/renderers/shared/fiber/ReactChildFiber.js +++ b/src/renderers/shared/fiber/ReactChildFiber.js @@ -29,7 +29,6 @@ var { } = require('ReactPortal'); var ReactFiber = require('ReactFiber'); -var ReactReifiedYield = require('ReactReifiedYield'); var ReactTypeOfSideEffect = require('ReactTypeOfSideEffect'); var ReactTypeOfWork = require('ReactTypeOfWork'); @@ -53,11 +52,6 @@ const { createFiberFromPortal, } = ReactFiber; -const { - createReifiedYield, - createUpdatedReifiedYield, -} = ReactReifiedYield; - const isArray = Array.isArray; const { @@ -316,21 +310,16 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { yieldNode : ReactYield, priority : PriorityLevel ) : Fiber { - // TODO: Should this also compare continuation to determine whether to reuse? if (current == null || current.tag !== YieldComponent) { // Insert - const reifiedYield = createReifiedYield(yieldNode); const created = createFiberFromYield(yieldNode, priority); - created.type = reifiedYield; + created.type = yieldNode.value; created.return = returnFiber; return created; } else { // Move based on index const existing = useFiber(current, priority); - existing.type = createUpdatedReifiedYield( - current.type, - yieldNode - ); + existing.type = yieldNode.value; existing.return = returnFiber; return existing; } @@ -411,9 +400,8 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { } case REACT_YIELD_TYPE: { - const reifiedYield = createReifiedYield(newChild); const created = createFiberFromYield(newChild, priority); - created.type = reifiedYield; + created.type = newChild.value; created.return = returnFiber; return created; } @@ -474,7 +462,10 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { } case REACT_YIELD_TYPE: { - if (newChild.key === key) { + // Yields doesn't have keys. If the previous node is implicitly keyed + // we can continue to replace it without aborting even if it is not a + // yield. + if (key === null) { return updateYield(returnFiber, oldFiber, newChild, priority); } else { return null; @@ -527,9 +518,9 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { } case REACT_YIELD_TYPE: { - const matchedFiber = existingChildren.get( - newChild.key === null ? newIdx : newChild.key - ) || null; + // Yields doesn't have keys, so we neither have to check the old nor + // new node for the key. If both are yields, they match. + const matchedFiber = existingChildren.get(newIdx) || null; return updateYield(returnFiber, matchedFiber, newChild, priority); } @@ -561,7 +552,6 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { switch (child.$$typeof) { case REACT_ELEMENT_TYPE: case REACT_COROUTINE_TYPE: - case REACT_YIELD_TYPE: case REACT_PORTAL_TYPE: const key = child.key; if (typeof key !== 'string') { @@ -1019,34 +1009,22 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { yieldNode : ReactYield, priority : PriorityLevel ) : Fiber { - const key = yieldNode.key; + // There's no need to check for keys on yields since they're stateless. let child = currentFirstChild; - while (child) { - // TODO: If key === null and child.key === null, then this only applies to - // the first item in the list. - if (child.key === key) { - if (child.tag === YieldComponent) { - deleteRemainingChildren(returnFiber, child.sibling); - const existing = useFiber(child, priority); - existing.type = createUpdatedReifiedYield( - child.type, - yieldNode - ); - existing.return = returnFiber; - return existing; - } else { - deleteRemainingChildren(returnFiber, child); - break; - } + if (child) { + if (child.tag === YieldComponent) { + deleteRemainingChildren(returnFiber, child.sibling); + const existing = useFiber(child, priority); + existing.type = yieldNode.value; + existing.return = returnFiber; + return existing; } else { - deleteChild(returnFiber, child); + deleteRemainingChildren(returnFiber, child); } - child = child.sibling; } - const reifiedYield = createReifiedYield(yieldNode); const created = createFiberFromYield(yieldNode, priority); - created.type = reifiedYield; + created.type = yieldNode.value; created.return = returnFiber; return created; } diff --git a/src/renderers/shared/fiber/ReactFiber.js b/src/renderers/shared/fiber/ReactFiber.js index 3657d087bb8c8..b488c0bbb69c3 100644 --- a/src/renderers/shared/fiber/ReactFiber.js +++ b/src/renderers/shared/fiber/ReactFiber.js @@ -393,8 +393,7 @@ exports.createFiberFromCoroutine = function(coroutine : ReactCoroutine, priority }; exports.createFiberFromYield = function(yieldNode : ReactYield, priorityLevel : PriorityLevel) : Fiber { - const fiber = createFiber(YieldComponent, yieldNode.key); - fiber.pendingProps = {}; + const fiber = createFiber(YieldComponent, null); return fiber; }; diff --git a/src/renderers/shared/fiber/ReactFiberCompleteWork.js b/src/renderers/shared/fiber/ReactFiberCompleteWork.js index 08644759ca000..fd59d37ef4576 100644 --- a/src/renderers/shared/fiber/ReactFiberCompleteWork.js +++ b/src/renderers/shared/fiber/ReactFiberCompleteWork.js @@ -17,7 +17,6 @@ import type { Fiber } from 'ReactFiber'; import type { HostContext } from 'ReactFiberHostContext'; import type { FiberRoot } from 'ReactFiberRoot'; import type { HostConfig } from 'ReactFiberReconciler'; -import type { ReifiedYield } from 'ReactReifiedYield'; var { reconcileChildFibers } = require('ReactChildFiber'); var { @@ -84,7 +83,7 @@ module.exports = function( workInProgress.effectTag |= Update; } - function appendAllYields(yields : Array, workInProgress : Fiber) { + function appendAllYields(yields : Array, workInProgress : Fiber) { let node = workInProgress.stateNode; while (node) { if (node.tag === HostComponent || node.tag === HostText || @@ -128,7 +127,7 @@ module.exports = function( // Build up the yields. // TODO: Compare this to a generator or opaque helpers like Children. - var yields : Array = []; + var yields : Array = []; appendAllYields(yields, workInProgress); var fn = coroutine.handler; var props = coroutine.props; diff --git a/src/renderers/shared/fiber/ReactReifiedYield.js b/src/renderers/shared/fiber/ReactReifiedYield.js deleted file mode 100644 index 1cb899c15ec59..0000000000000 --- a/src/renderers/shared/fiber/ReactReifiedYield.js +++ /dev/null @@ -1,47 +0,0 @@ -/** - * Copyright 2013-present, Facebook, Inc. - * All rights reserved. - * - * This source code is licensed under the BSD-style license found in the - * LICENSE file in the root directory of this source tree. An additional grant - * of patent rights can be found in the PATENTS file in the same directory. - * - * @providesModule ReactReifiedYield - * @flow - */ - -'use strict'; - -import type { ReactYield } from 'ReactCoroutine'; -import type { Fiber } from 'ReactFiber'; - -var { createFiberFromElementType } = require('ReactFiber'); - -export type ReifiedYield = { continuation: Fiber, props: Object }; - -exports.createReifiedYield = function(yieldNode : ReactYield) : ReifiedYield { - var fiber = createFiberFromElementType( - yieldNode.continuation, - yieldNode.key, - null // debugOwner - ); - return { - continuation: fiber, - props: yieldNode.props, - }; -}; - -exports.createUpdatedReifiedYield = function(previousYield : ReifiedYield, yieldNode : ReactYield) : ReifiedYield { - var fiber = previousYield.continuation; - if (fiber.type !== yieldNode.continuation) { - fiber = createFiberFromElementType( - yieldNode.continuation, - yieldNode.key, - null // debugOwner - ); - } - return { - continuation: fiber, - props: yieldNode.props, - }; -}; diff --git a/src/renderers/shared/fiber/__tests__/ReactCoroutine-test.js b/src/renderers/shared/fiber/__tests__/ReactCoroutine-test.js index b21b1025bac92..327e02c2bbb76 100644 --- a/src/renderers/shared/fiber/__tests__/ReactCoroutine-test.js +++ b/src/renderers/shared/fiber/__tests__/ReactCoroutine-test.js @@ -39,8 +39,11 @@ describe('ReactCoroutine', () => { function Child({ bar }) { ops.push(['Child', bar]); return ReactCoroutine.createYield({ - bar: bar, - }, Continuation, null); + props: { + bar: bar, + }, + continuation: Continuation, + }); } function Indirection() { @@ -103,7 +106,7 @@ describe('ReactCoroutine', () => { class Child extends React.Component { render() { ops.push('Child'); - return ReactCoroutine.createYield({}, Continuation, null); + return ReactCoroutine.createYield(Continuation); } componentWillUnmount() { ops.push('Unmount Child'); @@ -112,7 +115,7 @@ describe('ReactCoroutine', () => { function HandleYields(props, yields) { ops.push('HandleYields'); - return yields.map(y => ); + return yields.map(ContinuationComponent => ); } class Parent extends React.Component { diff --git a/src/renderers/shared/fiber/isomorphic/ReactCoroutine.js b/src/renderers/shared/fiber/isomorphic/ReactCoroutine.js index 609470fd39bc9..68f3a99b5d8e5 100644 --- a/src/renderers/shared/fiber/isomorphic/ReactCoroutine.js +++ b/src/renderers/shared/fiber/isomorphic/ReactCoroutine.js @@ -16,30 +16,29 @@ import type { ReactNodeList } from 'ReactTypes'; // The Symbol used to tag the special React types. If there is no native Symbol // nor polyfill, then a plain number is used for performance. -var REACT_COROUTINE_TYPE = - (typeof Symbol === 'function' && Symbol.for && Symbol.for('react.coroutine')) || - 0xeac8; +var REACT_COROUTINE_TYPE; +var REACT_YIELD_TYPE; +if (typeof Symbol === 'function' && Symbol.for) { + REACT_COROUTINE_TYPE = Symbol.for('react.coroutine'); + REACT_YIELD_TYPE = Symbol.for('react.yield'); +} else { + REACT_COROUTINE_TYPE = 0xeac8; + REACT_YIELD_TYPE = 0xeac9; +} -var REACT_YIELD_TYPE = - (typeof Symbol === 'function' && Symbol.for && Symbol.for('react.yield')) || - 0xeac9; - -type ReifiedYield = { continuation: Object, props: Object }; -type CoroutineHandler = (props: T, yields: Array) => ReactNodeList; +type CoroutineHandler = (props: T, yields: Array) => ReactNodeList; export type ReactCoroutine = { $$typeof: Symbol | number, key: null | string, children: any, // This should be a more specific CoroutineHandler - handler: (props: any, yields: Array) => ReactNodeList, + handler: (props: any, yields: Array) => ReactNodeList, props: any, }; export type ReactYield = { $$typeof: Symbol | number, - key: null | string, - props: Object, - continuation: mixed + value: mixed, }; exports.createCoroutine = function( @@ -68,19 +67,16 @@ exports.createCoroutine = function( return coroutine; }; -exports.createYield = function(props : mixed, continuation : mixed, key : ?string = null) { +exports.createYield = function(value : mixed) : ReactYield { var yieldNode = { // This tag allow us to uniquely identify this as a React Yield $$typeof: REACT_YIELD_TYPE, - key: key == null ? null : '' + key, - props: props, - continuation: continuation, + value: value, }; if (__DEV__) { // TODO: Add _store property for marking this as validated. if (Object.freeze) { - Object.freeze(yieldNode.props); Object.freeze(yieldNode); } } From e8500fb1c2df2b215c3a20aa596446eaea0390ad Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Fri, 20 Jan 2017 23:57:31 -0800 Subject: [PATCH 3/5] Add failing tests for coroutines There are a few different issues: * Updates result in unnecessary duplicate placements because it can't find the current fiber for continuations. * When run together, coroutine update and unmounting tests appear to lock down in an infinite loop. They don't freeze in isolation. I don't have a solution for this but just leaving it for future fixes. --- scripts/fiber/tests-passing.txt | 1 + .../fiber/__tests__/ReactCoroutine-test.js | 72 ++++++++++++++++++- 2 files changed, 72 insertions(+), 1 deletion(-) diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index d6733db76eafb..bd0a2dd82a8d7 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -1143,6 +1143,7 @@ src/renderers/shared/__tests__/ReactPerf-test.js src/renderers/shared/fiber/__tests__/ReactCoroutine-test.js * should render a coroutine +* should update a coroutine * should unmount a composite in a coroutine src/renderers/shared/fiber/__tests__/ReactIncremental-test.js diff --git a/src/renderers/shared/fiber/__tests__/ReactCoroutine-test.js b/src/renderers/shared/fiber/__tests__/ReactCoroutine-test.js index 327e02c2bbb76..140ea7ec0ecac 100644 --- a/src/renderers/shared/fiber/__tests__/ReactCoroutine-test.js +++ b/src/renderers/shared/fiber/__tests__/ReactCoroutine-test.js @@ -26,12 +26,21 @@ describe('ReactCoroutine', () => { ReactFeatureFlags.disableNewFiberFeatures = false; }); + function div(...children) { + children = children.map(c => typeof c === 'string' ? { text: c } : c); + return { type: 'div', children, prop: undefined }; + } + + function span(prop) { + return { type: 'span', children: [], prop }; + } + it('should render a coroutine', () => { var ops = []; function Continuation({ isSame }) { ops.push(['Continuation', isSame]); - return {isSame ? 'foo==bar' : 'foo!=bar'}; + return ; } // An alternative API could mark Continuation as something that needs @@ -88,6 +97,67 @@ describe('ReactCoroutine', () => { ['Continuation', true], ['Continuation', false], ]); + expect(ReactNoop.getChildren()).toEqual([ + div( + span('foo==bar'), + span('foo!=bar'), + ), + ]); + }); + + it('should update a coroutine', () => { + function Continuation({ isSame }) { + return ; + } + + function Child({ bar }) { + return ReactCoroutine.createYield({ + props: { + bar: bar, + }, + continuation: Continuation, + }); + } + + function Indirection() { + return [, ]; + } + + function HandleYields(props, yields) { + return yields.map(y => + + ); + } + + function Parent(props) { + return ReactCoroutine.createCoroutine( + props.children, + HandleYields, + props + ); + } + + function App(props) { + return
; + } + + ReactNoop.render(); + ReactNoop.flush(); + expect(ReactNoop.getChildren()).toEqual([ + div( + span('foo==bar'), + span('foo!=bar'), + ), + ]); + + ReactNoop.render(); + ReactNoop.flush(); + expect(ReactNoop.getChildren()).toEqual([ + div( + span('foo!=bar'), + span('foo==bar'), + ), + ]); }); it('should unmount a composite in a coroutine', () => { From 754f72a175f8448d9f0851a2a041ca0ceb12a46e Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Wed, 25 Jan 2017 17:36:20 -0800 Subject: [PATCH 4/5] Don't visit siblings of the coroutine when looking for yields When visiting the yields, the root is the stateNode of the coroutine. If its return value is wrong we'll end up at the alternate of the workInProgress which will start scanning its children which is wrong. --- src/renderers/shared/fiber/ReactFiberCompleteWork.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/renderers/shared/fiber/ReactFiberCompleteWork.js b/src/renderers/shared/fiber/ReactFiberCompleteWork.js index fd59d37ef4576..400bdf839465e 100644 --- a/src/renderers/shared/fiber/ReactFiberCompleteWork.js +++ b/src/renderers/shared/fiber/ReactFiberCompleteWork.js @@ -85,6 +85,9 @@ module.exports = function( function appendAllYields(yields : Array, workInProgress : Fiber) { let node = workInProgress.stateNode; + if (node) { + node.return = workInProgress; + } while (node) { if (node.tag === HostComponent || node.tag === HostText || node.tag === HostPortal) { @@ -96,9 +99,6 @@ module.exports = function( node = node.child; continue; } - if (node === workInProgress) { - return; - } while (!node.sibling) { if (!node.return || node.return === workInProgress) { return; From 975ad92e68781aaa5fce215e73496781799e9c3c Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Wed, 25 Jan 2017 17:50:21 -0800 Subject: [PATCH 5/5] Don't bail to .child of coroutines This should bail to stateNode instead. --- scripts/fiber/tests-passing.txt | 1 + .../shared/fiber/ReactFiberBeginWork.js | 5 ++- .../fiber/__tests__/ReactCoroutine-test.js | 41 +++++++++++++++++++ 3 files changed, 46 insertions(+), 1 deletion(-) diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index bd0a2dd82a8d7..a9d16d92ebc4a 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -1145,6 +1145,7 @@ src/renderers/shared/fiber/__tests__/ReactCoroutine-test.js * should render a coroutine * should update a coroutine * should unmount a composite in a coroutine +* should handle deep updates in coroutine src/renderers/shared/fiber/__tests__/ReactIncremental-test.js * should render a simple component diff --git a/src/renderers/shared/fiber/ReactFiberBeginWork.js b/src/renderers/shared/fiber/ReactFiberBeginWork.js index 67caa03ad5191..a19f6659fc2a6 100644 --- a/src/renderers/shared/fiber/ReactFiberBeginWork.js +++ b/src/renderers/shared/fiber/ReactFiberBeginWork.js @@ -516,7 +516,10 @@ module.exports = function( } } } else if (nextCoroutine === null || workInProgress.memoizedProps === nextCoroutine) { - return bailoutOnAlreadyFinishedWork(current, workInProgress); + nextCoroutine = workInProgress.memoizedProps; + // TODO: When bailing out, we might need to return the stateNode instead + // of the child. To check it for work. + // return bailoutOnAlreadyFinishedWork(current, workInProgress); } const nextChildren = nextCoroutine.children; diff --git a/src/renderers/shared/fiber/__tests__/ReactCoroutine-test.js b/src/renderers/shared/fiber/__tests__/ReactCoroutine-test.js index 140ea7ec0ecac..9de98abb1c500 100644 --- a/src/renderers/shared/fiber/__tests__/ReactCoroutine-test.js +++ b/src/renderers/shared/fiber/__tests__/ReactCoroutine-test.js @@ -224,4 +224,45 @@ describe('ReactCoroutine', () => { ]); }); + + it('should handle deep updates in coroutine', () => { + let instances = {}; + + class Counter extends React.Component { + state = {value: 5}; + render() { + instances[this.props.id] = this; + return ReactCoroutine.createYield(this.state.value); + } + } + + function App(props) { + return ReactCoroutine.createCoroutine( + [ + , + , + , + ], + (p, yields) => yields.map(y => ), + {} + ); + } + + ReactNoop.render(); + ReactNoop.flush(); + expect(ReactNoop.getChildren()).toEqual([ + span(500), + span(500), + span(500), + ]); + + instances.a.setState({value: 1}); + instances.b.setState({value: 2}); + ReactNoop.flush(); + expect(ReactNoop.getChildren()).toEqual([ + span(100), + span(200), + span(500), + ]); + }); });