From e1e4887feb943e9aa32cff465f99139ad53b1fdf Mon Sep 17 00:00:00 2001 From: Pawel Kozlowski Date: Wed, 9 Jan 2019 16:33:25 +0100 Subject: [PATCH] refactor(ivy): merge canInsertNativeNode and getParentNative (#28011) Previously the canInsertNativeNode and getRenderParent functions had almost _exaclty_ the same logic. What was worse that getRenderParent was calling canInsertNativeNode thus executing the same, non-trivial logic twice. This commit merges canInsertNativeNode and getRenderParent into one function. Now getRenderParent will return a native parent or null if a node can't be inserted (content projection, root of a view that is not inserted etc.). PR Close #28011 --- .../core/src/render3/node_manipulation.ts | 130 ++++++------------ .../hello_world/bundle.golden_symbols.json | 12 -- .../bundling/todo/bundle.golden_symbols.json | 12 -- 3 files changed, 41 insertions(+), 113 deletions(-) diff --git a/packages/core/src/render3/node_manipulation.ts b/packages/core/src/render3/node_manipulation.ts index 0214e460d9..988351476b 100644 --- a/packages/core/src/render3/node_manipulation.ts +++ b/packages/core/src/render3/node_manipulation.ts @@ -18,23 +18,6 @@ import {findComponentView, getNativeByTNode, isLContainer, isRootView, readEleme const unusedValueToPlacateAjd = unused1 + unused2 + unused3 + unused4 + unused5; -/** Retrieves the native node (element or a comment) for the parent of a given node. */ -function getParentNative(tNode: TNode, currentView: LView): RElement|RComment|null { - return tNode.parent == null ? getHostNative(currentView) : - getNativeByTNode(tNode.parent, currentView); -} - -/** - * Gets the host element given a view. Will return null if the current view is an embedded view, - * which does not have a host element. - */ -function getHostNative(currentView: LView): RElement|null { - const hostTNode = currentView[HOST_NODE] as TElementNode; - return hostTNode && hostTNode.type !== TNodeType.View ? - (getNativeByTNode(hostTNode, currentView[PARENT] !) as RElement) : - null; -} - export function getLContainer(tNode: TViewNode, embeddedView: LView): LContainer|null { if (tNode.index === -1) { // This is a dynamically created view inside a dynamic container. @@ -485,51 +468,8 @@ function executeOnDestroys(view: LView): void { } } -function getRenderParent(tNode: TNode, currentView: LView): RElement|null { - if (canInsertNativeNode(tNode, currentView)) { - // If we are asked for a render parent of the root component we need to do low-level DOM - // operation as LTree doesn't exist above the topmost host node. We might need to find a render - // parent of the topmost host node if the root component injects ViewContainerRef. - if (isRootView(currentView)) { - return nativeParentNode(currentView[RENDERER], getNativeByTNode(tNode, currentView)); - } - - // skip over element and ICU containers as those are represented by a comment node and - // can't be used as a render parent - tNode = getHighestElementOrICUContainer(tNode); - - const hostTNode = currentView[HOST_NODE]; - return tNode.parent == null && hostTNode !.type === TNodeType.View ? - getContainerRenderParent(hostTNode as TViewNode, currentView) : - getParentNative(tNode, currentView) as RElement; - } - return null; -} - -function canInsertNativeChildOfElement(tNode: TElementNode): boolean { - // Component's content nodes are not inserted immediately - // because they will be projected, and so doing insert at this point would be wasteful. - // Since the projection would than move it to its final destination. - return !(tNode.flags & TNodeFlags.isComponent); -} - /** - * We might delay insertion of children for a given view if it is disconnected. - * This might happen for 2 main reasons: - * - view is not inserted into any container (view was created but not inserted yet) - * - view is inserted into a container but the container itself is not inserted into the DOM - * (container might be part of projection or child of a view that is not inserted yet). - * - * In other words we can insert children of a given view if this view was inserted into a container - * and - * the container itself has its render parent determined. - */ -function canInsertNativeChildOfView(viewTNode: TViewNode, view: LView): boolean { - return getContainerRenderParent(viewTNode, view) != null; -} - -/** - * Returns whether a native element can be inserted into the given parent. + * Returns a native element if a node can be inserted into the given parent. * * There are two reasons why we may not be able to insert a element immediately. * - Projection: When creating a child content element of a component, we have to skip the @@ -539,19 +479,15 @@ function canInsertNativeChildOfView(viewTNode: TViewNode, view: LView): boolean * parent container, which itself is disconnected. For example the parent container is part * of a View which has not be inserted or is mare for projection but has not been inserted * into destination. - * - - * - * @param tNode The tNode of the node that we want to insert. - * @param currentView Current LView being processed. - * @return boolean Whether the node should be inserted now (or delayed until later). */ -function canInsertNativeNode(tNode: TNode, currentView: LView): boolean { - // Nodes of the top-most view can be inserted eagerly +function getRenderParent(tNode: TNode, currentView: LView): RElement|null { + // Nodes of the top-most view can be inserted eagerly. if (isRootView(currentView)) { - return true; + return nativeParentNode(currentView[RENDERER], getNativeByTNode(tNode, currentView)); } + // Skip over element and ICU containers as those are represented by a comment node and + // can't be used as a render parent. const parent = getHighestElementOrICUContainer(tNode).parent; // If the parent is null, then we are inserting across views: either into an embedded view or a @@ -559,24 +495,44 @@ function canInsertNativeNode(tNode: TNode, currentView: LView): boolean { if (parent == null) { const hostTNode = currentView[HOST_NODE] !; if (hostTNode.type === TNodeType.View) { - // We are inserting a root element of an embedded view - this should only be possible if the - // embedded view in question is already inserted into a container and the container itself is - // inserted into the DOM. - return canInsertNativeChildOfView(hostTNode as TViewNode, currentView); + // We are inserting a root element of an embedded view We might delay insertion of children + // for a given view if it is disconnected. This might happen for 2 main reasons: + // - view is not inserted into any container(view was created but not inserted yet) + // - view is inserted into a container but the container itself is not inserted into the DOM + // (container might be part of projection or child of a view that is not inserted yet). + // In other words we can insert children of a given view if this view was inserted into a + // container and the container itself has its render parent determined. + return getContainerRenderParent(hostTNode as TViewNode, currentView); } else { // We are inserting a root element of the component view into the component host element and // it should always be eager. - return true; + return getHostNative(currentView); } } else { ngDevMode && assertNodeType(parent, TNodeType.Element); // We've got a parent which is an element in the current view. We just need to verify if the - // parent element is not a component (this would mean that the tNode represents a root content - // node of a component and its insertion should be delayed due to content projection). - return canInsertNativeChildOfElement(parent as TElementNode); + // parent element is not a component. Component's content nodes are not inserted immediately + // because they will be projected, and so doing insert at this point would be wasteful. + // Since the projection would than move it to its final destination. + if (!(parent.flags & TNodeFlags.isComponent)) { + return getNativeByTNode(parent, currentView) as RElement; + } else { + return null; + } } } +/** + * Gets the native host element for a given view. Will return null if the current view does not have + * a host element. + */ +function getHostNative(currentView: LView): RElement|null { + const hostTNode = currentView[HOST_NODE]; + return hostTNode && hostTNode.type === TNodeType.Element ? + (getNativeByTNode(hostTNode, currentView[PARENT] !) as RElement) : + null; +} + /** * Inserts a native node before another native node for a given parent using {@link Renderer3}. * This is a utility function that can be used when native nodes were determined - it abstracts an @@ -623,10 +579,10 @@ export function nativeNextSibling(renderer: Renderer3, node: RNode): RNode|null * @param currentView The current LView * @returns Whether or not the child was appended */ -export function appendChild(childEl: RNode, childTNode: TNode, currentView: LView): boolean { - if (canInsertNativeNode(childTNode, currentView)) { +export function appendChild(childEl: RNode, childTNode: TNode, currentView: LView): void { + const renderParent = getRenderParent(childTNode, currentView); + if (renderParent != null) { const renderer = currentView[RENDERER]; - const renderParent = getRenderParent(childTNode, currentView) !; const parentTNode: TNode = childTNode.parent || currentView[HOST_NODE] !; if (parentTNode.type === TNodeType.View) { @@ -644,9 +600,7 @@ export function appendChild(childEl: RNode, childTNode: TNode, currentView: LVie isProceduralRenderer(renderer) ? renderer.appendChild(renderParent, childEl) : renderParent.appendChild(childEl); } - return true; } - return false; } /** @@ -681,14 +635,12 @@ export function getBeforeNodeForView(index: number, views: LView[], containerNat * @param currentView The current LView * @returns Whether or not the child was removed */ -export function removeChild(childTNode: TNode, childEl: RNode, currentView: LView): boolean { - // We only remove the element if not in View or not projected. - if (canInsertNativeNode(childTNode, currentView)) { - const parentNative = getRenderParent(childTNode, currentView) !; +export function removeChild(childTNode: TNode, childEl: RNode, currentView: LView): void { + const parentNative = getRenderParent(childTNode, currentView); + // We only remove the element if it already has a render parent. + if (parentNative) { nativeRemoveChild(currentView[RENDERER], parentNative, childEl); - return true; } - return false; } /** diff --git a/packages/core/test/bundling/hello_world/bundle.golden_symbols.json b/packages/core/test/bundling/hello_world/bundle.golden_symbols.json index d86b3bb48e..c1a61277f8 100644 --- a/packages/core/test/bundling/hello_world/bundle.golden_symbols.json +++ b/packages/core/test/bundling/hello_world/bundle.golden_symbols.json @@ -155,15 +155,6 @@ { "name": "callHooks" }, - { - "name": "canInsertNativeChildOfElement" - }, - { - "name": "canInsertNativeChildOfView" - }, - { - "name": "canInsertNativeNode" - }, { "name": "checkNoChangesMode" }, @@ -299,9 +290,6 @@ { "name": "getParentInjectorViewOffset" }, - { - "name": "getParentNative" - }, { "name": "getPipeDef" }, diff --git a/packages/core/test/bundling/todo/bundle.golden_symbols.json b/packages/core/test/bundling/todo/bundle.golden_symbols.json index c2beaf4e8b..3cc4e94ef6 100644 --- a/packages/core/test/bundling/todo/bundle.golden_symbols.json +++ b/packages/core/test/bundling/todo/bundle.golden_symbols.json @@ -410,15 +410,6 @@ { "name": "callHooks" }, - { - "name": "canInsertNativeChildOfElement" - }, - { - "name": "canInsertNativeChildOfView" - }, - { - "name": "canInsertNativeNode" - }, { "name": "checkNoChanges" }, @@ -749,9 +740,6 @@ { "name": "getParentInjectorViewOffset" }, - { - "name": "getParentNative" - }, { "name": "getParentState" },