fix(ivy): cache local names and support multiple locals with same value (#22807)

PR Close #22807

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] =
+        node.tNode = tData[index] = createTNode(name, attrs || null, null, localNames);
-            createTNode(name, attrs || null, null, hostComponentDef ? null : queryName);
       }
 
       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
+ * This function instantiates the given directives. It is a hack since it assumes the directives
- * compute the query value only once and store it with the template (rather than on each invocation)
+ * come in the correct order for DI.
  */
 function hack_declareDirectives(
     index: number, directiveTypes: DirectiveType<any>[] | null | undefined,
@@ -546,30 +544,35 @@ function hack_declareDirectives(
       const directiveType = directiveTypes[i];
       const directiveDef = currentView.tView.firstTemplatePass ? directiveType.ngDirectiveDef :
                                                                  tData[index] as DirectiveDef<any>;
-      directiveCreate(
+      const localNames = currentView.tView.firstTemplatePass ?
-          index, directiveDef.n(), directiveDef, hack_findQueryName(directiveDef, localRefs));
+          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
+ * Finds any local names that match the given directive's exportAs and returns them with directive
- * compute the query value only once and store it with the template (rather than on each invocation)
+ * index. If the directiveDef is null, it matches against the default '' value instead of
+ * exportAs.
  */
-function hack_findQueryName(
+function findMatchingLocalNames(
-    directiveDef: DirectiveDef<any>| null, localRefs: string[] | null | undefined,
+    directiveDef: DirectiveDef<any>| null, localRefs: string[] | null | undefined, index: number,
-    defaultExport?: string, ): string|null {
+    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<T>(
  * @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<T>(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<T>(
-    index: number, directive: T, directiveDef: DirectiveDef<T>, queryName?: string | null): T {
+    index: number, directive: T, directiveDef: DirectiveDef<T>,
+    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<T>(
 
   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 localNames: (string | number)[]|null = findMatchingLocalNames(null, localRefs, -1, '');
-    const queryName: string|null = hack_findQueryName(null, localRefs, '');
+    node.tNode = tData[index] = createTNode(tagName || null, attrs || null, [], localNames);
-    node.tNode = tData[index] = createTNode(tagName || null, attrs || null, [], queryName || null);
   }
 
   // Containers are added to the current view tree instead of their embedded views

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;
+
+      /**
+       * <div #foo #bar></div>
+       * <div></div>
+       * 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<QueryList<any>>(0)) && (ctx.fooQuery = tmp as QueryList<any>);
+        queryRefresh(tmp = load<QueryList<any>>(1)) && (ctx.barQuery = tmp as QueryList<any>);
+      });
+
+      const cmptInstance = renderComponent(Cmpt);
+
+      const fooList = (cmptInstance.fooQuery as QueryList<any>);
+      expect(fooList.length).toBe(1);
+      expect(fooList.first.nativeElement).toEqual(elToQuery);
+
+      const barList = (cmptInstance.barQuery as QueryList<any>);
+      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;
+
+      /**
+       * <div child #foo="child" #bar="child"></div>
+       * 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<QueryList<any>>(0)) && (ctx.fooQuery = tmp as QueryList<any>);
+        queryRefresh(tmp = load<QueryList<any>>(1)) && (ctx.barQuery = tmp as QueryList<any>);
+      });
+
+      const cmptInstance = renderComponent(Cmpt);
+
+      const fooList = cmptInstance.fooQuery as QueryList<any>;
+      expect(fooList.length).toBe(1);
+      expect(fooList.first).toBe(childInstance);
+
+      const barList = cmptInstance.barQuery as QueryList<any>;
+      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'});