From bafdad9083a47f8f3f9bdeccae96360c2110cd20 Mon Sep 17 00:00:00 2001 From: Kara Erickson Date: Thu, 15 Mar 2018 12:18:31 -0700 Subject: [PATCH] fix(ivy): cache local names and support multiple locals with same value (#22807) PR Close #22807 --- packages/core/src/render3/instructions.ts | 56 +++++++++-------- packages/core/test/render3/query_spec.ts | 73 ++++++++++++++++++++++- 2 files changed, 102 insertions(+), 27 deletions(-) diff --git a/packages/core/src/render3/instructions.ts b/packages/core/src/render3/instructions.ts index e7d76e8e44..73f2db8a8b 100644 --- a/packages/core/src/render3/instructions.ts +++ b/packages/core/src/render3/instructions.ts @@ -491,13 +491,11 @@ export function elementStart( // accessed through their containers because they may be removed / re-added later. node = createLNode(index, LNodeFlags.Element, native, componentView); - // TODO(misko): implement code which caches the local reference resolution - const queryName: string|null = hack_findQueryName(hostComponentDef, localRefs, ''); - if (node.tNode == null) { + const localNames: (string | number)[]|null = + findMatchingLocalNames(hostComponentDef, localRefs, isHostElement ? index + 1 : -1, ''); ngDevMode && assertDataInRange(index - 1); - node.tNode = tData[index] = - createTNode(name, attrs || null, null, hostComponentDef ? null : queryName); + node.tNode = tData[index] = createTNode(name, attrs || null, null, localNames); } if (attrs) setUpAttributes(native, attrs); @@ -508,7 +506,7 @@ export function elementStart( // is not guaranteed. Must be refactored to take it into account. const instance = hostComponentDef.n(); storeComponentIndex(index); - directiveCreate(++index, instance, hostComponentDef, queryName); + directiveCreate(++index, instance, hostComponentDef, null); initChangeDetectorIfExisting(node.nodeInjector, instance); } hack_declareDirectives(index, directiveTypes, localRefs); @@ -532,8 +530,8 @@ export function initChangeDetectorIfExisting(injector: LInjector | null, instanc } /** - * This function instantiates a directive with a correct queryName. It is a hack since we should - * compute the query value only once and store it with the template (rather than on each invocation) + * This function instantiates the given directives. It is a hack since it assumes the directives + * come in the correct order for DI. */ function hack_declareDirectives( index: number, directiveTypes: DirectiveType[] | null | undefined, @@ -546,30 +544,35 @@ function hack_declareDirectives( const directiveType = directiveTypes[i]; const directiveDef = currentView.tView.firstTemplatePass ? directiveType.ngDirectiveDef : tData[index] as DirectiveDef; - directiveCreate( - index, directiveDef.n(), directiveDef, hack_findQueryName(directiveDef, localRefs)); + const localNames = currentView.tView.firstTemplatePass ? + findMatchingLocalNames(directiveDef, localRefs, index) : + null; + + directiveCreate(index, directiveDef.n(), directiveDef, localNames); } } } /** - * This function returns the queryName for a directive. It is a hack since we should - * compute the query value only once and store it with the template (rather than on each invocation) + * Finds any local names that match the given directive's exportAs and returns them with directive + * index. If the directiveDef is null, it matches against the default '' value instead of + * exportAs. */ -function hack_findQueryName( - directiveDef: DirectiveDef| null, localRefs: string[] | null | undefined, - defaultExport?: string, ): string|null { +function findMatchingLocalNames( + directiveDef: DirectiveDef| null, localRefs: string[] | null | undefined, index: number, + defaultExport?: string): (string | number)[]|null { const exportAs = directiveDef && directiveDef.exportAs || defaultExport; + let matches: (string | number)[]|null = null; if (exportAs != null && localRefs) { for (let i = 0; i < localRefs.length; i = i + 2) { const local = localRefs[i]; const toExportAs = localRefs[i | 1]; if (toExportAs === exportAs || toExportAs === defaultExport) { - return local; + (matches || (matches = [])).push(local, index); } } } - return null; + return matches; } /** @@ -801,15 +804,16 @@ export function elementProperty( * @param tagName * @param attrs * @param data + * @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, - localName: string | null): TNode { + localNames: (string | number)[] | null): TNode { return { tagName: tagName, attrs: attrs, - localNames: localName ? [localName, -1] : null, + localNames: localNames, initialInputs: undefined, inputs: undefined, outputs: undefined, @@ -1043,10 +1047,11 @@ export function textBinding(index: number, value: T | NO_CHANGE): void { * be created or retrieved out of order. * @param directive The directive instance. * @param directiveDef DirectiveDef object which contains information about the template. - * @param queryName Name under which the query can retrieve the directive instance. + * @param localNames Names under which a query can retrieve the directive instance */ export function directiveCreate( - index: number, directive: T, directiveDef: DirectiveDef, queryName?: string | null): T { + index: number, directive: T, directiveDef: DirectiveDef, + localNames?: (string | number)[] | null): T { let instance; ngDevMode && assertNull(currentView.bindingStartIndex, 'directives should be created before any bindings'); @@ -1068,10 +1073,10 @@ export function directiveCreate( if (index >= tData.length) { tData[index] = directiveDef !; - if (queryName) { + if (localNames) { ngDevMode && assertNotNull(previousOrParentNode.tNode, 'previousOrParentNode.tNode'); const tNode = previousOrParentNode !.tNode !; - (tNode.localNames || (tNode.localNames = [])).push(queryName, index); + tNode.localNames = tNode.localNames ? tNode.localNames.concat(localNames) : localNames; } } @@ -1197,9 +1202,8 @@ export function container( const node = createLNode(index, LNodeFlags.Container, undefined, lContainer); if (node.tNode == null) { - // TODO(misko): implement queryName caching - const queryName: string|null = hack_findQueryName(null, localRefs, ''); - node.tNode = tData[index] = createTNode(tagName || null, attrs || null, [], queryName || null); + const localNames: (string | number)[]|null = findMatchingLocalNames(null, localRefs, -1, ''); + node.tNode = tData[index] = createTNode(tagName || null, attrs || null, [], localNames); } // Containers are added to the current view tree instead of their embedded views diff --git a/packages/core/test/render3/query_spec.ts b/packages/core/test/render3/query_spec.ts index d58dd4af04..34f8db3b23 100644 --- a/packages/core/test/render3/query_spec.ts +++ b/packages/core/test/render3/query_spec.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ import {QUERY_READ_CONTAINER_REF, QUERY_READ_ELEMENT_REF, QUERY_READ_FROM_NODE, QUERY_READ_TEMPLATE_REF} from '../../src/render3/di'; -import {QueryList, detectChanges, defineComponent} from '../../src/render3/index'; +import {QueryList, defineComponent, detectChanges} from '../../src/render3/index'; import {container, containerRefreshEnd, containerRefreshStart, elementEnd, elementStart, embeddedViewEnd, embeddedViewStart, load} from '../../src/render3/instructions'; import {query, queryRefresh} from '../../src/render3/query'; @@ -191,6 +191,42 @@ describe('query', () => { expect(qList.first.nativeElement).toEqual(elToQuery); }); + it('should query multiple locals on the same element', () => { + let elToQuery; + + /** + *
+ *
+ * class Cmpt { + * @ViewChildren('foo') fooQuery; + * @ViewChildren('bar') barQuery; + * } + */ + const Cmpt = createComponent('cmpt', function(ctx: any, cm: boolean) { + let tmp: any; + if (cm) { + query(0, ['foo'], false, QUERY_READ_FROM_NODE); + query(1, ['bar'], false, QUERY_READ_FROM_NODE); + elToQuery = elementStart(2, 'div', null, null, ['foo', '', 'bar', '']); + elementEnd(); + elementStart(3, 'div'); + elementEnd(); + } + queryRefresh(tmp = load>(0)) && (ctx.fooQuery = tmp as QueryList); + queryRefresh(tmp = load>(1)) && (ctx.barQuery = tmp as QueryList); + }); + + const cmptInstance = renderComponent(Cmpt); + + const fooList = (cmptInstance.fooQuery as QueryList); + expect(fooList.length).toBe(1); + expect(fooList.first.nativeElement).toEqual(elToQuery); + + const barList = (cmptInstance.barQuery as QueryList); + expect(barList.length).toBe(1); + expect(barList.first.nativeElement).toEqual(elToQuery); + }); + it('should query for multiple elements and read ElementRef by default', () => { let el1ToQuery; @@ -489,6 +525,41 @@ describe('query', () => { expect(qList.last).toBe(child2Instance); }); + it('should read multiple locals exporting the same directive from a given element', () => { + const Child = createDirective({exportAs: 'child'}); + let childInstance; + + /** + *
+ * class Cmpt { + * @ViewChildren('foo') fooQuery; + * @ViewChildren('bar') barQuery; + * } + */ + const Cmpt = createComponent('cmpt', function(ctx: any, cm: boolean) { + let tmp: any; + if (cm) { + query(0, ['foo'], true, QUERY_READ_FROM_NODE); + query(1, ['bar'], true, QUERY_READ_FROM_NODE); + elementStart(2, 'div', null, [Child], ['foo', 'child', 'bar', 'child']); + { childInstance = load(3); } + elementEnd(); + } + queryRefresh(tmp = load>(0)) && (ctx.fooQuery = tmp as QueryList); + queryRefresh(tmp = load>(1)) && (ctx.barQuery = tmp as QueryList); + }); + + const cmptInstance = renderComponent(Cmpt); + + const fooList = cmptInstance.fooQuery as QueryList; + expect(fooList.length).toBe(1); + expect(fooList.first).toBe(childInstance); + + const barList = cmptInstance.barQuery as QueryList; + expect(barList.length).toBe(1); + expect(barList.first).toBe(childInstance); + }); + it('should match on exported directive name and read a requested token', () => { const Child = createDirective({exportAs: 'child'});