From a9afe629c79c6686c8fba8e6187a9fbeb8d8cbba Mon Sep 17 00:00:00 2001 From: Andrew Kushnir Date: Fri, 8 Feb 2019 14:11:33 -0800 Subject: [PATCH] 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 --- .../compiler-cli/test/ngtsc/ngtsc_spec.ts | 40 +--- .../compiler/src/render3/view/template.ts | 10 +- packages/core/test/acceptance/exports_spec.ts | 15 ++ packages/core/test/acceptance/query_spec.ts | 212 ++++++++++++++++++ .../test/linker/query_integration_spec.ts | 20 +- 5 files changed, 248 insertions(+), 49 deletions(-) create mode 100644 packages/core/test/acceptance/query_spec.ts diff --git a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts index 0c0bcf7ce3..6d727ce9fb 100644 --- a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts +++ b/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 - // throw the "ref is already defined" error - const invalidCases = [ + const cases = [ `
`, - ` -
-
-
- `, - ` -
-
-
-
-
-
- `, `
- ` - ]; - - // Components with templates listed below should not throw - // the error, since refs are located in different scopes - const validCases = [ + `, `
@@ -1529,18 +1509,8 @@ describe('ngtsc behavioral tests', () => { ` ]; - invalidCases.forEach(template => { - it('should throw in case of duplicate refs', () => { - 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', () => { + cases.forEach(template => { + it('should not throw', () => { env.tsconfig(); env.write('test.ts', getComponentScript(template)); const errors = env.driveDiagnostics(); diff --git a/packages/compiler/src/render3/view/template.ts b/packages/compiler/src/render3/view/template.ts index 71876720da..2483fa571c 100644 --- a/packages/compiler/src/render3/view/template.ts +++ b/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) || - error(`The name ${name} is already defined in scope to be ${this.map.get(name)}`); + if (this.map.has(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, diff --git a/packages/core/test/acceptance/exports_spec.ts b/packages/core/test/acceptance/exports_spec.ts index 0945171357..42bc020b58 100644 --- a/packages/core/test/acceptance/exports_spec.ts +++ b/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, ` +
+ + + + {{ ref.value }} +
+ `); + fixture.detectChanges(); + expect(fixture.nativeElement.querySelector('span').innerHTML).toBe('First'); + }); }); function initWithTemplate(compType: Type, template: string) { diff --git a/packages/core/test/acceptance/query_spec.ts b/packages/core/test/acceptance/query_spec.ts new file mode 100644 index 0000000000..80775e1c1a --- /dev/null +++ b/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 = ` +
+
+ `; + 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 = ` + + + + `; + 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 = ` + + + + + `; + 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 = ` +
+ `; + 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 = ` +
A
+
B
+ `; + 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 = ` + + `; + 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 = ` + + + `; + 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 = ` + +
+ + `; + 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 = ` + +
+
+ + `; + 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 = ` + + + + `; + 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 = ` + + + + + `; + 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, 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: ''}) +class QueryComp { + @ViewChild('viewQuery') viewChild !: any; + @ContentChild('contentQuery') contentChild !: any; + + @ViewChildren('viewQuery') viewChildren !: QueryList; + @ContentChildren('contentQuery') contentChildren !: QueryList; +} + +@Component({selector: 'app-comp', template: ``}) +class AppComp { +} + +@Component({selector: 'simple-comp-a', template: ''}) +class SimpleCompA { +} + +@Component({selector: 'simple-comp-b', template: ''}) +class SimpleCompB { +} \ No newline at end of file diff --git a/packages/core/test/linker/query_integration_spec.ts b/packages/core/test/linker/query_integration_spec.ts index 29d72ddb13..63f6282875 100644 --- a/packages/core/test/linker/query_integration_spec.ts +++ b/packages/core/test/linker/query_integration_spec.ts @@ -270,19 +270,15 @@ describe('Query API', () => { }); describe('read a different token', () => { - modifiedInIvy( - 'Breaking change in Ivy: no longer allow multiple local refs with the same name, all local refs are now unique') - .it('should contain all content children', () => { - const template = - '
'; - const view = createTestCmpAndDetectChanges(MyComp0, template); + it('should contain all content children', () => { + const template = + '
'; + const view = createTestCmpAndDetectChanges(MyComp0, template); - const comp: NeedsContentChildrenWithRead = - view.debugElement.children[0].injector.get(NeedsContentChildrenWithRead); - expect(comp.textDirChildren.map(textDirective => textDirective.text)).toEqual([ - 'ca', 'cb' - ]); - }); + const comp: NeedsContentChildrenWithRead = + view.debugElement.children[0].injector.get(NeedsContentChildrenWithRead); + expect(comp.textDirChildren.map(textDirective => textDirective.text)).toEqual(['ca', 'cb']); + }); it('should contain the first content child', () => { const template =