From c3c72f689a22325ad165934c2e834ee6cb02d43d Mon Sep 17 00:00:00 2001 From: crisbeto Date: Sun, 29 Dec 2019 10:50:19 +0200 Subject: [PATCH] fix(ivy): handle overloaded constructors in ngtsc (#34590) Currently ngtsc looks for the first `ConstructorDeclaration` when figuring out what the parameters are so that it can generate the DI instructions. The problem is that if a constructor has overloads, it'll have several `ConstructorDeclaration` members with a different number of parameters. These changes tweak the logic so it looks for the constructor implementation. PR Close #34590 --- .../src/ngtsc/reflection/src/typescript.ts | 10 ++++-- .../src/ngtsc/reflection/test/ts_host_spec.ts | 23 ++++++++++++ .../compliance/r3_view_compiler_di_spec.ts | 35 +++++++++++++++++++ .../compiler-cli/test/ngtsc/ngtsc_spec.ts | 26 ++++++++++++++ packages/core/test/acceptance/di_spec.ts | 24 +++++++++++++ 5 files changed, 115 insertions(+), 3 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/reflection/src/typescript.ts b/packages/compiler-cli/src/ngtsc/reflection/src/typescript.ts index 7fddda5784..c9c7fd779b 100644 --- a/packages/compiler-cli/src/ngtsc/reflection/src/typescript.ts +++ b/packages/compiler-cli/src/ngtsc/reflection/src/typescript.ts @@ -35,8 +35,12 @@ export class TypeScriptReflectionHost implements ReflectionHost { getConstructorParameters(clazz: ClassDeclaration): CtorParameter[]|null { const tsClazz = castDeclarationToClassOrDie(clazz); - // First, find the constructor. - const ctor = tsClazz.members.find(ts.isConstructorDeclaration); + // First, find the constructor with a `body`. The constructors without a `body` are overloads + // whereas we want the implementation since it's the one that'll be executed and which can + // have decorators. + const ctor = tsClazz.members.find( + (member): member is ts.ConstructorDeclaration => + ts.isConstructorDeclaration(member) && member.body !== undefined); if (ctor === undefined) { return null; } @@ -564,4 +568,4 @@ function getExportedName(decl: ts.Declaration, originalId: ts.Identifier): strin return ts.isImportSpecifier(decl) ? (decl.propertyName !== undefined ? decl.propertyName : decl.name).text : originalId.text; -} \ No newline at end of file +} diff --git a/packages/compiler-cli/src/ngtsc/reflection/test/ts_host_spec.ts b/packages/compiler-cli/src/ngtsc/reflection/test/ts_host_spec.ts index 8f2bfe8615..475d2451aa 100644 --- a/packages/compiler-cli/src/ngtsc/reflection/test/ts_host_spec.ts +++ b/packages/compiler-cli/src/ngtsc/reflection/test/ts_host_spec.ts @@ -211,6 +211,29 @@ runInEachFileSystem(() => { expect(args.length).toBe(1); expectParameter(args[0], 'bar', {moduleName: './bar', name: 'Bar'}); }); + + it('should reflect the arguments from an overloaded constructor', () => { + const {program} = makeProgram([{ + name: _('/entry.ts'), + contents: ` + class Bar {} + class Baz {} + + class Foo { + constructor(bar: Bar); + constructor(bar: Bar, baz?: Baz) {} + } + ` + }]); + const clazz = getDeclaration(program, _('/entry.ts'), 'Foo', isNamedClassDeclaration); + const checker = program.getTypeChecker(); + const host = new TypeScriptReflectionHost(checker); + const args = host.getConstructorParameters(clazz) !; + expect(args.length).toBe(2); + expectParameter(args[0], 'bar', 'Bar'); + expectParameter(args[1], 'baz', 'Baz'); + }); + }); diff --git a/packages/compiler-cli/test/compliance/r3_view_compiler_di_spec.ts b/packages/compiler-cli/test/compliance/r3_view_compiler_di_spec.ts index b69ebbc50d..935d4e447a 100644 --- a/packages/compiler-cli/test/compliance/r3_view_compiler_di_spec.ts +++ b/packages/compiler-cli/test/compliance/r3_view_compiler_di_spec.ts @@ -99,6 +99,41 @@ describe('compiler compliance: dependency injection', () => { expectEmit(result.source, def, 'Incorrect injectable definition'); }); + it('should create a factory definition for an injectable with an overloaded constructor', () => { + const files = { + app: { + 'spec.ts': ` + import {Injectable, Optional} from '@angular/core'; + + class MyDependency {} + class MyOptionalDependency {} + + @Injectable() + export class MyService { + constructor(dep: MyDependency); + constructor(dep: MyDependency, @Optional() optionalDep?: MyOptionalDependency) {} + } + ` + } + }; + + const factory = ` + MyService.ɵfac = function MyService_Factory(t) { + return new (t || MyService)($r3$.ɵɵinject(MyDependency), $r3$.ɵɵinject(MyOptionalDependency, 8)); + }`; + + const def = ` + MyService.ɵprov = $r3$.ɵɵdefineInjectable({ + token: MyService, + factory: MyService.ɵfac + }); + `; + + const result = compile(files, angularFiles); + expectEmit(result.source, factory, 'Incorrect factory definition'); + expectEmit(result.source, def, 'Incorrect injectable definition'); + }); + it('should create a single factory def if the class has more than one decorator', () => { const files = { app: { diff --git a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts index 11e8567501..5f8611f8ae 100644 --- a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts +++ b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts @@ -192,6 +192,32 @@ runInEachFileSystem(os => { expect(jsContents).toContain('inject(Dep, 8)'); }); + it('should compile @Injectable with constructor overloads', () => { + env.write('test.ts', ` + import {Injectable, Optional} from '@angular/core'; + + @Injectable() + class Dep {} + + @Injectable() + class OptionalDep {} + + @Injectable() + class Service { + constructor(dep: Dep); + + constructor(dep: Dep, @Optional() optionalDep?: OptionalDep) {} + } + `); + env.driveMain(); + const jsContents = env.getContents('test.js'); + + expect(jsContents) + .toContain( + `Service.ɵfac = function Service_Factory(t) { ` + + `return new (t || Service)(i0.ɵɵinject(Dep), i0.ɵɵinject(OptionalDep, 8)); };`); + }); + it('should compile Directives without errors', () => { env.write('test.ts', ` import {Directive} from '@angular/core'; diff --git a/packages/core/test/acceptance/di_spec.ts b/packages/core/test/acceptance/di_spec.ts index 9a2df0297c..83faaac385 100644 --- a/packages/core/test/acceptance/di_spec.ts +++ b/packages/core/test/acceptance/di_spec.ts @@ -982,6 +982,30 @@ describe('di', () => { `This will become an error in v10. Please add @Injectable() to the "MyRootService" class.`); } }); + + it('should inject services in constructor with overloads', () => { + @Injectable({providedIn: 'root'}) + class MyService { + } + + @Injectable({providedIn: 'root'}) + class MyOtherService { + } + + @Component({template: ''}) + class MyComp { + constructor(myService: MyService); + constructor( + public myService: MyService, @Optional() public myOtherService?: MyOtherService) {} + } + TestBed.configureTestingModule({declarations: [MyComp]}); + const fixture = TestBed.createComponent(MyComp); + fixture.detectChanges(); + + expect(fixture.componentInstance.myService instanceof MyService).toBe(true); + expect(fixture.componentInstance.myOtherService instanceof MyOtherService).toBe(true); + }); + }); describe('inject', () => {