feat(ivy): allow non-unique #localRefs to be defined in a template (#28627)

Prior to this change in Ivy we had strict check that disabled non-unique #localRefs usage within a given template. While this limitation was technically present in View Engine, in many cases View Engine neglected this restriction and as a result, some apps relied on a fact that multiple non-unique #localRefs can be defined and utilized to query elements via @ViewChild(ren) and @ContentChild(ren). In order to provide better compatibility with View Engine, this commit removes existing restriction.

As a part of this commit, are few tests were added to verify VE and Ivy compatibility in most common use-cases where multiple non-unique #localRefs were used.

PR Close #28627

packages/compiler-cli/test/ngtsc/ngtsc_spec.ts

@@ -1471,7 +1471,7 @@ describe('ngtsc behavioral tests', () => {
     });
   });
 
-  describe('duplicate local refs', () => {
+  describe('multiple local refs', () => {
     const getComponentScript = (template: string): string => `
       import {Component, Directive, NgModule} from '@angular/core';
 
@@ -1482,37 +1482,17 @@ describe('ngtsc behavioral tests', () => {
       class Module {}
     `;
 
-    // Components with templates listed below should
+    const cases = [
-    // throw the "ref is already defined" error
-    const invalidCases = [
       `
         <div #ref></div>
         <div #ref></div>
       `,
-      `
-        <div #ref>
-          <div #ref></div>
-        </div>
-      `,
-      `
-        <div>
-          <div #ref></div>
-        </div>
-        <div>
-          <div #ref></div>
-        </div>
-      `,
       `
         <ng-container>
           <div #ref></div>
         </ng-container>
         <div #ref></div>
-      `
+      `,
-    ];
-
-    // Components with templates listed below should not throw
-    // the error, since refs are located in different scopes
-    const validCases = [
       `
         <ng-template>
           <div #ref></div>
@@ -1529,18 +1509,8 @@ describe('ngtsc behavioral tests', () => {
       `
     ];
 
-    invalidCases.forEach(template => {
+    cases.forEach(template => {
-      it('should throw in case of duplicate refs', () => {
+      it('should not throw', () => {
-        env.tsconfig();
-        env.write('test.ts', getComponentScript(template));
-        const errors = env.driveDiagnostics();
-        expect(errors[0].messageText)
-            .toContain('Internal Error: The name ref is already defined in scope');
-      });
-    });
-
-    validCases.forEach(template => {
-      it('should not throw in case refs are in different scopes', () => {
         env.tsconfig();
         env.write('test.ts', getComponentScript(template));
         const errors = env.driveDiagnostics();

packages/compiler/src/render3/view/template.ts

@@ -1395,8 +1395,14 @@ export class BindingScope implements LocalResolver {
   set(retrievalLevel: number, name: string, lhs: o.ReadVarExpr,
       priority: number = DeclarationPriority.DEFAULT,
       declareLocalCallback?: DeclareLocalVarCallback, localRef?: true): BindingScope {
-    !this.map.has(name) ||
+    if (this.map.has(name)) {
-        error(`The name ${name} is already defined in scope to be ${this.map.get(name)}`);
+      if (localRef) {
+        // Do not throw an error if it's a local ref and do not update existing value,
+        // so the first defined ref is always returned.
+        return this;
+      }
+      error(`The name ${name} is already defined in scope to be ${this.map.get(name)}`);
+    }
     this.map.set(name, {
       retrievalLevel: retrievalLevel,
       lhs: lhs,

packages/core/test/acceptance/exports_spec.ts

@@ -55,6 +55,21 @@ describe('exports', () => {
     expect(dirWithInput.comp).toEqual(myComp);
   });
 
+
+  onlyInIvy(
+      'in Ivy first declared ref is selected in case of multiple non-unique refs, when VE used the last one')
+      .it('should point to the first declared ref', () => {
+        const fixture = initWithTemplate(AppComp, `
+          <div>
+            <input value="First" #ref />
+            <input value="Second" #ref />
+            <input value="Third" #ref />
+            <span>{{ ref.value }}</span>
+          </div>
+        `);
+        fixture.detectChanges();
+        expect(fixture.nativeElement.querySelector('span').innerHTML).toBe('First');
+      });
 });
 
 function initWithTemplate(compType: Type<any>, template: string) {

packages/core/test/acceptance/query_spec.ts

@@ -0,0 +1,212 @@
+/**
+ * @license
+ * Copyright Google Inc. All Rights Reserved.
+ *
+ * Use of this source code is governed by an MIT-style license that can be
+ * found in the LICENSE file at https://angular.io/license
+ */
+
+import {Component, ContentChild, ContentChildren, ElementRef, QueryList, TemplateRef, Type, ViewChild, ViewChildren} from '@angular/core';
+import {TestBed} from '@angular/core/testing';
+import {expect} from '@angular/platform-browser/testing/src/matchers';
+import {onlyInIvy} from '@angular/private/testing';
+
+
+describe('query logic', () => {
+  beforeEach(() => {
+    TestBed.configureTestingModule({declarations: [AppComp, QueryComp, SimpleCompA, SimpleCompB]});
+  });
+
+  it('should return Component instances when Components are labelled and retrieved via View query',
+     () => {
+       const template = `
+         <div><simple-comp-a #viewQuery></simple-comp-a></div>
+         <div><simple-comp-b #viewQuery></simple-comp-b></div>
+       `;
+       const fixture = initWithTemplate(QueryComp, template);
+       const comp = fixture.componentInstance;
+       expect(comp.viewChild).toBeAnInstanceOf(SimpleCompA);
+       expect(comp.viewChildren.first).toBeAnInstanceOf(SimpleCompA);
+       expect(comp.viewChildren.last).toBeAnInstanceOf(SimpleCompB);
+     });
+
+  it('should return Component instance when Component is labelled and retrieved via Content query',
+     () => {
+       const template = `
+         <local-ref-query-component #q>
+           <simple-comp-a #contentQuery></simple-comp-a>
+         </local-ref-query-component>
+       `;
+       const fixture = initWithTemplate(AppComp, template);
+       const comp = fixture.debugElement.children[0].references['q'];
+       expect(comp.contentChild).toBeAnInstanceOf(SimpleCompA);
+       expect(comp.contentChildren.first).toBeAnInstanceOf(SimpleCompA);
+     });
+
+  onlyInIvy('multiple local refs are supported in Ivy')
+      .it('should return Component instances when Components are labelled and retrieved via Content query',
+          () => {
+            const template = `
+              <local-ref-query-component #q>
+                <simple-comp-a #contentQuery></simple-comp-a>
+                <simple-comp-b #contentQuery></simple-comp-b>
+              </local-ref-query-component>
+            `;
+            const fixture = initWithTemplate(AppComp, template);
+            const comp = fixture.debugElement.children[0].references['q'];
+            expect(comp.contentChild).toBeAnInstanceOf(SimpleCompA);
+            expect(comp.contentChildren.first).toBeAnInstanceOf(SimpleCompA);
+            expect(comp.contentChildren.last).toBeAnInstanceOf(SimpleCompB);
+            expect(comp.contentChildren.length).toBe(2);
+          });
+
+  it('should return ElementRef when HTML element is labelled and retrieved via View query', () => {
+    const template = `
+      <div #viewQuery></div>
+    `;
+    const fixture = initWithTemplate(QueryComp, template);
+    const comp = fixture.componentInstance;
+    expect(comp.viewChild).toBeAnInstanceOf(ElementRef);
+    expect(comp.viewChildren.first).toBeAnInstanceOf(ElementRef);
+  });
+
+  onlyInIvy('multiple local refs are supported in Ivy')
+      .it('should return ElementRefs when HTML elements are labelled and retrieved via View query',
+          () => {
+            const template = `
+              <div #viewQuery #first>A</div>
+              <div #viewQuery #second>B</div>
+            `;
+            const fixture = initWithTemplate(QueryComp, template);
+            const comp = fixture.componentInstance;
+
+            expect(comp.viewChild).toBeAnInstanceOf(ElementRef);
+            expect(comp.viewChild.nativeElement)
+                .toBe(fixture.debugElement.children[0].nativeElement);
+
+            expect(comp.viewChildren.first).toBeAnInstanceOf(ElementRef);
+            expect(comp.viewChildren.last).toBeAnInstanceOf(ElementRef);
+            expect(comp.viewChildren.length).toBe(2);
+          });
+
+  it('should return TemplateRef when template is labelled and retrieved via View query', () => {
+    const template = `
+      <ng-template #viewQuery></ng-template>
+    `;
+    const fixture = initWithTemplate(QueryComp, template);
+    const comp = fixture.componentInstance;
+    expect(comp.viewChildren.first).toBeAnInstanceOf(TemplateRef);
+  });
+
+  onlyInIvy('multiple local refs are supported in Ivy')
+      .it('should return TemplateRefs when templates are labelled and retrieved via View query',
+          () => {
+            const template = `
+              <ng-template #viewQuery></ng-template>
+              <ng-template #viewQuery></ng-template>
+            `;
+            const fixture = initWithTemplate(QueryComp, template);
+            const comp = fixture.componentInstance;
+            expect(comp.viewChild).toBeAnInstanceOf(TemplateRef);
+            expect(comp.viewChild.elementRef.nativeElement)
+                .toBe(fixture.debugElement.childNodes[0].nativeNode);
+
+            expect(comp.viewChildren.first).toBeAnInstanceOf(TemplateRef);
+            expect(comp.viewChildren.last).toBeAnInstanceOf(TemplateRef);
+            expect(comp.viewChildren.length).toBe(2);
+          });
+
+  it('should return ElementRef when HTML element is labelled and retrieved via Content query',
+     () => {
+       const template = `
+         <local-ref-query-component #q>
+           <div #contentQuery></div>
+         </local-ref-query-component>
+       `;
+       const fixture = initWithTemplate(AppComp, template);
+       const comp = fixture.debugElement.children[0].references['q'];
+       expect(comp.contentChildren.first).toBeAnInstanceOf(ElementRef);
+     });
+
+  onlyInIvy('multiple local refs are supported in Ivy')
+      .it('should return ElementRefs when HTML elements are labelled and retrieved via Content query',
+          () => {
+            const template = `
+              <local-ref-query-component #q>
+                <div #contentQuery></div>
+                <div #contentQuery></div>
+              </local-ref-query-component>
+            `;
+            const fixture = initWithTemplate(AppComp, template);
+            const firstChild = fixture.debugElement.children[0];
+            const comp = firstChild.references['q'];
+
+            expect(comp.contentChild).toBeAnInstanceOf(ElementRef);
+            expect(comp.contentChild.nativeElement).toBe(firstChild.children[0].nativeElement);
+
+            expect(comp.contentChildren.first).toBeAnInstanceOf(ElementRef);
+            expect(comp.contentChildren.last).toBeAnInstanceOf(ElementRef);
+            expect(comp.contentChildren.length).toBe(2);
+          });
+
+  it('should return TemplateRef when template is labelled and retrieved via Content query', () => {
+    const template = `
+       <local-ref-query-component #q>
+         <ng-template #contentQuery></ng-template>
+       </local-ref-query-component>
+     `;
+    const fixture = initWithTemplate(AppComp, template);
+    const comp = fixture.debugElement.children[0].references['q'];
+    expect(comp.contentChildren.first).toBeAnInstanceOf(TemplateRef);
+  });
+
+  onlyInIvy('multiple local refs are supported in Ivy')
+      .it('should return TemplateRefs when templates are labelled and retrieved via Content query',
+          () => {
+            const template = `
+              <local-ref-query-component #q>
+                <ng-template #contentQuery></ng-template>
+                <ng-template #contentQuery></ng-template>
+              </local-ref-query-component>
+            `;
+            const fixture = initWithTemplate(AppComp, template);
+            const firstChild = fixture.debugElement.children[0];
+            const comp = firstChild.references['q'];
+
+            expect(comp.contentChild).toBeAnInstanceOf(TemplateRef);
+            expect(comp.contentChild.elementRef.nativeElement)
+                .toBe(firstChild.childNodes[0].nativeNode);
+
+            expect(comp.contentChildren.first).toBeAnInstanceOf(TemplateRef);
+            expect(comp.contentChildren.last).toBeAnInstanceOf(TemplateRef);
+            expect(comp.contentChildren.length).toBe(2);
+          });
+});
+
+function initWithTemplate(compType: Type<any>, template: string) {
+  TestBed.overrideComponent(compType, {set: new Component({template})});
+  const fixture = TestBed.createComponent(compType);
+  fixture.detectChanges();
+  return fixture;
+}
+
+@Component({selector: 'local-ref-query-component', template: '<ng-content></ng-content>'})
+class QueryComp {
+  @ViewChild('viewQuery') viewChild !: any;
+  @ContentChild('contentQuery') contentChild !: any;
+
+  @ViewChildren('viewQuery') viewChildren !: QueryList<any>;
+  @ContentChildren('contentQuery') contentChildren !: QueryList<any>;
+}
+
+@Component({selector: 'app-comp', template: ``})
+class AppComp {
+}
+
+@Component({selector: 'simple-comp-a', template: ''})
+class SimpleCompA {
+}
+
+@Component({selector: 'simple-comp-b', template: ''})
+class SimpleCompB {
+}

packages/core/test/linker/query_integration_spec.ts

@@ -270,19 +270,15 @@ describe('Query API', () => {
   });
 
   describe('read a different token', () => {
-    modifiedInIvy(
+    it('should contain all content children', () => {
-        'Breaking change in Ivy: no longer allow multiple local refs with the same name, all local refs are now unique')
+      const template =
-        .it('should contain all content children', () => {
+          '<needs-content-children-read #q text="ca"><div #q text="cb"></div></needs-content-children-read>';
-          const template =
+      const view = createTestCmpAndDetectChanges(MyComp0, template);
-              '<needs-content-children-read #q text="ca"><div #q text="cb"></div></needs-content-children-read>';
-          const view = createTestCmpAndDetectChanges(MyComp0, template);
 
-          const comp: NeedsContentChildrenWithRead =
+      const comp: NeedsContentChildrenWithRead =
-              view.debugElement.children[0].injector.get(NeedsContentChildrenWithRead);
+          view.debugElement.children[0].injector.get(NeedsContentChildrenWithRead);
-          expect(comp.textDirChildren.map(textDirective => textDirective.text)).toEqual([
+      expect(comp.textDirChildren.map(textDirective => textDirective.text)).toEqual(['ca', 'cb']);
-            'ca', 'cb'
+    });
-          ]);
-        });
 
     it('should contain the first content child', () => {
       const template =