refactor(ivy): code review changes (#21638)

PR Close #21638

packages/core/src/render3/di.ts

@@ -392,7 +392,10 @@ export class ReadFromInjectorFn<T> {
  * @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 = <QueryReadType<viewEngine_TemplateRef<any>>>(

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 |
+    index: number | null, type: LNodeFlags, native: RText | RElement | null | undefined,
-        LContainer | LProjection): LElementNode&LTextNode&LViewNode&LContainerNode&LProjectionNode {
+    state?: null | LView | LContainer | LProjection): LElementNode&LTextNode&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 = <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,
+    query: null
-    nextNative: undefined
   };
 
-  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;
+  (previousOrParentNode as LContainerNode).data.nextIndex = 0;
-  container.data.nextIndex = 0;
+  ngDevMode && assertEqual(
-  ngDevMode &&
+                   (previousOrParentNode as LContainerNode).native === undefined, true,
-      assertEqual(
+                   'previousOrParentNode.native === undefined');
-          container.data.nextNative === undefined, true, 'container.data.nextNative === 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) {
+  if (projectionNodeData.tail) {
-    projectionNodeData.last.pNextOrParent = appendedFirst;
+    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 <ng-content> 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,14 +1311,16 @@ export function projection(nodeIndex: number, localIndex: number, selectorIndex:
     }
   }
 
+  if (canInsertNativeNode(currentParent, currentView)) {
     // process each node in the list of projected nodes:
-  let nodeToProject: LNode|null = node.data.first;
+    let nodeToProject: LNode|null = node.data.head;
-  const lastNodeToProject = node.data.last;
+    const lastNodeToProject = node.data.tail;
     while (nodeToProject) {
-    processProjectedNode(
+      appendProjectedNode(
           nodeToProject as LTextNode | LElementNode | LContainerNode, currentParent, currentView);
       nodeToProject = nodeToProject === lastNodeToProject ? null : nodeToProject.pNextOrParent;
     }
+  }
 }
 
 /**

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;
 }
 
 /**

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;

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;
+  head: LElementNode|LTextNode|LContainerNode|null;
-  last: LElementNode|LTextNode|LContainerNode|null;
+  tail: LElementNode|LTextNode|LContainerNode|null;
 }
 
 /**

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 (pNextOrParent.flags & LNodeFlags.TYPE_MASK) === LNodeFlags.Projection ? null :
-    return type === LNodeFlags.Projection ? null : pNextOrParent;
+                                                                                    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 (child !== null && canInsertNativeNode(parent, currentView)) {
-  // 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. */)) {
     // 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 &&
+  if (canInsertNativeNode(parent, currentView)) {
-      (parent.view !==
-           currentView /* Crossing View Boundaries, its Component, but add Element of View */
-       || parent.data === null /* Regular Element. */)) {
     // 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
+ * appends the nodes from all of the container's active views to the DOM.
- * node in the given projectedNodes array.
  *
  * @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 &&
+  if ((node.flags & LNodeFlags.TYPE_MASK) !== LNodeFlags.Container) {
-      (currentParent.flags & LNodeFlags.TYPE_MASK) === LNodeFlags.Element &&
+    appendChild(currentParent, (node as LElementNode | LTextNode).native, currentView);
-      (currentParent.data === null || currentParent.data === 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);
 }