From 5a2c3ff8b54364a58807a71f827350dee60cad2c Mon Sep 17 00:00:00 2001 From: Andrew Kushnir Date: Tue, 29 Jan 2019 17:13:02 -0800 Subject: [PATCH] fix(ivy): proper component resolution in case of inheritance (#28439) Ivy allows Components to extend Directives (but not the other way around) and as a result we may have Component and Directive annotations present at the same time. The logic that resolves annotations to pick the necessary one didn't take this into account and as a result Components were recognized as Directives (and vice versa) in case of inheritance. This change updates the resolution logic by picking known annotation that is the nearest one (in inheritance tree) and compares it with expected type. That should help avoid mis-classification of Components/Directives during resolution. PR Close #28439 --- packages/core/src/render3/jit/module.ts | 9 +- .../linker/inheritance_integration_spec.ts | 93 +++++++++++++++++++ packages/core/test/test_bed_spec.ts | 83 +++++++++++------ packages/core/testing/src/r3_test_bed.ts | 13 ++- packages/core/testing/src/resolvers.ts | 18 +++- 5 files changed, 180 insertions(+), 36 deletions(-) create mode 100644 packages/core/test/linker/inheritance_integration_spec.ts diff --git a/packages/core/src/render3/jit/module.ts b/packages/core/src/render3/jit/module.ts index 4773018e48..f706d178c8 100644 --- a/packages/core/src/render3/jit/module.ts +++ b/packages/core/src/render3/jit/module.ts @@ -345,9 +345,12 @@ function setScopeOnDeclaredComponents(moduleType: Type, ngModule: NgModule) */ export function patchComponentDefWithScope( componentDef: ComponentDef, transitiveScopes: NgModuleTransitiveScopes) { - componentDef.directiveDefs = () => Array.from(transitiveScopes.compilation.directives) - .map(dir => getDirectiveDef(dir) || getComponentDef(dir) !) - .filter(def => !!def); + componentDef.directiveDefs = () => + Array.from(transitiveScopes.compilation.directives) + .map( + dir => dir.hasOwnProperty(NG_COMPONENT_DEF) ? getComponentDef(dir) ! : + getDirectiveDef(dir) !) + .filter(def => !!def); componentDef.pipeDefs = () => Array.from(transitiveScopes.compilation.pipes).map(pipe => getPipeDef(pipe) !); } diff --git a/packages/core/test/linker/inheritance_integration_spec.ts b/packages/core/test/linker/inheritance_integration_spec.ts new file mode 100644 index 0000000000..24f7129079 --- /dev/null +++ b/packages/core/test/linker/inheritance_integration_spec.ts @@ -0,0 +1,93 @@ +/** + * @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, Directive, HostBinding} from '@angular/core'; +import {TestBed} from '@angular/core/testing'; +import {describe, expect, it} from '@angular/core/testing/src/testing_internal'; +import {modifiedInIvy, onlyInIvy} from '@angular/private/testing'; + +@Directive({selector: '[directiveA]'}) +class DirectiveA { +} + +@Directive({selector: '[directiveB]'}) +class DirectiveB { + @HostBinding('title') + title = 'DirectiveB Title'; +} + +@Component({selector: 'component-a', template: 'ComponentA Template'}) +class ComponentA { +} + +@Component( + {selector: 'component-extends-directive', template: 'ComponentExtendsDirective Template'}) +class ComponentExtendsDirective extends DirectiveA { +} + +class ComponentWithNoAnnotation extends ComponentA {} + +@Directive({selector: '[directiveExtendsComponent]'}) +class DirectiveExtendsComponent extends ComponentA { + @HostBinding('title') + title = 'DirectiveExtendsComponent Title'; +} + +class DirectiveWithNoAnnotation extends DirectiveB {} + +@Component({selector: 'my-app', template: '...'}) +class App { +} + +describe('Inheritance logic', () => { + it('should handle Components that extend Directives', () => { + TestBed.configureTestingModule({declarations: [ComponentExtendsDirective, App]}); + const template = ''; + TestBed.overrideComponent(App, {set: {template}}); + const fixture = TestBed.createComponent(App); + fixture.detectChanges(); + expect(fixture.nativeElement.firstChild.innerHTML).toBe('ComponentExtendsDirective Template'); + }); + + it('should handle classes with no annotations that extend Components', () => { + TestBed.configureTestingModule({declarations: [ComponentWithNoAnnotation, App]}); + const template = ''; + TestBed.overrideComponent(App, {set: {template}}); + const fixture = TestBed.createComponent(App); + fixture.detectChanges(); + expect(fixture.nativeElement.firstChild.innerHTML).toBe('ComponentA Template'); + }); + + it('should handle classes with no annotations that extend Directives', () => { + TestBed.configureTestingModule({declarations: [DirectiveWithNoAnnotation, App]}); + const template = '
'; + TestBed.overrideComponent(App, {set: {template}}); + const fixture = TestBed.createComponent(App); + fixture.detectChanges(); + expect(fixture.nativeElement.firstChild.title).toBe('DirectiveB Title'); + }); + + modifiedInIvy('View Engine allows Directives to extend Components') + .it('should handle Directives that extend Components', () => { + TestBed.configureTestingModule({declarations: [DirectiveExtendsComponent, App]}); + const template = '
Some content
'; + TestBed.overrideComponent(App, {set: {template}}); + const fixture = TestBed.createComponent(App); + fixture.detectChanges(); + expect(fixture.nativeElement.firstChild.title).toBe('DirectiveExtendsComponent Title'); + }); + + onlyInIvy('Ivy does not allow Directives to extend Components') + .it('should throw in case a Directive tries to extend a Component', () => { + TestBed.configureTestingModule({declarations: [DirectiveExtendsComponent, App]}); + const template = '
Some content
'; + TestBed.overrideComponent(App, {set: {template}}); + expect(() => TestBed.createComponent(App)) + .toThrowError('Directives cannot inherit Components'); + }); +}); diff --git a/packages/core/test/test_bed_spec.ts b/packages/core/test/test_bed_spec.ts index d1a0d2f3ae..07e2445fe4 100644 --- a/packages/core/test/test_bed_spec.ts +++ b/packages/core/test/test_bed_spec.ts @@ -257,42 +257,67 @@ describe('TestBed', () => { }); onlyInIvy('patched ng defs should be removed after resetting TestingModule') - .it('make sure we restore ng defs to their initial states', () => { - @Pipe({name: 'somePipe', pure: true}) - class SomePipe { - transform(value: string): string { return `transformed ${value}`; } - } + .describe('resetting ng defs', () => { + it('should restore ng defs to their initial states', () => { + @Pipe({name: 'somePipe', pure: true}) + class SomePipe { + transform(value: string): string { return `transformed ${value}`; } + } - @Directive({selector: 'someDirective'}) - class SomeDirective { - someProp = 'hello'; - } + @Directive({selector: 'someDirective'}) + class SomeDirective { + someProp = 'hello'; + } - @Component({selector: 'comp', template: 'someText'}) - class SomeComponent { - } + @Component({selector: 'comp', template: 'someText'}) + class SomeComponent { + } - @NgModule({declarations: [SomeComponent]}) - class SomeModule { - } + @NgModule({declarations: [SomeComponent]}) + class SomeModule { + } - TestBed.configureTestingModule({imports: [SomeModule]}); + TestBed.configureTestingModule({imports: [SomeModule]}); - // adding Pipe and Directive via metadata override - TestBed.overrideModule( - SomeModule, {set: {declarations: [SomeComponent, SomePipe, SomeDirective]}}); - TestBed.overrideComponent( - SomeComponent, {set: {template: `{{'hello' | somePipe}}`}}); - TestBed.createComponent(SomeComponent); + // adding Pipe and Directive via metadata override + TestBed.overrideModule( + SomeModule, {set: {declarations: [SomeComponent, SomePipe, SomeDirective]}}); + TestBed.overrideComponent( + SomeComponent, + {set: {template: `{{'hello' | somePipe}}`}}); + TestBed.createComponent(SomeComponent); - const defBeforeReset = (SomeComponent as any).ngComponentDef; - expect(defBeforeReset.pipeDefs().length).toEqual(1); - expect(defBeforeReset.directiveDefs().length).toEqual(2); // directive + component + const defBeforeReset = (SomeComponent as any).ngComponentDef; + expect(defBeforeReset.pipeDefs().length).toEqual(1); + expect(defBeforeReset.directiveDefs().length).toEqual(2); // directive + component - TestBed.resetTestingModule(); + TestBed.resetTestingModule(); - const defAfterReset = (SomeComponent as any).ngComponentDef; - expect(defAfterReset.pipeDefs().length).toEqual(0); - expect(defAfterReset.directiveDefs().length).toEqual(1); // component + const defAfterReset = (SomeComponent as any).ngComponentDef; + expect(defAfterReset.pipeDefs().length).toEqual(0); + expect(defAfterReset.directiveDefs().length).toEqual(1); // component + }); + + it('should cleanup ng defs for classes with no ng annotations (in case of inheritance)', + () => { + @Component({selector: 'someDirective', template: '...'}) + class SomeComponent { + } + + class ComponentWithNoAnnotations extends SomeComponent {} + + TestBed.configureTestingModule({declarations: [ComponentWithNoAnnotations]}); + TestBed.createComponent(ComponentWithNoAnnotations); + + expect(ComponentWithNoAnnotations.hasOwnProperty('ngComponentDef')).toBeTruthy(); + expect(SomeComponent.hasOwnProperty('ngComponentDef')).toBeTruthy(); + + TestBed.resetTestingModule(); + + expect(ComponentWithNoAnnotations.hasOwnProperty('ngComponentDef')).toBeFalsy(); + + // ngComponentDef should be preserved on super component + expect(SomeComponent.hasOwnProperty('ngComponentDef')).toBeTruthy(); + }); }); }); diff --git a/packages/core/testing/src/r3_test_bed.ts b/packages/core/testing/src/r3_test_bed.ts index 442e7b8ff1..3dcf1f12a0 100644 --- a/packages/core/testing/src/r3_test_bed.ts +++ b/packages/core/testing/src/r3_test_bed.ts @@ -336,7 +336,18 @@ export class TestBedRender3 implements Injector, TestBed { // restore initial component/directive/pipe defs this._initiaNgDefs.forEach((value: [string, PropertyDescriptor], type: Type) => { - Object.defineProperty(type, value[0], value[1]); + const [prop, descriptor] = value; + if (!descriptor) { + // Delete operations are generally undesirable since they have performance implications on + // objects they were applied to. In this particular case, situations where this code is + // invoked should be quite rare to cause any noticable impact, since it's applied only to + // some test cases (for example when class with no annotations extends some @Component) when + // we need to clear 'ngComponentDef' field on a given class to restore its original state + // (before applying overrides and running tests). + delete (type as any)[prop]; + } else { + Object.defineProperty(type, prop, descriptor); + } }); this._initiaNgDefs.clear(); clearResolutionOfComponentResourcesQueue(); diff --git a/packages/core/testing/src/resolvers.ts b/packages/core/testing/src/resolvers.ts index bacbe78273..027929078e 100644 --- a/packages/core/testing/src/resolvers.ts +++ b/packages/core/testing/src/resolvers.ts @@ -37,9 +37,21 @@ abstract class OverrideResolver implements Resolver { } getAnnotation(type: Type): T|null { - // We should always return the last match from filter(), or we may return superclass data by - // mistake. - return reflection.annotations(type).filter(a => a instanceof this.type).pop() || null; + const annotations = reflection.annotations(type); + // Try to find the nearest known Type annotation and make sure that this annotation is an + // instance of the type we are looking for, so we can use it for resolution. Note: there might + // be multiple known annotations found due to the fact that Components can extend Directives (so + // both Directive and Component annotations would be present), so we always check if the known + // annotation has the right type. + for (let i = annotations.length - 1; i >= 0; i--) { + const annotation = annotations[i]; + const isKnownType = annotation instanceof Directive || annotation instanceof Component || + annotation instanceof Pipe || annotation instanceof NgModule; + if (isKnownType) { + return annotation instanceof this.type ? annotation : null; + } + } + return null; } resolve(type: Type): T|null {