From c5cfc3a1b652dd748a774d7f1dbc4ca4e8524b15 Mon Sep 17 00:00:00 2001 From: Kara Erickson Date: Thu, 26 Apr 2018 10:44:49 -0700 Subject: [PATCH] fix(ivy): only generate TViews once per embedded template (#23385) PR Close #23385 --- packages/core/src/render3/di.ts | 36 ++++--- packages/core/src/render3/instructions.ts | 94 ++++++++++++------- .../core/src/render3/interfaces/container.ts | 18 ---- packages/core/src/render3/interfaces/node.ts | 28 ++++-- .../core/test/render3/instructions_spec.ts | 22 ++--- .../core/test/render3/integration_spec.ts | 4 +- .../render3/node_selector_matcher_spec.ts | 2 +- .../test/render3/view_container_ref_spec.ts | 14 +-- 8 files changed, 117 insertions(+), 101 deletions(-) diff --git a/packages/core/src/render3/di.ts b/packages/core/src/render3/di.ts index c82de9358f..a5b98c2c7c 100644 --- a/packages/core/src/render3/di.ts +++ b/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(di: LInjector): viewEngine_TemplateRef { - ngDevMode && assertNodeType(di.node, LNodeType.Container); - const data = (di.node as LContainerNode).data; - const tView = di.node.view.tView; - return di.templateRef || (di.templateRef = new TemplateRef( - getOrCreateElementRef(di), data.template !, getRenderer(), - tView.directiveRegistry, tView.pipeRegistry)); + if (!di.templateRef) { + ngDevMode && assertNodeType(di.node, LNodeType.Container); + const hostNode = di.node as LContainerNode; + const hostTNode = hostNode.tNode !; + const hostTView = hostNode.view.tView; + if (!hostTNode.tViews) { + hostTNode.tViews = createTView(hostTView.directiveRegistry, hostTView.pipeRegistry); + } + ngDevMode && assertNotNull(hostTNode.tViews, 'TView must be allocated'); + di.templateRef = new TemplateRef( + getOrCreateElementRef(di), hostTNode.tViews as TView, hostNode.data.template !, + getRenderer(), hostTView.directiveRegistry, hostTView.pipeRegistry); + } + return di.templateRef; } class TemplateRef implements viewEngine_TemplateRef { readonly elementRef: viewEngine_ElementRef; - private _template: ComponentTemplate; constructor( - elementRef: viewEngine_ElementRef, template: ComponentTemplate, - private _renderer: Renderer3, private _directives: DirectiveDefList|null, - private _pipes: PipeDefList|null) { + elementRef: viewEngine_ElementRef, private _tView: TView, + private _template: ComponentTemplate, private _renderer: Renderer3, + private _directives: DirectiveDefList|null, private _pipes: PipeDefList|null) { this.elementRef = elementRef; - this._template = template; } createEmbeddedView(context: T): viewEngine_EmbeddedViewRef { 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)); } } diff --git a/packages/core/src/render3/instructions.ts b/packages/core/src/render3/instructions.ts index 62d5a3fbfa..86b4ea07ea 100644 --- a/packages/core/src/render3/instructions.ts +++ b/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( 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( - viewNode: LViewNode | null, template: ComponentTemplate, context: T, renderer: Renderer3, - directives?: DirectiveDefList | null, pipes?: PipeDefList | null): LViewNode { + viewNode: LViewNode | null, tView: TView, template: ComponentTemplate, context: T, + 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( 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, 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( /** * Constructs a TNode object from the arguments. * - * @param tagName - * @param attrs - * @param data + * @param tagName The tag name of the node + * @param attrs The attributes defined on this ndoe + * @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(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) { - // If DOM node exists and value changed, update textContent - ngDevMode && ngDevMode.rendererSetText++; - value !== NO_CHANGE && - (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); - } + ngDevMode && ngDevMode.rendererSetText++; + value !== NO_CHANGE && + (isProceduralRenderer(renderer) ? renderer.setValue(existingNode.native, stringify(value)) : + existingNode.native.textContent = stringify(value)); } ////////////////////////// @@ -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 - * that view. Otherwise, the view's static data for a particular node would overwrite - * the static data for a node in the view above it with the same index (since it's in the - * same template). + * Each embedded view block must create or retrieve its own TView. Otherwise, the embedded view's + * static data for a particular node would overwrite the static data for a node in the view above + * it with the same index (since it's in the 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; - if (viewIndex >= tContainer.length || tContainer[viewIndex] == null) { + const containerTViews = (parent !.tNode as TContainerNode).tViews as TView[]; + 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. */ diff --git a/packages/core/src/render3/interfaces/container.ts b/packages/core/src/render3/interfaces/container.ts index 46e0a54966..be4fedb11b 100644 --- a/packages/core/src/render3/interfaces/container.ts +++ b/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; diff --git a/packages/core/src/render3/interfaces/node.ts b/packages/core/src/render3/interfaces/node.ts index 691e492095..0ec9adeb2f 100644 --- a/packages/core/src/render3/interfaces/node.ts +++ b/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 - * need to store separate static data for each of its views (TContainer). + * If this TNode corresponds to an LContainerNode with inline views, the container will + * 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 diff --git a/packages/core/test/render3/instructions_spec.ts b/packages/core/test/render3/instructions_spec.ts index 1bba3b2cc5..224b17c436 100644 --- a/packages/core/test/render3/instructions_spec.ts +++ b/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('
'); }); }); 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('
'); }); @@ -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) { - 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) { - 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 - tView: 7, // should be: 4, + // Expect: fixture view/Host view + component + ngForRow + ngForCol + tView: 4, // should be: 4, }); }); diff --git a/packages/core/test/render3/integration_spec.ts b/packages/core/test/render3/integration_spec.ts index f95229b3c1..d1ae1f67a5 100644 --- a/packages/core/test/render3/integration_spec.ts +++ b/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); diff --git a/packages/core/test/render3/node_selector_matcher_spec.ts b/packages/core/test/render3/node_selector_matcher_spec.ts index d04a9adaec..e47cc4aab8 100644 --- a/packages/core/test/render3/node_selector_matcher_spec.ts +++ b/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, }; } diff --git a/packages/core/test/render3/view_container_ref_spec.ts b/packages/core/test/render3/view_container_ref_spec.ts index 98b3781da7..91563976de 100644 --- a/packages/core/test/render3/view_container_ref_spec.ts +++ b/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('
'); + expect(fixture.html).toEqual('
'); createView('A'); fixture.update(); - expect(fixture.html).toEqual('
A
'); + expect(fixture.html).toEqual('A
'); createView('B'); createView('C'); fixture.update(); - expect(fixture.html).toEqual('
ABC
'); + expect(fixture.html).toEqual('ABC
'); createView('Y', 0); fixture.update(); - expect(fixture.html).toEqual('
YABC
'); + expect(fixture.html).toEqual('YABC'); expect(() => { createView('Z', -1); }).toThrow(); expect(() => { createView('Z', 5); }).toThrow();