From 513c18bc5d2e5ef9c8efff5871d336b71f4fd088 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Fri, 7 Feb 2020 10:46:52 +0000 Subject: [PATCH 1/2] Refactor commitPlacement to recursively insert nodes Fix Flow types --- packages/react-dom/src/client/ReactDOM.js | 8 ++- .../src/client/ReactDOMHostConfig.js | 36 +++++----- .../src/ReactNativeHostConfig.js | 6 +- .../src/ReactFiberCommitWork.js | 67 ++++++++++--------- 4 files changed, 62 insertions(+), 55 deletions(-) diff --git a/packages/react-dom/src/client/ReactDOM.js b/packages/react-dom/src/client/ReactDOM.js index ee54497f232af..92555e6bbf91c 100644 --- a/packages/react-dom/src/client/ReactDOM.js +++ b/packages/react-dom/src/client/ReactDOM.js @@ -113,8 +113,14 @@ setBatchingImplementation( batchedEventUpdates, ); +export type DOMInstance = Element & {_reactRootContainer: ?RootType, ...}; +export type DOMHTMLInstance = HTMLElement & { + _reactRootContainer: ?RootType, + ... +}; + export type DOMContainer = - | (Element & {_reactRootContainer: ?RootType, ...}) + | DOMInstance | (Document & {_reactRootContainer: ?RootType, ...}); function createPortal( diff --git a/packages/react-dom/src/client/ReactDOMHostConfig.js b/packages/react-dom/src/client/ReactDOMHostConfig.js index 393f9f033f72a..a1a780186e9d4 100644 --- a/packages/react-dom/src/client/ReactDOMHostConfig.js +++ b/packages/react-dom/src/client/ReactDOMHostConfig.js @@ -45,7 +45,7 @@ import { } from '../shared/HTMLNodeType'; import dangerousStyleValue from '../shared/dangerousStyleValue'; -import type {DOMContainer} from './ReactDOM'; +import type {DOMContainer, DOMInstance, DOMHTMLInstance} from './ReactDOM'; import type { ReactDOMEventResponder, ReactDOMEventResponderInstance, @@ -88,8 +88,8 @@ export type EventTargetChildElement = { }, ... }; -export type Container = Element | Document; -export type Instance = Element; +export type Container = DOMContainer; +export type Instance = DOMInstance; export type TextInstance = Text; export type SuspenseInstance = Comment & {_reactRetry?: () => void, ...}; export type HydratableInstance = Instance | TextInstance | SuspenseInstance; @@ -252,7 +252,7 @@ export function createInstance( } else { parentNamespace = ((hostContext: any): HostContextProd); } - const domElement: Instance = createElement( + const domElement = createElement( type, props, rootContainerInstance, @@ -260,7 +260,7 @@ export function createInstance( ); precacheFiberNode(internalInstanceHandle, domElement); updateFiberProps(domElement, props); - return domElement; + return ((domElement: any): DOMInstance); } export function appendInitialChild( @@ -398,7 +398,7 @@ export function commitUpdate( updateProperties(domElement, updatePayload, type, oldProps, newProps); } -export function resetTextContent(domElement: Instance): void { +export function resetTextContent(domElement: DOMInstance): void { setTextContent(domElement, ''); } @@ -411,15 +411,15 @@ export function commitTextUpdate( } export function appendChild( - parentInstance: Instance, - child: Instance | TextInstance, + parentInstance: DOMInstance, + child: DOMInstance | TextInstance, ): void { parentInstance.appendChild(child); } export function appendChildToContainer( - container: DOMContainer, - child: Instance | TextInstance, + container: DOMInstance, + child: DOMInstance | TextInstance, ): void { let parentNode; if (container.nodeType === COMMENT_NODE) { @@ -448,17 +448,17 @@ export function appendChildToContainer( } export function insertBefore( - parentInstance: Instance, - child: Instance | TextInstance, - beforeChild: Instance | TextInstance | SuspenseInstance, + parentInstance: DOMContainer, + child: DOMInstance | TextInstance, + beforeChild: DOMInstance | TextInstance | SuspenseInstance, ): void { parentInstance.insertBefore(child, beforeChild); } export function insertInContainerBefore( - container: Container, - child: Instance | TextInstance, - beforeChild: Instance | TextInstance | SuspenseInstance, + container: DOMContainer, + child: DOMInstance | TextInstance, + beforeChild: DOMInstance | TextInstance | SuspenseInstance, ): void { if (container.nodeType === COMMENT_NODE) { (container.parentNode: any).insertBefore(child, beforeChild); @@ -587,7 +587,7 @@ export function clearSuspenseBoundaryFromContainer( export function hideInstance(instance: Instance): void { // TODO: Does this work for all element types? What about MathML? Should we // pass host context to this method? - instance = ((instance: any): HTMLElement); + instance = ((instance: any): DOMHTMLInstance); const style = instance.style; if (typeof style.setProperty === 'function') { style.setProperty('display', 'none', 'important'); @@ -601,7 +601,7 @@ export function hideTextInstance(textInstance: TextInstance): void { } export function unhideInstance(instance: Instance, props: Props): void { - instance = ((instance: any): HTMLElement); + instance = ((instance: any): DOMHTMLInstance); const styleProp = props[STYLE]; const display = styleProp !== undefined && diff --git a/packages/react-native-renderer/src/ReactNativeHostConfig.js b/packages/react-native-renderer/src/ReactNativeHostConfig.js index e91c16a4909dc..1584a2192ba05 100644 --- a/packages/react-native-renderer/src/ReactNativeHostConfig.js +++ b/packages/react-native-renderer/src/ReactNativeHostConfig.js @@ -289,12 +289,12 @@ export function appendChild( } export function appendChildToContainer( - parentInstance: Container, + parentInstance: Instance | Container, child: Instance | TextInstance, ): void { const childTag = typeof child === 'number' ? child : child._nativeTag; UIManager.setChildren( - parentInstance, // containerTag + ((parentInstance: any): Container), // containerTag [childTag], // reactTags ); } @@ -386,7 +386,7 @@ export function insertBefore( } export function insertInContainerBefore( - parentInstance: Container, + parentInstance: Instance | Container, child: Instance | TextInstance, beforeChild: Instance | TextInstance, ): void { diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.js b/packages/react-reconciler/src/ReactFiberCommitWork.js index f4282ac4fa5b5..69a64dcd70eae 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.js @@ -1099,44 +1099,45 @@ function commitPlacement(finishedWork: Fiber): void { const before = getHostSibling(finishedWork); // We only have the top Fiber that was inserted but we need to recurse down its // children to find all the terminal nodes. - let node: Fiber = finishedWork; - while (true) { - const isHost = node.tag === HostComponent || node.tag === HostText; - if (isHost || (enableFundamentalAPI && node.tag === FundamentalComponent)) { - const stateNode = isHost ? node.stateNode : node.stateNode.instance; - if (before) { - if (isContainer) { - insertInContainerBefore(parent, stateNode, before); - } else { - insertBefore(parent, stateNode, before); - } + insertOrAppendPlacementNode(finishedWork, before, parent, isContainer); +} + +function insertOrAppendPlacementNode( + node: Fiber, + before: ?Instance, + parent: Instance, + isContainer: boolean, +): void { + const isHost = node.tag === HostComponent || node.tag === HostText; + if (isHost || (enableFundamentalAPI && node.tag === FundamentalComponent)) { + const stateNode = isHost ? node.stateNode : node.stateNode.instance; + if (before) { + if (isContainer) { + insertInContainerBefore(parent, stateNode, before); } else { - if (isContainer) { - appendChildToContainer(parent, stateNode); - } else { - appendChild(parent, stateNode); - } + insertBefore(parent, stateNode, before); + } + } else { + if (isContainer) { + appendChildToContainer(parent, stateNode); + } else { + appendChild(parent, stateNode); } - } else if (node.tag === HostPortal) { - // 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 !== null) { - node.child.return = node; - node = node.child; - continue; - } - if (node === finishedWork) { - return; } - while (node.sibling === null) { - if (node.return === null || node.return === finishedWork) { - return; + } else if (node.tag === HostPortal) { + // 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 { + const child = node.child; + if (child !== null) { + insertOrAppendPlacementNode(child, before, parent, isContainer); + let sibling = child.sibling; + while (sibling !== null) { + insertOrAppendPlacementNode(sibling, before, parent, isContainer); + sibling = sibling.sibling; } - node = node.return; } - node.sibling.return = node.return; - node = node.sibling; } } From 281a345b0a4fc65833d1c422fefe7c46cc162307 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Sat, 8 Feb 2020 10:09:14 +0000 Subject: [PATCH 2/2] Address feedback Other file too --- packages/react-dom/src/client/ReactDOM.js | 8 +-- .../src/client/ReactDOMHostConfig.js | 34 +++++------ .../src/ReactNativeHostConfig.js | 6 +- .../src/ReactFiberCommitWork.js | 61 +++++++++++++------ 4 files changed, 65 insertions(+), 44 deletions(-) diff --git a/packages/react-dom/src/client/ReactDOM.js b/packages/react-dom/src/client/ReactDOM.js index 92555e6bbf91c..ee54497f232af 100644 --- a/packages/react-dom/src/client/ReactDOM.js +++ b/packages/react-dom/src/client/ReactDOM.js @@ -113,14 +113,8 @@ setBatchingImplementation( batchedEventUpdates, ); -export type DOMInstance = Element & {_reactRootContainer: ?RootType, ...}; -export type DOMHTMLInstance = HTMLElement & { - _reactRootContainer: ?RootType, - ... -}; - export type DOMContainer = - | DOMInstance + | (Element & {_reactRootContainer: ?RootType, ...}) | (Document & {_reactRootContainer: ?RootType, ...}); function createPortal( diff --git a/packages/react-dom/src/client/ReactDOMHostConfig.js b/packages/react-dom/src/client/ReactDOMHostConfig.js index a1a780186e9d4..2e90978b31c03 100644 --- a/packages/react-dom/src/client/ReactDOMHostConfig.js +++ b/packages/react-dom/src/client/ReactDOMHostConfig.js @@ -45,7 +45,7 @@ import { } from '../shared/HTMLNodeType'; import dangerousStyleValue from '../shared/dangerousStyleValue'; -import type {DOMContainer, DOMInstance, DOMHTMLInstance} from './ReactDOM'; +import type {DOMContainer} from './ReactDOM'; import type { ReactDOMEventResponder, ReactDOMEventResponderInstance, @@ -89,7 +89,7 @@ export type EventTargetChildElement = { ... }; export type Container = DOMContainer; -export type Instance = DOMInstance; +export type Instance = Element; export type TextInstance = Text; export type SuspenseInstance = Comment & {_reactRetry?: () => void, ...}; export type HydratableInstance = Instance | TextInstance | SuspenseInstance; @@ -252,7 +252,7 @@ export function createInstance( } else { parentNamespace = ((hostContext: any): HostContextProd); } - const domElement = createElement( + const domElement: Instance = createElement( type, props, rootContainerInstance, @@ -260,7 +260,7 @@ export function createInstance( ); precacheFiberNode(internalInstanceHandle, domElement); updateFiberProps(domElement, props); - return ((domElement: any): DOMInstance); + return domElement; } export function appendInitialChild( @@ -398,7 +398,7 @@ export function commitUpdate( updateProperties(domElement, updatePayload, type, oldProps, newProps); } -export function resetTextContent(domElement: DOMInstance): void { +export function resetTextContent(domElement: Instance): void { setTextContent(domElement, ''); } @@ -411,15 +411,15 @@ export function commitTextUpdate( } export function appendChild( - parentInstance: DOMInstance, - child: DOMInstance | TextInstance, + parentInstance: Instance, + child: Instance | TextInstance, ): void { parentInstance.appendChild(child); } export function appendChildToContainer( - container: DOMInstance, - child: DOMInstance | TextInstance, + container: DOMContainer, + child: Instance | TextInstance, ): void { let parentNode; if (container.nodeType === COMMENT_NODE) { @@ -448,17 +448,17 @@ export function appendChildToContainer( } export function insertBefore( - parentInstance: DOMContainer, - child: DOMInstance | TextInstance, - beforeChild: DOMInstance | TextInstance | SuspenseInstance, + parentInstance: Instance, + child: Instance | TextInstance, + beforeChild: Instance | TextInstance | SuspenseInstance, ): void { parentInstance.insertBefore(child, beforeChild); } export function insertInContainerBefore( - container: DOMContainer, - child: DOMInstance | TextInstance, - beforeChild: DOMInstance | TextInstance | SuspenseInstance, + container: Container, + child: Instance | TextInstance, + beforeChild: Instance | TextInstance | SuspenseInstance, ): void { if (container.nodeType === COMMENT_NODE) { (container.parentNode: any).insertBefore(child, beforeChild); @@ -587,7 +587,7 @@ export function clearSuspenseBoundaryFromContainer( export function hideInstance(instance: Instance): void { // TODO: Does this work for all element types? What about MathML? Should we // pass host context to this method? - instance = ((instance: any): DOMHTMLInstance); + instance = ((instance: any): HTMLElement); const style = instance.style; if (typeof style.setProperty === 'function') { style.setProperty('display', 'none', 'important'); @@ -601,7 +601,7 @@ export function hideTextInstance(textInstance: TextInstance): void { } export function unhideInstance(instance: Instance, props: Props): void { - instance = ((instance: any): DOMHTMLInstance); + instance = ((instance: any): HTMLElement); const styleProp = props[STYLE]; const display = styleProp !== undefined && diff --git a/packages/react-native-renderer/src/ReactNativeHostConfig.js b/packages/react-native-renderer/src/ReactNativeHostConfig.js index 1584a2192ba05..e91c16a4909dc 100644 --- a/packages/react-native-renderer/src/ReactNativeHostConfig.js +++ b/packages/react-native-renderer/src/ReactNativeHostConfig.js @@ -289,12 +289,12 @@ export function appendChild( } export function appendChildToContainer( - parentInstance: Instance | Container, + parentInstance: Container, child: Instance | TextInstance, ): void { const childTag = typeof child === 'number' ? child : child._nativeTag; UIManager.setChildren( - ((parentInstance: any): Container), // containerTag + parentInstance, // containerTag [childTag], // reactTags ); } @@ -386,7 +386,7 @@ export function insertBefore( } export function insertInContainerBefore( - parentInstance: Instance | Container, + parentInstance: Container, child: Instance | TextInstance, beforeChild: Instance | TextInstance, ): void { diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.js b/packages/react-reconciler/src/ReactFiberCommitWork.js index 69a64dcd70eae..99a95dde732b8 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.js @@ -1099,42 +1099,69 @@ function commitPlacement(finishedWork: Fiber): void { const before = getHostSibling(finishedWork); // We only have the top Fiber that was inserted but we need to recurse down its // children to find all the terminal nodes. - insertOrAppendPlacementNode(finishedWork, before, parent, isContainer); + if (isContainer) { + insertOrAppendPlacementNodeIntoContainer(finishedWork, before, parent); + } else { + insertOrAppendPlacementNode(finishedWork, before, parent); + } +} + +function insertOrAppendPlacementNodeIntoContainer( + node: Fiber, + before: ?Instance, + parent: Container, +): void { + const {tag} = node; + const isHost = tag === HostComponent || tag === HostText; + if (isHost || (enableFundamentalAPI && tag === FundamentalComponent)) { + const stateNode = isHost ? node.stateNode : node.stateNode.instance; + if (before) { + insertInContainerBefore(parent, stateNode, before); + } else { + appendChildToContainer(parent, stateNode); + } + } else if (tag === HostPortal) { + // 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 { + const child = node.child; + if (child !== null) { + insertOrAppendPlacementNodeIntoContainer(child, before, parent); + let sibling = child.sibling; + while (sibling !== null) { + insertOrAppendPlacementNodeIntoContainer(sibling, before, parent); + sibling = sibling.sibling; + } + } + } } function insertOrAppendPlacementNode( node: Fiber, before: ?Instance, parent: Instance, - isContainer: boolean, ): void { - const isHost = node.tag === HostComponent || node.tag === HostText; - if (isHost || (enableFundamentalAPI && node.tag === FundamentalComponent)) { + const {tag} = node; + const isHost = tag === HostComponent || tag === HostText; + if (isHost || (enableFundamentalAPI && tag === FundamentalComponent)) { const stateNode = isHost ? node.stateNode : node.stateNode.instance; if (before) { - if (isContainer) { - insertInContainerBefore(parent, stateNode, before); - } else { - insertBefore(parent, stateNode, before); - } + insertBefore(parent, stateNode, before); } else { - if (isContainer) { - appendChildToContainer(parent, stateNode); - } else { - appendChild(parent, stateNode); - } + appendChild(parent, stateNode); } - } else if (node.tag === HostPortal) { + } else if (tag === HostPortal) { // 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 { const child = node.child; if (child !== null) { - insertOrAppendPlacementNode(child, before, parent, isContainer); + insertOrAppendPlacementNode(child, before, parent); let sibling = child.sibling; while (sibling !== null) { - insertOrAppendPlacementNode(sibling, before, parent, isContainer); + insertOrAppendPlacementNode(sibling, before, parent); sibling = sibling.sibling; } }