From ab69f12e2c96ab608ec0ee204b184e1748f4ca1c Mon Sep 17 00:00:00 2001 From: David-Emmanuel Divernois Date: Fri, 26 Jan 2018 11:36:31 +0100 Subject: [PATCH] refactor(ivy): code review changes (#21638) PR Close #21638 --- packages/core/src/render3/di.ts | 5 +- packages/core/src/render3/instructions.ts | 72 +++++++--------- .../core/src/render3/interfaces/container.ts | 10 --- packages/core/src/render3/interfaces/node.ts | 14 ++- .../core/src/render3/interfaces/projection.ts | 4 +- .../core/src/render3/node_manipulation.ts | 86 +++++++++++-------- 6 files changed, 98 insertions(+), 93 deletions(-) diff --git a/packages/core/src/render3/di.ts b/packages/core/src/render3/di.ts index 0fbf633fca..a0e20cdeb4 100644 --- a/packages/core/src/render3/di.ts +++ b/packages/core/src/render3/di.ts @@ -392,7 +392,10 @@ export class ReadFromInjectorFn { * @returns The ElementRef instance to use */ export function getOrCreateElementRef(di: LInjector): viewEngine_ElementRef { - return di.elementRef || (di.elementRef = new ElementRef(di.node.native)); + return di.elementRef || + (di.elementRef = new ElementRef( + ((di.node.flags & LNodeFlags.TYPE_MASK) === LNodeFlags.Container) ? null : + di.node.native)); } export const QUERY_READ_TEMPLATE_REF = >>( diff --git a/packages/core/src/render3/instructions.ts b/packages/core/src/render3/instructions.ts index 69e28a56ac..a156f6fcce 100644 --- a/packages/core/src/render3/instructions.ts +++ b/packages/core/src/render3/instructions.ts @@ -16,7 +16,7 @@ import {LView, LifecycleStage, TData, TView} from './interfaces/view'; import {LContainerNode, LElementNode, LNode, LNodeFlags, LProjectionNode, LTextNode, LViewNode, TNode, TContainerNode, InitialInputData, InitialInputs, PropertyAliases, PropertyAliasValue,} from './interfaces/node'; import {assertNodeType, assertNodeOfPossibleTypes} from './node_assert'; -import {appendChild, insertChild, insertView, processProjectedNode, removeView} from './node_manipulation'; +import {appendChild, insertChild, insertView, appendProjectedNode, removeView, canInsertNativeNode} from './node_manipulation'; import {isNodeMatchingSelector} from './node_selector_matcher'; import {ComponentDef, ComponentTemplate, ComponentType, DirectiveDef, DirectiveType} from './interfaces/definition'; import {RElement, RText, Renderer3, RendererFactory3, ProceduralRenderer3, ObjectOrientedRenderer3, RendererStyleFlags3} from './interfaces/renderer'; @@ -194,14 +194,15 @@ export function createLNode( export function createLNode( index: null, type: LNodeFlags.View, native: null, lView: LView): LViewNode; export function createLNode( - index: number, type: LNodeFlags.Container, native: null, + index: number, type: LNodeFlags.Container, native: undefined, lContainer: LContainer): LContainerNode; export function createLNode( index: number, type: LNodeFlags.Projection, native: null, lProjection: LProjection): LProjectionNode; export function createLNode( - index: number | null, type: LNodeFlags, native: RText | RElement | null, state?: null | LView | - LContainer | LProjection): LElementNode<extNode&LViewNode&LContainerNode&LProjectionNode { + index: number | null, type: LNodeFlags, native: RText | RElement | null | undefined, + state?: null | LView | LContainer | LProjection): LElementNode<extNode&LViewNode& + LContainerNode&LProjectionNode { const parent = isParent ? previousOrParentNode : previousOrParentNode && previousOrParentNode.parent as LNode; let query = (isParent ? currentQuery : previousOrParentNode && previousOrParentNode.query) || @@ -986,35 +987,23 @@ export function container( tagName?: string, attrs?: string[], localRefs?: string[] | null): void { ngDevMode && assertEqual(currentView.bindingStartIndex, null, 'bindingStartIndex'); - // If the direct parent of the container is a view, its views (including its comment) - // will need to be added through insertView() when its parent view is being inserted. - // For now, it is marked "headless" so we know to append its views later. - let renderParent: LElementNode|null = null; const currentParent = isParent ? previousOrParentNode : previousOrParentNode.parent !; ngDevMode && assertNotEqual(currentParent, null, 'currentParent'); - if ((currentParent.flags & LNodeFlags.TYPE_MASK) === LNodeFlags.Element && - (currentParent.view !== - currentView /* Crossing View Boundaries, it is Component, but add Element of View */ - || currentParent.data === null /* Regular Element. */)) { - // we are adding to an Element which is either: - // - Not a component (will not be re-projected, just added) - // - View of the Component - renderParent = currentParent as LElementNode; - } - const lContainer = { views: [], - nextIndex: 0, renderParent, + nextIndex: 0, + // If the direct parent of the container is a view, its views will need to be added + // through insertView() when its parent view is being inserted: + renderParent: canInsertNativeNode(currentParent, currentView) ? currentParent : null, template: template == null ? null : template, next: null, parent: currentView, dynamicViewCount: 0, - query: null, - nextNative: undefined + query: null }; - const node = createLNode(index, LNodeFlags.Container, null, lContainer); + const node = createLNode(index, LNodeFlags.Container, undefined, lContainer); if (node.tNode == null) { // TODO(misko): implement queryName caching @@ -1048,11 +1037,10 @@ export function containerRefreshStart(index: number): void { previousOrParentNode = data[index] as LNode; ngDevMode && assertNodeType(previousOrParentNode, LNodeFlags.Container); isParent = true; - const container = previousOrParentNode as LContainerNode; - container.data.nextIndex = 0; - ngDevMode && - assertEqual( - container.data.nextNative === undefined, true, 'container.data.nextNative === undefined'); + (previousOrParentNode as LContainerNode).data.nextIndex = 0; + ngDevMode && assertEqual( + (previousOrParentNode as LContainerNode).native === undefined, true, + 'previousOrParentNode.native === undefined'); // We need to execute init hooks here so ngOnInit hooks are called in top level views // before they are called in embedded views (for backwards compatibility). @@ -1074,7 +1062,7 @@ export function containerRefreshEnd(): void { } ngDevMode && assertNodeType(previousOrParentNode, LNodeFlags.Container); const container = previousOrParentNode as LContainerNode; - container.data.nextNative = undefined; + container.native = undefined; ngDevMode && assertNodeType(container, LNodeFlags.Container); const nextIndex = container.data.nextIndex; while (nextIndex < container.data.views.length) { @@ -1281,12 +1269,12 @@ function appendToProjectionNode( return; } const projectionNodeData = projectionNode.data; - if (projectionNodeData.last) { - projectionNodeData.last.pNextOrParent = appendedFirst; + if (projectionNodeData.tail) { + projectionNodeData.tail.pNextOrParent = appendedFirst; } else { - projectionNodeData.first = appendedFirst; + projectionNodeData.head = appendedFirst; } - projectionNodeData.last = appendedLast; + projectionNodeData.tail = appendedLast; appendedLast.pNextOrParent = projectionNode; } @@ -1299,7 +1287,7 @@ function appendToProjectionNode( * @param selectorIndex - 0 means without any selector */ export function projection(nodeIndex: number, localIndex: number, selectorIndex: number = 0): void { - const node = createLNode(nodeIndex, LNodeFlags.Projection, null, {first: null, last: null}); + const node = createLNode(nodeIndex, LNodeFlags.Projection, null, {head: null, tail: null}); isParent = false; // self closing const currentParent = node.parent; @@ -1315,7 +1303,7 @@ export function projection(nodeIndex: number, localIndex: number, selectorIndex: const nodeToProject = nodesForSelector[i]; if ((nodeToProject.flags & LNodeFlags.TYPE_MASK) === LNodeFlags.Projection) { const previouslyProjected = (nodeToProject as LProjectionNode).data; - appendToProjectionNode(node, previouslyProjected.first, previouslyProjected.last); + appendToProjectionNode(node, previouslyProjected.head, previouslyProjected.tail); } else { appendToProjectionNode( node, nodeToProject as LTextNode | LElementNode | LContainerNode, @@ -1323,13 +1311,15 @@ export function projection(nodeIndex: number, localIndex: number, selectorIndex: } } - // process each node in the list of projected nodes: - let nodeToProject: LNode|null = node.data.first; - const lastNodeToProject = node.data.last; - while (nodeToProject) { - processProjectedNode( - nodeToProject as LTextNode | LElementNode | LContainerNode, currentParent, currentView); - nodeToProject = nodeToProject === lastNodeToProject ? null : nodeToProject.pNextOrParent; + if (canInsertNativeNode(currentParent, currentView)) { + // process each node in the list of projected nodes: + let nodeToProject: LNode|null = node.data.head; + const lastNodeToProject = node.data.tail; + while (nodeToProject) { + appendProjectedNode( + nodeToProject as LTextNode | LElementNode | LContainerNode, currentParent, currentView); + nodeToProject = nodeToProject === lastNodeToProject ? null : nodeToProject.pNextOrParent; + } } } diff --git a/packages/core/src/render3/interfaces/container.ts b/packages/core/src/render3/interfaces/container.ts index e10722e6c3..1a5edd8398 100644 --- a/packages/core/src/render3/interfaces/container.ts +++ b/packages/core/src/render3/interfaces/container.ts @@ -9,7 +9,6 @@ import {ComponentTemplate} from './definition'; import {LElementNode, LViewNode} from './node'; import {LQuery} from './query'; -import {RNode} from './renderer'; import {LView, TView} from './view'; @@ -81,15 +80,6 @@ export interface LContainer { * this container are reported to queries referenced here. */ query: LQuery|null; - - /* - * Caches the reference of the first native node following this container in the same native - * parent. - * This is reset to undefined in containerRefreshEnd. - * When it is undefined, it means the value has not been computed yet. - * Otherwise, it contains the result of findBeforeNode(container, null). - */ - nextNative: RNode|null|undefined; } /** diff --git a/packages/core/src/render3/interfaces/node.ts b/packages/core/src/render3/interfaces/node.ts index a0a40906c3..43526ca8e6 100644 --- a/packages/core/src/render3/interfaces/node.ts +++ b/packages/core/src/render3/interfaces/node.ts @@ -11,10 +11,11 @@ import {DirectiveDef} from './definition'; import {LInjector} from './injector'; import {LProjection} from './projection'; import {LQuery} from './query'; -import {RElement, RText} from './renderer'; +import {RElement, RNode, RText} from './renderer'; import {LView, TData, TView} from './view'; + /** * LNodeFlags corresponds to the LNode.flags property. It contains information * on how to map a particular set of bits in LNode.flags to the node type, directive @@ -75,7 +76,7 @@ export interface LNode { * - append children to their element parents in the DOM (e.g. `parent.native.appendChild(...)`) * - retrieve the sibling elements of text nodes whose creation / insertion has been delayed */ - readonly native: RElement|RText|null; + readonly native: RElement|RText|null|undefined; /** * We need a reference to a node's parent so we can append the node to its parent's native @@ -177,7 +178,14 @@ export interface LViewNode extends LNode { /** Abstract node container which contains other views. */ export interface LContainerNode extends LNode { - readonly native: null; + /* + * Caches the reference of the first native node following this container in the same native + * parent. + * This is reset to undefined in containerRefreshEnd. + * When it is undefined, it means the value has not been computed yet. + * Otherwise, it contains the result of findBeforeNode(container, null). + */ + native: RElement|RText|null|undefined; readonly data: LContainer; child: null; next: LContainerNode|LElementNode|LTextNode|LProjectionNode|null; diff --git a/packages/core/src/render3/interfaces/projection.ts b/packages/core/src/render3/interfaces/projection.ts index a34e45b601..8906d23cdc 100644 --- a/packages/core/src/render3/interfaces/projection.ts +++ b/packages/core/src/render3/interfaces/projection.ts @@ -13,8 +13,8 @@ import {LContainerNode, LElementNode, LTextNode} from './node'; * It is a linked list (using the pNextOrParent property). */ export interface LProjection { - first: LElementNode|LTextNode|LContainerNode|null; - last: LElementNode|LTextNode|LContainerNode|null; + head: LElementNode|LTextNode|LContainerNode|null; + tail: LElementNode|LTextNode|LContainerNode|null; } /** diff --git a/packages/core/src/render3/node_manipulation.ts b/packages/core/src/render3/node_manipulation.ts index 20e30cf1c2..d276815e07 100644 --- a/packages/core/src/render3/node_manipulation.ts +++ b/packages/core/src/render3/node_manipulation.ts @@ -27,7 +27,7 @@ const unusedValueToPlacateAjd = unused1 + unused2 + unused3 + unused4 + unused5; * @returns Node before which the provided node should be inserted or null if the lookup was stopped * or if there is no native node after the given logical node in the same native parent. */ -function findBeforeNode(node: LNode | null, stopNode: LNode | null): RNode|null { +function findBeforeNode(node: LNode | null, stopNode: LNode | null): RElement|RText|null { let currentNode = node; while (currentNode && currentNode !== stopNode) { const currentNodeType = currentNode.flags && LNodeFlags.TYPE_MASK; @@ -77,8 +77,8 @@ function findBeforeNode(node: LNode | null, stopNode: LNode | null): RNode|null function getNextNode(node: LNode): LNode|null { const pNextOrParent = node.pNextOrParent; if (pNextOrParent) { - const type = pNextOrParent.flags & LNodeFlags.TYPE_MASK; - return type === LNodeFlags.Projection ? null : pNextOrParent; + return (pNextOrParent.flags & LNodeFlags.TYPE_MASK) === LNodeFlags.Projection ? null : + pNextOrParent; } else { return node.next; } @@ -94,7 +94,7 @@ function getNextNode(node: LNode): LNode|null { * @param rootNode The root node at which the lookup should stop. * @return The following node in the LNode tree. */ -function findFollowingNode(initialNode: LNode, rootNode: LNode): LNode|null { +function getNextOrParentSiblingNode(initialNode: LNode, rootNode: LNode): LNode|null { let node: LNode|null = initialNode; let nextNode = getNextNode(node); while (node && !nextNode) { @@ -113,23 +113,23 @@ function findFollowingNode(initialNode: LNode, rootNode: LNode): LNode|null { * @param node The node whose first DOM node must be found * @returns The first native node of the given logical node or null if there is none. */ -function findFirstNativeNode(rootNode: LNode): RNode|null { +function findFirstNativeNode(rootNode: LNode): RElement|RText|null { let node: LNode|null = rootNode; while (node) { const type = node.flags & LNodeFlags.TYPE_MASK; let nextNode: LNode|null = null; if (type === LNodeFlags.Element) { - return node.native; + return (node as LElementNode).native; } else if (type === LNodeFlags.Container) { const childContainerData: LContainer = (node as LContainerNode).data; nextNode = childContainerData.views.length ? childContainerData.views[0].child : null; } else if (type === LNodeFlags.Projection) { - nextNode = (node as LProjectionNode).data.first; + nextNode = (node as LProjectionNode).data.head; } else { nextNode = (node as LViewNode).child; } if (nextNode === null) { - node = findFollowingNode(node, rootNode); + node = getNextOrParentSiblingNode(node, rootNode); } else { node = nextNode; } @@ -185,12 +185,12 @@ export function addRemoveViewFromContainer( childContainerData.renderParent = parentNode; nextNode = childContainerData.views.length ? childContainerData.views[0].child : null; } else if (type === LNodeFlags.Projection) { - nextNode = (node as LProjectionNode).data.first; + nextNode = (node as LProjectionNode).data.head; } else { nextNode = (node as LViewNode).child; } if (nextNode === null) { - node = findFollowingNode(node, rootNode); + node = getNextOrParentSiblingNode(node, rootNode); } else { node = nextNode; } @@ -280,9 +280,9 @@ export function insertView( if (container.data.renderParent !== null) { let beforeNode = findBeforeNode(newView, container); if (!beforeNode) { - let containerNextNativeNode = container.data.nextNative; + let containerNextNativeNode = container.native; if (containerNextNativeNode === undefined) { - containerNextNativeNode = container.data.nextNative = findBeforeNode(container, null); + containerNextNativeNode = container.native = findBeforeNode(container, null); } beforeNode = containerNextNativeNode; } @@ -392,6 +392,37 @@ function executeOnDestroys(view: LView): void { } } +/** + * Returns whether a child native element should be inserted now in the given parent. + * + * If the parent is a view, the element will be appended as part of viewEnd(), so + * the element should not be appended now. Similarly, if the child is a content child + * of a parent component, the child will be appended to the right position later by + * the content projection system. + * + * @param parent The parent in which to insert the child + * @param currentView The current LView + * @return Whether the child element should be inserted now. + */ +export function canInsertNativeNode(parent: LNode, view: LView): boolean { + // Only add native child element to parent element if the parent element is regular Element. + // If parent is: + // - Regular element => add child + // - Component host element => + // - Current View, and parent view same => content => don't add -> parent component will + // re-project if needed. + // - Current View, and parent view different => view => add Child + // - View element => View's get added separately. + + return ( + (parent.flags & LNodeFlags.TYPE_MASK) === LNodeFlags.Element && + (parent.view !== view /* Crossing View Boundaries, it is Component, but add Element of View */ + || parent.data === null /* Regular Element. */)); + // we are adding to an Element which is either: + // - Not a component (will not be re-projected, just added) + // - View of the Component +} + /** * Appends the provided child element to the provided parent, if appropriate. * @@ -406,20 +437,8 @@ function executeOnDestroys(view: LView): void { * @returns Whether or not the child was appended */ export function appendChild(parent: LNode, child: RNode | null, currentView: LView): boolean { - // Only add native child element to parent element if the parent element is regular Element. - // If parent is: - // - Regular element => add child - // - Component host element => - // - Current View, and parent view same => content => don't add -> parent component will - // re-project if needed. - // - Current View, and parent view different => view => add Child - // - View element => View's get added separately. - if (child !== null && (parent.flags & LNodeFlags.TYPE_MASK) === LNodeFlags.Element && - (parent.view !== - currentView /* Crossing View Boundaries, it is Component, but add Element of View */ - || parent.data === null /* Regular Element. */)) { + if (child !== null && canInsertNativeNode(parent, currentView)) { // We only add element if not in View or not projected. - const renderer = currentView.renderer; (renderer as ProceduralRenderer3).listen ? (renderer as ProceduralRenderer3).appendChild !(parent.native !as RElement, child) : @@ -450,10 +469,7 @@ export function insertChild(node: LNode, currentView: LView): void { // re-project if needed. // - Current View, and parent view different => view => add Child // - View element => View's get added separately. - if ((parent.flags & LNodeFlags.TYPE_MASK) === LNodeFlags.Element && - (parent.view !== - currentView /* Crossing View Boundaries, its Component, but add Element of View */ - || parent.data === null /* Regular Element. */)) { + if (canInsertNativeNode(parent, currentView)) { // We only add element if not in View or not projected. let nativeSibling: RNode|null = findBeforeNode(node, null); @@ -467,19 +483,18 @@ export function insertChild(node: LNode, currentView: LView): void { /** * Appends a projected node to the DOM, or in the case of a projected container, - * appends the nodes from all of the container's active views to the DOM. Also stores the - * node in the given projectedNodes array. + * appends the nodes from all of the container's active views to the DOM. * * @param node The node to process * @param currentParent The last parent element to be processed * @param currentView Current LView */ -export function processProjectedNode( +export function appendProjectedNode( node: LElementNode | LTextNode | LContainerNode, currentParent: LViewNode | LElementNode, currentView: LView): void { - if ((node.flags & LNodeFlags.TYPE_MASK) === LNodeFlags.Container && - (currentParent.flags & LNodeFlags.TYPE_MASK) === LNodeFlags.Element && - (currentParent.data === null || currentParent.data === currentView)) { + if ((node.flags & LNodeFlags.TYPE_MASK) !== LNodeFlags.Container) { + appendChild(currentParent, (node as LElementNode | LTextNode).native, currentView); + } else if (canInsertNativeNode(currentParent, currentView)) { // The node we are adding is a Container and we are adding it to Element which // is not a component (no more re-projection). // Alternatively a container is projected at the root of a component's template @@ -492,5 +507,4 @@ export function processProjectedNode( addRemoveViewFromContainer(node as LContainerNode, views[i], true, null); } } - appendChild(currentParent, node.native, currentView); }