From 78add2dc639e4a8aaa9ea77bec64e45fac6aa11b Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Wed, 23 Nov 2016 15:30:00 -0800 Subject: [PATCH 1/4] Update root children using the appendChild/insertBefore/removeChild methods This removes updateContainer and instead uses the regular child mutation methods to insert into the root container and portals. Since we're no longer clearing out the container DOM in updateContainer we have to do that manually during initial mount. This now works on a document and one of the tests end up unmounting the body when you render into the document so I had to work around that bit since we don't yet properly support rendering into the document root. --- scripts/fiber/tests-failing.txt | 1 - scripts/fiber/tests-passing.txt | 1 + src/renderers/dom/fiber/ReactDOMFiber.js | 30 +++++----- src/renderers/noop/ReactNoop.js | 10 +--- .../shared/fiber/ReactFiberBeginWork.js | 19 ++++++- .../shared/fiber/ReactFiberCommitWork.js | 55 +++++++++++-------- .../shared/fiber/ReactFiberCompleteWork.js | 8 +-- .../shared/fiber/ReactFiberReconciler.js | 12 +--- 8 files changed, 76 insertions(+), 60 deletions(-) diff --git a/scripts/fiber/tests-failing.txt b/scripts/fiber/tests-failing.txt index 8084ad6f828..63a7de9dff8 100644 --- a/scripts/fiber/tests-failing.txt +++ b/scripts/fiber/tests-failing.txt @@ -120,4 +120,3 @@ src/renderers/shared/stack/reconciler/__tests__/refs-test.js src/test/__tests__/ReactTestUtils-test.js * traverses children in the correct order -* should support injected wrapper components as DOM components diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index 52430274b69..cd096d16be0 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -1560,6 +1560,7 @@ src/test/__tests__/ReactTestUtils-test.js * can scryRenderedDOMComponentsWithClass with TextComponent * can scryRenderedDOMComponentsWithClass with className contains \n * can scryRenderedDOMComponentsWithClass with multiple classes +* should support injected wrapper components as DOM components * should change the value of an input field * should change the value of an input field in a component * should throw when attempting to use ReactTestUtils.Simulate with shallow rendering diff --git a/src/renderers/dom/fiber/ReactDOMFiber.js b/src/renderers/dom/fiber/ReactDOMFiber.js index f09f9ddbe19..1bd071d04f1 100644 --- a/src/renderers/dom/fiber/ReactDOMFiber.js +++ b/src/renderers/dom/fiber/ReactDOMFiber.js @@ -38,6 +38,8 @@ var { } = ReactDOMFiberComponent; var { precacheFiberNode } = ReactDOMComponentTree; +const DOCUMENT_NODE = 9; + ReactDOMInjection.inject(); ReactControlledComponent.injection.injectFiberControlledHostComponent( ReactDOMFiberComponent @@ -88,19 +90,13 @@ var DOMRenderer = ReactFiberReconciler({ eventsEnabled = null; }, - updateContainer(container : Container, children : HostChildren) : void { - // TODO: Containers should update similarly to other parents. - container.innerHTML = ''; - recursivelyAppendChildren(container, children); - }, - createInstance( type : string, props : Props, children : HostChildren, internalInstanceHandle : Object ) : Instance { - const root = document.body; // HACK + const root = document.documentElement; // HACK const domElement : Instance = createElement(type, props, root); precacheFiberNode(internalInstanceHandle, domElement); @@ -124,7 +120,7 @@ var DOMRenderer = ReactFiberReconciler({ internalInstanceHandle : Object ) : void { var type = domElement.tagName.toLowerCase(); // HACK - var root = document.body; // HACK + var root = document.documentElement; // HACK // Update the internal instance handle so that we know which props are // the current ones. precacheFiberNode(internalInstanceHandle, domElement); @@ -141,19 +137,19 @@ var DOMRenderer = ReactFiberReconciler({ textInstance.nodeValue = newText; }, - appendChild(parentInstance : Instance, child : Instance | TextInstance) : void { + appendChild(parentInstance : Instance | Container, child : Instance | TextInstance) : void { parentInstance.appendChild(child); }, insertBefore( - parentInstance : Instance, + parentInstance : Instance | Container, child : Instance | TextInstance, beforeChild : Instance | TextInstance ) : void { parentInstance.insertBefore(child, beforeChild); }, - removeChild(parentInstance : Instance, child : Instance | TextInstance) : void { + removeChild(parentInstance : Instance | Container, child : Instance | TextInstance) : void { parentInstance.removeChild(child); }, @@ -177,9 +173,15 @@ function warnAboutUnstableUse() { warned = true; } -function renderSubtreeIntoContainer(parentComponent : ?ReactComponent, element : ReactElement, container : DOMContainerElement, callback: ?Function) { +function renderSubtreeIntoContainer(parentComponent : ?ReactComponent, element : ReactElement, containerNode : DOMContainerElement | Document, callback: ?Function) { + let container : DOMContainerElement = + containerNode.nodeType === DOCUMENT_NODE ? (containerNode : any).documentElement : (containerNode : any); let root; if (!container._reactRootContainer) { + // First clear any existing content. + while (container.lastChild) { + container.removeChild(container.lastChild); + } root = container._reactRootContainer = DOMRenderer.mountContainer(element, container, parentComponent, callback); } else { DOMRenderer.updateContainer(element, root = container._reactRootContainer, parentComponent, callback); @@ -194,12 +196,12 @@ var ReactDOM = { return renderSubtreeIntoContainer(null, element, container, callback); }, - unstable_renderSubtreeIntoContainer(parentComponent : ReactComponent, element : ReactElement, container : DOMContainerElement, callback: ?Function) { + unstable_renderSubtreeIntoContainer(parentComponent : ReactComponent, element : ReactElement, containerNode : DOMContainerElement | Document, callback: ?Function) { invariant( parentComponent != null && ReactInstanceMap.has(parentComponent), 'parentComponent must be a valid React Component' ); - return renderSubtreeIntoContainer(parentComponent, element, container, callback); + return renderSubtreeIntoContainer(parentComponent, element, containerNode, callback); }, unmountComponentAtNode(container : DOMContainerElement) { diff --git a/src/renderers/noop/ReactNoop.js b/src/renderers/noop/ReactNoop.js index 5bd32f08c7d..febe0b2c772 100644 --- a/src/renderers/noop/ReactNoop.js +++ b/src/renderers/noop/ReactNoop.js @@ -67,10 +67,6 @@ function flattenChildren(children : HostChildren) { var NoopRenderer = ReactFiberReconciler({ - updateContainer(containerInfo : Container, children : HostChildren) : void { - containerInfo.children = flattenChildren(children); - }, - createInstance(type : string, props : Props, children : HostChildren) : Instance { const inst = { tag: TERMINAL_TAG, @@ -104,7 +100,7 @@ var NoopRenderer = ReactFiberReconciler({ textInstance.text = newText; }, - appendChild(parentInstance : Instance, child : Instance | TextInstance) : void { + appendChild(parentInstance : Instance | Container, child : Instance | TextInstance) : void { const index = parentInstance.children.indexOf(child); if (index !== -1) { parentInstance.children.splice(index, 1); @@ -113,7 +109,7 @@ var NoopRenderer = ReactFiberReconciler({ }, insertBefore( - parentInstance : Instance, + parentInstance : Instance | Container, child : Instance | TextInstance, beforeChild : Instance | TextInstance ) : void { @@ -128,7 +124,7 @@ var NoopRenderer = ReactFiberReconciler({ parentInstance.children.splice(beforeIndex, 0, child); }, - removeChild(parentInstance : Instance, child : Instance | TextInstance) : void { + removeChild(parentInstance : Instance | Container, child : Instance | TextInstance) : void { const index = parentInstance.children.indexOf(child); if (index === -1) { throw new Error('This child does not exist.'); diff --git a/src/renderers/shared/fiber/ReactFiberBeginWork.js b/src/renderers/shared/fiber/ReactFiberBeginWork.js index 5d669db5525..887d142691c 100644 --- a/src/renderers/shared/fiber/ReactFiberBeginWork.js +++ b/src/renderers/shared/fiber/ReactFiberBeginWork.js @@ -300,7 +300,24 @@ module.exports = function( } function updatePortalComponent(current, workInProgress) { - reconcileChildren(current, workInProgress, workInProgress.pendingProps); + const priorityLevel = workInProgress.pendingWorkPriority; + const nextChildren = workInProgress.pendingProps; + if (!current) { + // Portals are special because we don't append the children during mount + // but at commit. Therefore we need to track insertions which the normal + // flow doesn't do during mount. This doesn't happen at the root because + // the root always starts with a "current" with a null child. + // TODO: Consider unifying this with how the root works. + workInProgress.child = reconcileChildFibersInPlace( + workInProgress, + workInProgress.child, + nextChildren, + priorityLevel + ); + markChildAsProgressed(current, workInProgress, priorityLevel); + } else { + reconcileChildren(current, workInProgress, nextChildren); + } } /* diff --git a/src/renderers/shared/fiber/ReactFiberCommitWork.js b/src/renderers/shared/fiber/ReactFiberCommitWork.js index 3b195734df0..9e00ff04573 100644 --- a/src/renderers/shared/fiber/ReactFiberCommitWork.js +++ b/src/renderers/shared/fiber/ReactFiberCommitWork.js @@ -13,7 +13,6 @@ 'use strict'; import type { Fiber } from 'ReactFiber'; -import type { FiberRoot } from 'ReactFiberRoot'; import type { HostConfig } from 'ReactFiberReconciler'; var ReactTypeOfWork = require('ReactTypeOfWork'); @@ -37,7 +36,6 @@ module.exports = function( trapError : (failedFiber : Fiber, error: Error, isUnmounting : boolean) => void ) { - const updateContainer = config.updateContainer; const commitUpdate = config.commitUpdate; const commitTextUpdate = config.commitTextUpdate; @@ -68,22 +66,30 @@ module.exports = function( } } - function getHostParent(fiber : Fiber) : ?I { + function getHostParent(fiber : Fiber) : null | I | C { let parent = fiber.return; while (parent) { switch (parent.tag) { case HostComponent: return parent.stateNode; case HostContainer: - // TODO: Currently we use the updateContainer feature to update these, - // but we should be able to handle this case too. - return null; + return parent.stateNode.containerInfo; + case Portal: + return parent.stateNode.containerInfo; } parent = parent.return; } return null; } + function isHostParent(fiber : Fiber) : boolean { + return ( + fiber.tag === HostComponent || + fiber.tag === HostContainer || + fiber.tag === Portal + ); + } + function getHostSibling(fiber : Fiber) : ?I { // We're going to search forward into the tree until we find a sibling host // node. Unfortunately, if multiple insertions are done in a row we have to @@ -93,7 +99,7 @@ module.exports = function( siblings: while (true) { // If we didn't find anything, let's try the next sibling. while (!node.sibling) { - if (!node.return || node.return.tag === HostComponent) { + if (!node.return || isHostParent(node.return)) { // If we pop out of the root or hit the parent the fiber we are the // last sibling. return null; @@ -142,6 +148,10 @@ module.exports = function( } else { appendChild(parent, node.stateNode); } + } else if (node.tag === Portal) { + // If the insertion itself is a portal, then we don't want to traverse + // 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; @@ -201,6 +211,18 @@ module.exports = function( if (parent) { removeChild(parent, node.stateNode); } + } else if (node.tag === Portal) { + // If this is a portal, then the parent is actually the portal itself. + // We need to keep track of which parent we're removing from. + // TODO: This uses a recursive call. We can get rid of that by mutating + // the parent binding and restoring it by searching for the host parent + // again when we pop past a portal. + const portalParent = node.stateNode.containerInfo; + let child = node.child; + while (child) { + unmountHostComponents(portalParent, child); + child = child.sibling; + } } else { commitUnmount(node); if (node.child) { @@ -260,11 +282,6 @@ module.exports = function( detachRef(current); return; } - case Portal: { - const containerInfo : C = current.stateNode.containerInfo; - updateContainer(containerInfo, null); - return; - } } } @@ -274,14 +291,6 @@ module.exports = function( detachRefIfNeeded(current, finishedWork); return; } - case HostContainer: { - // TODO: Attach children to root container. - const children = finishedWork.output; - const root : FiberRoot = finishedWork.stateNode; - const containerInfo : C = root.containerInfo; - updateContainer(containerInfo, children); - return; - } case HostComponent: { const instance : I = finishedWork.stateNode; if (instance != null && current) { @@ -303,10 +312,10 @@ module.exports = function( commitTextUpdate(textInstance, oldText, newText); return; } + case HostContainer: { + return; + } case Portal: { - const children = finishedWork.child; - const containerInfo : C = finishedWork.stateNode.containerInfo; - updateContainer(containerInfo, children); return; } default: diff --git a/src/renderers/shared/fiber/ReactFiberCompleteWork.js b/src/renderers/shared/fiber/ReactFiberCompleteWork.js index dc4ab684433..4a8f35b21e5 100644 --- a/src/renderers/shared/fiber/ReactFiberCompleteWork.js +++ b/src/renderers/shared/fiber/ReactFiberCompleteWork.js @@ -167,11 +167,8 @@ module.exports = function(config : HostConfig) { fiberRoot.context = fiberRoot.pendingContext; fiberRoot.pendingContext = null; } - // We don't know if a container has updated any children so we always - // need to update it right now. We schedule this side-effect before - // all the other side-effects in the subtree. We need to schedule it - // before so that the entire tree is up-to-date before the life-cycles - // are invoked. + // TODO: Only mark this as an update if we have any pending callbacks + // on it. markUpdate(workInProgress); return null; } @@ -253,6 +250,7 @@ module.exports = function(config : HostConfig) { transferOutput(workInProgress.child, workInProgress); 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; diff --git a/src/renderers/shared/fiber/ReactFiberReconciler.js b/src/renderers/shared/fiber/ReactFiberReconciler.js index 15b415f88d7..13828107a52 100644 --- a/src/renderers/shared/fiber/ReactFiberReconciler.js +++ b/src/renderers/shared/fiber/ReactFiberReconciler.js @@ -47,12 +47,6 @@ type OpaqueNode = Fiber; export type HostConfig = { - // TODO: We don't currently have a quick way to detect that children didn't - // reorder so we host will always need to check the set. We should make a flag - // or something so that it can bailout easily. - - updateContainer(containerInfo : C, children : HostChildren) : void, - createInstance(type : T, props : P, children : HostChildren, internalInstanceHandle : OpaqueNode) : I, prepareUpdate(instance : I, oldProps : P, newProps : P) : boolean, commitUpdate(instance : I, oldProps : P, newProps : P, internalInstanceHandle : OpaqueNode) : void, @@ -60,9 +54,9 @@ export type HostConfig = { createTextInstance(text : string, internalInstanceHandle : OpaqueNode) : TI, commitTextUpdate(textInstance : TI, oldText : string, newText : string) : void, - appendChild(parentInstance : I, child : I | TI) : void, - insertBefore(parentInstance : I, child : I | TI, beforeChild : I | TI) : void, - removeChild(parentInstance : I, child : I | TI) : void, + appendChild(parentInstance : I | C, child : I | TI) : void, + insertBefore(parentInstance : I | C, child : I | TI, beforeChild : I | TI) : void, + removeChild(parentInstance : I | C, child : I | TI) : void, scheduleAnimationCallback(callback : () => void) : void, scheduleDeferredCallback(callback : (deadline : Deadline) => void) : void, From fde18a4562f0cfe5dce3afce44579616d09d079a Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Wed, 23 Nov 2016 15:29:00 -0800 Subject: [PATCH 2/4] Assert that we always find a parent for DOM mutations --- src/renderers/shared/fiber/ReactFiberCommitWork.js | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/src/renderers/shared/fiber/ReactFiberCommitWork.js b/src/renderers/shared/fiber/ReactFiberCommitWork.js index 9e00ff04573..0fe3c232bd2 100644 --- a/src/renderers/shared/fiber/ReactFiberCommitWork.js +++ b/src/renderers/shared/fiber/ReactFiberCommitWork.js @@ -66,7 +66,7 @@ module.exports = function( } } - function getHostParent(fiber : Fiber) : null | I | C { + function getHostParent(fiber : Fiber) : I | C { let parent = fiber.return; while (parent) { switch (parent.tag) { @@ -79,7 +79,7 @@ module.exports = function( } parent = parent.return; } - return null; + throw new Error('Expected to find a host parent.'); } function isHostParent(fiber : Fiber) : boolean { @@ -134,9 +134,6 @@ module.exports = function( function commitInsertion(finishedWork : Fiber) : void { // Recursively insert all host nodes into the parent. const parent = getHostParent(finishedWork); - if (!parent) { - return; - } const before = getHostSibling(finishedWork); // We only have the top Fiber that was inserted but we need recurse down its // children to find all the terminal nodes. @@ -208,9 +205,7 @@ module.exports = function( commitNestedUnmounts(node); // After all the children have unmounted, it is now safe to remove the // node from the tree. - if (parent) { - removeChild(parent, node.stateNode); - } + removeChild(parent, node.stateNode); } else if (node.tag === Portal) { // If this is a portal, then the parent is actually the portal itself. // We need to keep track of which parent we're removing from. From 024e2a02591578dcf91ac68920b1ea0ff82f3949 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Thu, 24 Nov 2016 00:17:59 +0000 Subject: [PATCH 3/4] Remove recursion from unmounting portals (#1) --- .../shared/fiber/ReactFiberCommitWork.js | 21 +++++++++---------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/src/renderers/shared/fiber/ReactFiberCommitWork.js b/src/renderers/shared/fiber/ReactFiberCommitWork.js index 0fe3c232bd2..2636429ac23 100644 --- a/src/renderers/shared/fiber/ReactFiberCommitWork.js +++ b/src/renderers/shared/fiber/ReactFiberCommitWork.js @@ -207,17 +207,11 @@ module.exports = function( // node from the tree. removeChild(parent, node.stateNode); } else if (node.tag === Portal) { - // If this is a portal, then the parent is actually the portal itself. - // We need to keep track of which parent we're removing from. - // TODO: This uses a recursive call. We can get rid of that by mutating - // the parent binding and restoring it by searching for the host parent - // again when we pop past a portal. - const portalParent = node.stateNode.containerInfo; - let child = node.child; - while (child) { - unmountHostComponents(portalParent, child); - child = child.sibling; - } + // When we go into a portal, it becomes the parent to remove from. + // We will reassign it back when we pop the portal on the way up. + parent = node.stateNode.containerInfo; + node = node.child; + continue; } else { commitUnmount(node); if (node.child) { @@ -235,6 +229,11 @@ module.exports = function( return; } node = node.return; + if (node.tag === Portal) { + // When we go out of the portal, we need to restore the parent. + // Since we don't keep a stack of them, we will search for it. + parent = getHostParent(node); + } } node.sibling.return = node.return; node = node.sibling; From ea3420471f0e79b2212b95159d6959d221a52ca1 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Wed, 23 Nov 2016 17:38:20 -0800 Subject: [PATCH 4/4] Split initial children out of createInstance The goal of this is to avoid passing an opaque data structure that needs to be recursively searched by the host. I considered having some helper for doing the recursion but I figured it might be helpful to let the reconciler move this around. For example we might want to create an instance in beginWork and add to it as we go. This would let us avoid traversing the tree twice and would solve the IE11 perf issue. So instead, we create the instance first then call appendChild. I could just call the normal one but I figured that I would make a special one just in case. For example if you wanted to perform commits on a separate thread from creation. This turned out to be useful in ReactNoop where I can avoid searching the array for an existing one since I know the child isn't there already. (Although splitting placement into insertion/move might be better.) Finally, we need the ability to update an instance after all the children have been insertion. Such as `