fix(ivy): only generate TViews once per embedded template (#23385)

PR Close #23385

packages/core/src/render3/di.ts

@@ -19,13 +19,14 @@ import {EmbeddedViewRef as viewEngine_EmbeddedViewRef, ViewRef as viewEngine_Vie
 import {Type} from '../type';
 
 import {assertGreaterThan, assertLessThan, assertNotNull} from './assert';
-import {addToViewTree, assertPreviousIsParent, createLContainer, createLNodeObject, getDirectiveInstance, getPreviousOrParentNode, getRenderer, isComponent, renderEmbeddedTemplate, resolveDirective} from './instructions';
+import {addToViewTree, assertPreviousIsParent, createLContainer, createLNodeObject, createTView, getDirectiveInstance, getPreviousOrParentNode, getRenderer, isComponent, renderEmbeddedTemplate, resolveDirective} from './instructions';
+import {LContainer} from './interfaces/container';
 import {ComponentTemplate, DirectiveDef, DirectiveDefList, PipeDefList} from './interfaces/definition';
 import {LInjector} from './interfaces/injector';
 import {LContainerNode, LElementNode, LNode, LNodeType, LViewNode, TNodeFlags} from './interfaces/node';
 import {QueryReadType} from './interfaces/query';
 import {Renderer3} from './interfaces/renderer';
-import {LView} from './interfaces/view';
+import {LView, TView} from './interfaces/view';
 import {assertNodeOfPossibleTypes, assertNodeType} from './node_assert';
 import {insertView, removeView} from './node_manipulation';
 import {notImplemented, stringify} from './util';
@@ -568,7 +569,6 @@ export function getOrCreateContainerRef(di: LInjector): viewEngine_ViewContainer
     const vcRefHost = di.node;
 
     ngDevMode && assertNodeOfPossibleTypes(vcRefHost, LNodeType.Container, LNodeType.Element);
-
     const lContainer = createLContainer(vcRefHost.parent !, vcRefHost.view);
     const lContainerNode: LContainerNode = createLNodeObject(
         LNodeType.Container, vcRefHost.view, vcRefHost.parent !, undefined, lContainer, null);
@@ -695,29 +695,35 @@ class ViewContainerRef implements viewEngine_ViewContainerRef {
  * @returns The TemplateRef instance to use
  */
 export function getOrCreateTemplateRef<T>(di: LInjector): viewEngine_TemplateRef<T> {
-  ngDevMode && assertNodeType(di.node, LNodeType.Container);
+  if (!di.templateRef) {
-  const data = (di.node as LContainerNode).data;
+    ngDevMode && assertNodeType(di.node, LNodeType.Container);
-  const tView = di.node.view.tView;
+    const hostNode = di.node as LContainerNode;
-  return di.templateRef || (di.templateRef = new TemplateRef<any>(
+    const hostTNode = hostNode.tNode !;
-                                getOrCreateElementRef(di), data.template !, getRenderer(),
+    const hostTView = hostNode.view.tView;
-                                tView.directiveRegistry, tView.pipeRegistry));
+    if (!hostTNode.tViews) {
+      hostTNode.tViews = createTView(hostTView.directiveRegistry, hostTView.pipeRegistry);
+    }
+    ngDevMode && assertNotNull(hostTNode.tViews, 'TView must be allocated');
+    di.templateRef = new TemplateRef<any>(
+        getOrCreateElementRef(di), hostTNode.tViews as TView, hostNode.data.template !,
+        getRenderer(), hostTView.directiveRegistry, hostTView.pipeRegistry);
+  }
+  return di.templateRef;
 }
 
 class TemplateRef<T> implements viewEngine_TemplateRef<T> {
   readonly elementRef: viewEngine_ElementRef;
-  private _template: ComponentTemplate<T>;
 
   constructor(
-      elementRef: viewEngine_ElementRef, template: ComponentTemplate<T>,
+      elementRef: viewEngine_ElementRef, private _tView: TView,
-      private _renderer: Renderer3, private _directives: DirectiveDefList|null,
+      private _template: ComponentTemplate<T>, private _renderer: Renderer3,
-      private _pipes: PipeDefList|null) {
+      private _directives: DirectiveDefList|null, private _pipes: PipeDefList|null) {
     this.elementRef = elementRef;
-    this._template = template;
   }
 
   createEmbeddedView(context: T): viewEngine_EmbeddedViewRef<T> {
     const viewNode = renderEmbeddedTemplate(
-        null, this._template, context, this._renderer, this._directives, this._pipes);
+        null, this._tView, this._template, context, this._renderer, this._directives, this._pipes);
     return addDestroyable(new EmbeddedViewRef(viewNode, this._template, context));
   }
 }

packages/core/src/render3/instructions.ts

@@ -9,7 +9,7 @@
 import './ng_dev_mode';
 
 import {assertEqual, assertLessThan, assertNotEqual, assertNotNull, assertNull, assertSame} from './assert';
-import {LContainer, TContainer} from './interfaces/container';
+import {LContainer} from './interfaces/container';
 import {LInjector} from './interfaces/injector';
 import {CssSelectorList, LProjection, NG_PROJECT_AS_ATTR_NAME} from './interfaces/projection';
 import {LQueries} from './interfaces/query';
@@ -468,9 +468,20 @@ export function renderTemplate<T>(
   return host;
 }
 
+/**
+ * Used for rendering embedded views (e.g. dynamically created views)
+ *
+ * Dynamically created views must store/retrieve their TViews differently from component views
+ * because their template functions are nested in the template functions of their hosts, creating
+ * closures. If their host template happens to be an embedded template in a loop (e.g. ngFor inside
+ * an ngFor), the nesting would mean we'd have multiple instances of the template function, so we
+ * can't store TViews in the template function itself (as we do for comps). Instead, we store the
+ * TView for dynamically created views on their host TNode, which only has one instance.
+ */
 export function renderEmbeddedTemplate<T>(
-    viewNode: LViewNode | null, template: ComponentTemplate<T>, context: T, renderer: Renderer3,
+    viewNode: LViewNode | null, tView: TView, template: ComponentTemplate<T>, context: T,
-    directives?: DirectiveDefList | null, pipes?: PipeDefList | null): LViewNode {
+    renderer: Renderer3, directives?: DirectiveDefList | null,
+    pipes?: PipeDefList | null): LViewNode {
   const _isParent = isParent;
   const _previousOrParentNode = previousOrParentNode;
   let oldView: LView;
@@ -480,7 +491,6 @@ export function renderEmbeddedTemplate<T>(
     previousOrParentNode = null !;
 
     if (viewNode == null) {
-      const tView = getOrCreateTView(template, directives || null, pipes || null);
       const lView = createLView(-1, renderer, tView, template, context, LViewFlags.CheckAlways);
 
       viewNode = createLNode(null, LNodeType.View, null, lView);
@@ -572,18 +582,27 @@ export function elementStart(
 
   if (attrs) setUpAttributes(native, attrs);
   appendChild(node.parent !, native, currentView);
-  createDirectivesAndLocals(index, name, attrs, localRefs, null);
+  createDirectivesAndLocals(index, name, attrs, localRefs, false);
   return native;
 }
 
+/**
+ * Creates directive instances and populates local refs.
+ *
+ * @param index Index of the current node (to create TNode)
+ * @param name Tag name of the current node
+ * @param attrs Attrs of the current node
+ * @param localRefs Local refs of the current node
+ * @param inlineViews Whether or not this node will create inline views
+ */
 function createDirectivesAndLocals(
     index: number, name: string | null, attrs: string[] | null | undefined,
-    localRefs: string[] | null | undefined, containerData: TView[] | null) {
+    localRefs: string[] | null | undefined, inlineViews: boolean) {
   const node = previousOrParentNode;
   if (firstTemplatePass) {
     ngDevMode && ngDevMode.firstTemplatePass++;
     ngDevMode && assertDataInRange(index - 1);
-    node.tNode = tData[index] = createTNode(name, attrs || null, containerData);
+    node.tNode = tData[index] = createTNode(name, attrs || null, inlineViews ? [] : null);
     cacheMatchingDirectivesForNode(node.tNode, currentView.tView, localRefs || null);
   } else {
     instantiateDirectivesDirectly();
@@ -751,6 +770,13 @@ function saveResolvedLocalsInData(): void {
 function getOrCreateTView(
     template: ComponentTemplate<any>, directives: DirectiveDefListOrFactory | null,
     pipes: PipeDefListOrFactory | null): TView {
+  // TODO(misko): reading `ngPrivateData` here is problematic for two reasons
+  // 1. It is a megamorphic call on each invocation.
+  // 2. For nested embedded views (ngFor inside ngFor) the template instance is per
+  //    outer template invocation, which means that no such property will exist
+  // Correct solution is to only put `ngPrivateData` on the Component template
+  // and not on embedded templates.
+
   return template.ngPrivateData ||
       (template.ngPrivateData = createTView(directives, pipes) as never);
 }
@@ -994,14 +1020,14 @@ export function elementProperty<T>(
 /**
  * Constructs a TNode object from the arguments.
  *
- * @param tagName
+ * @param tagName The tag name of the node
- * @param attrs
+ * @param attrs The attributes defined on this ndoe
- * @param data
+ * @param tViews Any TViews attached to this node
  * @param localNames A list of local names and their matching indices
  * @returns the TNode object
  */
 function createTNode(
-    tagName: string | null, attrs: string[] | null, data: TContainer | null): TNode {
+    tagName: string | null, attrs: string[] | null, tViews: TView[] | null): TNode {
   ngDevMode && ngDevMode.tNode++;
   return {
     flags: 0,
@@ -1011,7 +1037,7 @@ function createTNode(
     initialInputs: undefined,
     inputs: undefined,
     outputs: undefined,
-    data: data
+    tViews: tViews
   };
 }
 
@@ -1224,18 +1250,10 @@ export function textBinding<T>(index: number, value: T | NO_CHANGE): void {
   let existingNode = data[index] as LTextNode;
   ngDevMode && assertNotNull(existingNode, 'LNode should exist');
   ngDevMode && assertNotNull(existingNode.native, 'native element should exist');
-  if (existingNode.native) {
+  ngDevMode && ngDevMode.rendererSetText++;
-    // If DOM node exists and value changed, update textContent
+  value !== NO_CHANGE &&
-    ngDevMode && ngDevMode.rendererSetText++;
+      (isProceduralRenderer(renderer) ? renderer.setValue(existingNode.native, stringify(value)) :
-    value !== NO_CHANGE &&
+                                        existingNode.native.textContent = stringify(value));
-        (isProceduralRenderer(renderer) ? renderer.setValue(existingNode.native, stringify(value)) :
-                                          existingNode.native.textContent = stringify(value));
-  } else {
-    // Node was created but DOM node creation was delayed. Create and append now.
-    ngDevMode && ngDevMode.rendererCreateTextNode++;
-    existingNode.native = createTextNode(value, renderer);
-    insertChild(existingNode, currentView);
-  }
 }
 
 //////////////////////////
@@ -1451,7 +1469,7 @@ export function container(
   // Containers are added to the current view tree instead of their embedded views
   // because views can be removed and re-inserted.
   addToViewTree(currentView, node.data);
-  createDirectivesAndLocals(index, tagName || null, attrs, localRefs, []);
+  createDirectivesAndLocals(index, tagName || null, attrs, localRefs, template == null);
 
   isParent = false;
   ngDevMode && assertNodeType(previousOrParentNode, LNodeType.Container);
@@ -1516,9 +1534,12 @@ function refreshDynamicChildren() {
     if (current.dynamicViewCount !== 0 && (current as LContainer).views) {
       const container = current as LContainer;
       for (let i = 0; i < container.views.length; i++) {
-        const view = container.views[i];
+        const lViewNode = container.views[i];
         // The directives and pipes are not needed here as an existing view is only being refreshed.
-        renderEmbeddedTemplate(view, view.data.template !, view.data.context !, renderer);
+        const dynamicView = lViewNode.data;
+        ngDevMode && assertNotNull(dynamicView.tView, 'TView must be allocated');
+        renderEmbeddedTemplate(
+            lViewNode, dynamicView.tView, dynamicView.template !, dynamicView.context !, renderer);
       }
     }
   }
@@ -1588,23 +1609,24 @@ export function embeddedViewStart(viewBlockId: number): RenderFlags {
 /**
  * Initialize the TView (e.g. static data) for the active embedded view.
  *
- * Each embedded view needs to set the global tData variable to the static data for
+ * Each embedded view block must create or retrieve its own TView. Otherwise, the embedded view's
- * that view. Otherwise, the view's static data for a particular node would overwrite
+ * static data for a particular node would overwrite the static data for a node in the view above
- * the static data for a node in the view above it with the same index (since it's in the
+ * it with the same index (since it's in the same template).
- * same template).
  *
- * @param viewIndex The index of the TView in TContainer
+ * @param viewIndex The index of the TView in TNode.tViews
  * @param parent The parent container in which to look for the view's static data
  * @returns TView
  */
 function getOrCreateEmbeddedTView(viewIndex: number, parent: LContainerNode): TView {
   ngDevMode && assertNodeType(parent, LNodeType.Container);
-  const tContainer = (parent !.tNode as TContainerNode).data;
+  const containerTViews = (parent !.tNode as TContainerNode).tViews as TView[];
-  if (viewIndex >= tContainer.length || tContainer[viewIndex] == null) {
+  ngDevMode && assertNotNull(containerTViews, 'TView expected');
+  ngDevMode && assertEqual(Array.isArray(containerTViews), true, 'TViews should be in an array');
+  if (viewIndex >= containerTViews.length || containerTViews[viewIndex] == null) {
     const tView = currentView.tView;
-    tContainer[viewIndex] = createTView(tView.directiveRegistry, tView.pipeRegistry);
+    containerTViews[viewIndex] = createTView(tView.directiveRegistry, tView.pipeRegistry);
   }
-  return tContainer[viewIndex];
+  return containerTViews[viewIndex];
 }
 
 /** Marks the end of an embedded view. */

packages/core/src/render3/interfaces/container.ts

@@ -82,24 +82,6 @@ export interface LContainer {
   queries: LQueries|null;
 }
 
-/**
- * The static equivalent of LContainer, used in TContainerNode.
- *
- * The container needs to store static data for each of its embedded views
- * (TViews). Otherwise, nodes in embedded views with the same index as nodes
- * in their parent views will overwrite each other, as they are in
- * the same template.
- *
- * Each index in this array corresponds to the static data for a certain
- * view. So if you had V(0) and V(1) in a container, you might have:
- *
- * [
- *   [{tagName: 'div', attrs: ...}, null],     // V(0) TView
- *   [{tagName: 'button', attrs ...}, null]    // V(1) TView
- * ]
- */
-export type TContainer = TView[];
-
 // Note: This hack is necessary so we don't erroneously get a circular dependency
 // failure based on types.
 export const unusedValueExportToPlacateAjd = 1;

packages/core/src/render3/interfaces/node.ts

@@ -6,7 +6,7 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {LContainer, TContainer} from './container';
+import {LContainer} from './container';
 import {LInjector} from './injector';
 import {LProjection} from './projection';
 import {LQueries} from './query';
@@ -283,21 +283,33 @@ export interface TNode {
   outputs: PropertyAliases|null|undefined;
 
   /**
-   * The static data equivalent of LNode.data.
+   * The TView or TViews attached to this node.
    *
-   * If this TNode corresponds to an LContainerNode, the container will
+   * If this TNode corresponds to an LContainerNode with inline views, the container will
-   * need to store separate static data for each of its views (TContainer).
+   * need to store separate static data for each of its view blocks (TView[]). Otherwise,
+   * nodes in inline views with the same index as nodes in their parent views will overwrite
+   * each other, as they are in the same template.
    *
-   * If this TNode corresponds to an LElementNode, data will be null.
+   * Each index in this array corresponds to the static data for a certain
+   * view. So if you had V(0) and V(1) in a container, you might have:
+   *
+   * [
+   *   [{tagName: 'div', attrs: ...}, null],     // V(0) TView
+   *   [{tagName: 'button', attrs ...}, null]    // V(1) TView
+   *
+   * If this TNode corresponds to an LContainerNode with a template (e.g. structural
+   * directive), the template's TView will be stored here.
+   *
+   * If this TNode corresponds to an LElementNode, tViews will be null .
    */
-  data: TContainer|null;
+  tViews: TView|TView[]|null;
 }
 
 /** Static data for an LElementNode  */
-export interface TElementNode extends TNode { data: null; }
+export interface TElementNode extends TNode { tViews: null; }
 
 /** Static data for an LContainerNode */
-export interface TContainerNode extends TNode { data: TContainer; }
+export interface TContainerNode extends TNode { tViews: TView|TView[]|null; }
 
 /**
  * This mapping is necessary so we can set input properties and output listeners

packages/core/test/render3/instructions_spec.ts

@@ -102,18 +102,18 @@ describe('instructions', () => {
       elementStart(0, 'div', ['style', 'height: 10px']);
       elementEnd();
     }
-    const fixture = new TemplateFixture(createDivWithStyle);
 
     it('should add style', () => {
+      const fixture = new TemplateFixture(createDivWithStyle);
       fixture.update(() => elementStyle(0, {'background-color': 'red'}));
       expect(fixture.html).toEqual('<div style="height: 10px; background-color: red;"></div>');
     });
   });
 
   describe('elementClass', () => {
-    const fixture = new TemplateFixture(createDiv);
 
     it('should add class', () => {
+      const fixture = new TemplateFixture(createDiv);
       fixture.update(() => elementClass(0, 'multiple classes'));
       expect(fixture.html).toEqual('<div class="multiple classes"></div>');
     });
@@ -132,34 +132,34 @@ describe('instructions', () => {
 
         static ngComponentDef = defineComponent({
           type: NestedLoops,
-          selectors: [['todo-app']],
+          selectors: [['nested-loops']],
           factory: function ToDoAppComponent_Factory() { return new NestedLoops(); },
           template: function ToDoAppComponent_Template(rf: RenderFlags, ctx: NestedLoops) {
-            if (rf & 1) {
+            if (rf & RenderFlags.Create) {
               container(0, ToDoAppComponent_NgForOf_Template_0, null, _c0);
             }
-            if (rf & 2) {
+            if (rf & RenderFlags.Update) {
               elementProperty(0, 'ngForOf', bind(ctx.rows));
             }
             function ToDoAppComponent_NgForOf_Template_0(
                 rf: RenderFlags, ctx0: NgForOfContext<any>) {
-              if (rf & 1) {
+              if (rf & RenderFlags.Create) {
                 elementStart(0, 'ul');
                 container(1, ToDoAppComponent_NgForOf_NgForOf_Template_1, null, _c0);
                 elementEnd();
               }
-              if (rf & 2) {
+              if (rf & RenderFlags.Update) {
                 const row_r2 = ctx0.$implicit;
                 elementProperty(1, 'ngForOf', bind(row_r2));
               }
               function ToDoAppComponent_NgForOf_NgForOf_Template_1(
                   rf: RenderFlags, ctx1: NgForOfContext<any>) {
-                if (rf & 1) {
+                if (rf & RenderFlags.Create) {
                   elementStart(0, 'li');
                   text(1);
                   elementEnd();
                 }
-                if (rf & 2) {
+                if (rf & RenderFlags.Update) {
                   const col_r3 = ctx1.$implicit;
                   textBinding(1, interpolation1('', col_r3, ''));
                 }
@@ -171,8 +171,8 @@ describe('instructions', () => {
       }
       const fixture = new ComponentFixture(NestedLoops);
       expect(ngDevMode).toHaveProperties({
-        // Expect: host view + component + *ngForRow + *ngForCol
+        // Expect: fixture view/Host view + component + ngForRow + ngForCol
-        tView: 7,  // should be: 4,
+        tView: 4,  // should be: 4,
       });
 
     });

packages/core/test/render3/integration_spec.ts

@@ -830,7 +830,7 @@ describe('render3 integration test', () => {
 
       const oldTemplateData = (Template as any).ngPrivateData;
       const oldContainerData = (oldTemplateData as any).data[0];
-      const oldElementData = oldContainerData.data[0][0];
+      const oldElementData = oldContainerData.tViews[0][0];
       expect(oldContainerData).not.toBeNull();
       expect(oldElementData).not.toBeNull();
 
@@ -839,7 +839,7 @@ describe('render3 integration test', () => {
 
       const newTemplateData = (Template as any).ngPrivateData;
       const newContainerData = (oldTemplateData as any).data[0];
-      const newElementData = oldContainerData.data[0][0];
+      const newElementData = oldContainerData.tViews[0][0];
       expect(newTemplateData === oldTemplateData).toBe(true);
       expect(newContainerData === oldContainerData).toBe(true);
       expect(newElementData === oldElementData).toBe(true);

packages/core/test/render3/node_selector_matcher_spec.ts

@@ -19,7 +19,7 @@ function testLStaticData(tagName: string, attrs: string[] | null): TNode {
     initialInputs: undefined,
     inputs: undefined,
     outputs: undefined,
-    data: null,
+    tViews: null,
   };
 }
 

packages/core/test/render3/view_container_ref_spec.ts

@@ -147,30 +147,24 @@ describe('ViewContainerRef', () => {
           const tplRef = getOrCreateTemplateRef(getOrCreateNodeInjectorForNode(load(0)));
           elementProperty(0, 'tplRef', bind(tplRef));
           containerRefreshStart(0);
-          let rf1 = embeddedViewStart(1);
-          if (rf1 & RenderFlags.Create) {
-            elementStart(0, 'header');
-            elementEnd();
-          }
-          embeddedViewEnd();
           containerRefreshEnd();
         }
 
         const fixture = new TemplateFixture(createTemplate, updateTemplate, [DirectiveWithVCRef]);
-        expect(fixture.html).toEqual('<header></header><footer></footer>');
+        expect(fixture.html).toEqual('<footer></footer>');
 
         createView('A');
         fixture.update();
-        expect(fixture.html).toEqual('<header></header>A<footer></footer>');
+        expect(fixture.html).toEqual('A<footer></footer>');
 
         createView('B');
         createView('C');
         fixture.update();
-        expect(fixture.html).toEqual('<header></header>ABC<footer></footer>');
+        expect(fixture.html).toEqual('ABC<footer></footer>');
 
         createView('Y', 0);
         fixture.update();
-        expect(fixture.html).toEqual('<header></header>YABC<footer></footer>');
+        expect(fixture.html).toEqual('YABC<footer></footer>');
 
         expect(() => { createView('Z', -1); }).toThrow();
         expect(() => { createView('Z', 5); }).toThrow();