From 747f0cff9ee92c4afd41ecccbbe656995eac906a Mon Sep 17 00:00:00 2001 From: JoostK Date: Sun, 29 Sep 2019 21:17:38 +0200 Subject: [PATCH] fix(ngcc): handle presence of both `ctorParameters` and `__decorate` (#32901) Recently ng-packagr was updated to include a transform that used to be done in tsickle (https://github.com/ng-packagr/ng-packagr/pull/1401), where only constructor parameter decorators are emitted in tsickle's format, not any of the other decorators. ngcc used to extract decorators from only a single format, so once it saw the `ctorParameters` static property it assumed the library is using the tsickle format. Therefore, none of the `__decorate` calls were considered. This resulted in missing decorator information, preventing proper processing of a package. This commit changes how decorators are extracted by always looking at both the static properties and the `__decorate` calls, merging these sources appropriately. Resolves FW-1573 PR Close #32901 --- .../ngcc/src/host/esm2015_host.ts | 33 +++---- .../host/esm2015_host_import_helper_spec.ts | 89 +++++++++++++++++ .../test/host/esm5_host_import_helper_spec.ts | 96 +++++++++++++++++++ 3 files changed, 200 insertions(+), 18 deletions(-) diff --git a/packages/compiler-cli/ngcc/src/host/esm2015_host.ts b/packages/compiler-cli/ngcc/src/host/esm2015_host.ts index a2a6f0b9ae..ad72c24820 100644 --- a/packages/compiler-cli/ngcc/src/host/esm2015_host.ts +++ b/packages/compiler-cli/ngcc/src/host/esm2015_host.ts @@ -655,12 +655,16 @@ export class Esm2015ReflectionHost extends TypeScriptReflectionHost implements N return this.decoratorCache.get(decl) !; } - // First attempt extracting decorators from static properties. - let decoratorInfo = this.computeDecoratorInfoFromStaticProperties(classSymbol); - if (decoratorInfo === null) { - // If none were present, use the `__decorate` helper calls instead. - decoratorInfo = this.computeDecoratorInfoFromHelperCalls(classSymbol); - } + // Extract decorators from static properties and `__decorate` helper calls, then merge them + // together where the information from the static properties is preferred. + const staticProps = this.computeDecoratorInfoFromStaticProperties(classSymbol); + const helperCalls = this.computeDecoratorInfoFromHelperCalls(classSymbol); + + const decoratorInfo: DecoratorInfo = { + classDecorators: staticProps.classDecorators || helperCalls.classDecorators, + memberDecorators: staticProps.memberDecorators || helperCalls.memberDecorators, + constructorParamInfo: staticProps.constructorParamInfo || helperCalls.constructorParamInfo, + }; this.decoratorCache.set(decl, decoratorInfo); return decoratorInfo; @@ -676,8 +680,10 @@ export class Esm2015ReflectionHost extends TypeScriptReflectionHost implements N * @returns All information on the decorators as extracted from static properties, or `null` if * none of the static properties exist. */ - protected computeDecoratorInfoFromStaticProperties(classSymbol: NgccClassSymbol): DecoratorInfo - |null { + protected computeDecoratorInfoFromStaticProperties(classSymbol: NgccClassSymbol): { + classDecorators: Decorator[] | null; memberDecorators: Map| null; + constructorParamInfo: ParamInfo[] | null; + } { let classDecorators: Decorator[]|null = null; let memberDecorators: Map|null = null; let constructorParamInfo: ParamInfo[]|null = null; @@ -697,16 +703,7 @@ export class Esm2015ReflectionHost extends TypeScriptReflectionHost implements N constructorParamInfo = this.getParamInfoFromStaticProperty(constructorParamsProperty); } - // If none of the static properties were present, no decorator info could be computed. - if (classDecorators === null && memberDecorators === null && constructorParamInfo === null) { - return null; - } - - return { - classDecorators, - memberDecorators: memberDecorators || new Map(), - constructorParamInfo: constructorParamInfo || [], - }; + return {classDecorators, memberDecorators, constructorParamInfo}; } /** diff --git a/packages/compiler-cli/ngcc/test/host/esm2015_host_import_helper_spec.ts b/packages/compiler-cli/ngcc/test/host/esm2015_host_import_helper_spec.ts index 230445bb2c..2b22192e88 100644 --- a/packages/compiler-cli/ngcc/test/host/esm2015_host_import_helper_spec.ts +++ b/packages/compiler-cli/ngcc/test/host/esm2015_host_import_helper_spec.ts @@ -62,6 +62,36 @@ runInEachFileSystem(() => { TemplateRef, String]) ], SomeDirective); export { SomeDirective }; + `, + }, + { + name: _('/some_directive_ctor_parameters.js'), + contents: ` + import * as tslib_1 from 'tslib'; + import { Directive, Inject, InjectionToken, Input } from '@angular/core'; + const INJECTED_TOKEN = new InjectionToken('injected'); + class ViewContainerRef { + } + class TemplateRef { + } + let SomeDirective = class SomeDirective { + constructor(_viewContainer, _template, injected) { + this.input1 = ''; + } + }; + SomeDirective.ctorParameters = () => [ + { type: ViewContainerRef, }, + { type: TemplateRef, }, + { type: undefined, decorators: [{ type: Inject, args: [INJECTED_TOKEN,] },] }, + ]; + tslib_1.__decorate([ + Input(), + ], SomeDirective.prototype, "input1", void 0); + SomeDirective = tslib_1.__decorate([ + Directive({ selector: '[someDirective]' }), + tslib_1.__param(2, Inject(INJECTED_TOKEN)), + ], SomeDirective); + export { SomeDirective }; `, }, { @@ -152,6 +182,28 @@ runInEachFileSystem(() => { ]); }); + it('should find the decorators on a class when mixing `ctorParameters` and `__decorate`', + () => { + const {program} = makeTestBundleProgram(_('/some_directive_ctor_parameters.js')); + const host = + new Esm2015ReflectionHost(new MockLogger(), false, program.getTypeChecker()); + const classNode = getDeclaration( + program, _('/some_directive_ctor_parameters.js'), 'SomeDirective', + isNamedVariableDeclaration); + const decorators = host.getDecoratorsOfDeclaration(classNode) !; + + expect(decorators).toBeDefined(); + expect(decorators.length).toEqual(1); + + const decorator = decorators[0]; + expect(decorator.name).toEqual('Directive'); + expect(decorator.identifier.getText()).toEqual('Directive'); + expect(decorator.import).toEqual({name: 'Directive', from: '@angular/core'}); + expect(decorator.args !.map(arg => arg.getText())).toEqual([ + '{ selector: \'[someDirective]\' }', + ]); + }); + it('should support decorators being used inside @angular/core', () => { const {program} = makeTestBundleProgram(_('/node_modules/@angular/core/some_directive.js')); @@ -195,6 +247,22 @@ runInEachFileSystem(() => { expect(input1.decorators !.map(d => d.name)).toEqual(['Input']); }); + it('should find decorated members on a class when mixing `ctorParameters` and `__decorate`', + () => { + const {program} = makeTestBundleProgram(_('/some_directive_ctor_parameters.js')); + const host = + new Esm2015ReflectionHost(new MockLogger(), false, program.getTypeChecker()); + const classNode = getDeclaration( + program, _('/some_directive_ctor_parameters.js'), 'SomeDirective', + isNamedVariableDeclaration); + const members = host.getMembersOfClass(classNode); + + const input1 = members.find(member => member.name === 'input1') !; + expect(input1.kind).toEqual(ClassMemberKind.Property); + expect(input1.isStatic).toEqual(false); + expect(input1.decorators !.map(d => d.name)).toEqual(['Input']); + }); + it('should find non decorated properties on a class', () => { const {program} = makeTestBundleProgram(_('/some_directive.js')); const host = @@ -292,6 +360,27 @@ runInEachFileSystem(() => { ]); }); + it('should find the decorated constructor parameters when mixing `ctorParameters` and `__decorate`', + () => { + const {program} = makeTestBundleProgram(_('/some_directive_ctor_parameters.js')); + const host = + new Esm2015ReflectionHost(new MockLogger(), false, program.getTypeChecker()); + const classNode = getDeclaration( + program, _('/some_directive_ctor_parameters.js'), 'SomeDirective', + isNamedVariableDeclaration); + const parameters = host.getConstructorParameters(classNode); + + expect(parameters).toBeDefined(); + expect(parameters !.map(parameter => parameter.name)).toEqual([ + '_viewContainer', '_template', 'injected' + ]); + expectTypeValueReferencesForParameters(parameters !, [ + 'ViewContainerRef', + 'TemplateRef', + null, + ]); + }); + describe('(returned parameters `decorators`)', () => { it('should use `getImportOfIdentifier()` to retrieve import info', () => { const mockImportInfo = {} as Import; diff --git a/packages/compiler-cli/ngcc/test/host/esm5_host_import_helper_spec.ts b/packages/compiler-cli/ngcc/test/host/esm5_host_import_helper_spec.ts index 1d6aa4c66e..b467be4e19 100644 --- a/packages/compiler-cli/ngcc/test/host/esm5_host_import_helper_spec.ts +++ b/packages/compiler-cli/ngcc/test/host/esm5_host_import_helper_spec.ts @@ -76,6 +76,43 @@ runInEachFileSystem(() => { return SomeDirective; }()); export { SomeDirective }; + `, + }, + { + name: _('/some_directive_ctor_parameters.js'), + contents: ` + import * as tslib_1 from 'tslib'; + import { Directive, Inject, InjectionToken, Input } from '@angular/core'; + var INJECTED_TOKEN = new InjectionToken('injected'); + var ViewContainerRef = /** @class */ (function () { + function ViewContainerRef() { + } + return ViewContainerRef; + }()); + var TemplateRef = /** @class */ (function () { + function TemplateRef() { + } + return TemplateRef; + }()); + var SomeDirective = /** @class */ (function () { + function SomeDirective(_viewContainer, _template, injected) { + this.input1 = ''; + } + SomeDirective.ctorParameters = function() { return [ + { type: ViewContainerRef, }, + { type: TemplateRef, }, + { type: undefined, decorators: [{ type: Inject, args: [INJECTED_TOKEN,] },] }, + ]; }; + tslib_1.__decorate([ + Input(), + ], SomeDirective.prototype, "input1", void 0); + SomeDirective = tslib_1.__decorate([ + Directive({ selector: '[someDirective]' }), + tslib_1.__param(2, Inject(INJECTED_TOKEN)), + ], SomeDirective); + return SomeDirective; + }()); + export { SomeDirective }; `, }, { @@ -172,6 +209,28 @@ export { SomeDirective }; ]); }); + it('should find the decorators on a class when mixing `ctorParameters` and `__decorate`', + () => { + const {program} = makeTestBundleProgram(_('/some_directive_ctor_parameters.js')); + const host = + new Esm5ReflectionHost(new MockLogger(), false, program.getTypeChecker()); + const classNode = getDeclaration( + program, _('/some_directive_ctor_parameters.js'), 'SomeDirective', + isNamedVariableDeclaration); + const decorators = host.getDecoratorsOfDeclaration(classNode) !; + + expect(decorators).toBeDefined(); + expect(decorators.length).toEqual(1); + + const decorator = decorators[0]; + expect(decorator.name).toEqual('Directive'); + expect(decorator.identifier.getText()).toEqual('Directive'); + expect(decorator.import).toEqual({name: 'Directive', from: '@angular/core'}); + expect(decorator.args !.map(arg => arg.getText())).toEqual([ + '{ selector: \'[someDirective]\' }', + ]); + }); + it('should support decorators being used inside @angular/core', () => { const {program} = makeTestBundleProgram(_('/node_modules/@angular/core/some_directive.js')); @@ -213,6 +272,22 @@ export { SomeDirective }; expect(input1.decorators !.map(d => d.name)).toEqual(['Input']); }); + it('should find decorated members on a class when mixing `ctorParameters` and `__decorate`', + () => { + const {program} = makeTestBundleProgram(_('/some_directive_ctor_parameters.js')); + const host = + new Esm5ReflectionHost(new MockLogger(), false, program.getTypeChecker()); + const classNode = getDeclaration( + program, _('/some_directive_ctor_parameters.js'), 'SomeDirective', + isNamedVariableDeclaration); + const members = host.getMembersOfClass(classNode); + + const input1 = members.find(member => member.name === 'input1') !; + expect(input1.kind).toEqual(ClassMemberKind.Property); + expect(input1.isStatic).toEqual(false); + expect(input1.decorators !.map(d => d.name)).toEqual(['Input']); + }); + it('should find non decorated properties on a class', () => { const {program} = makeTestBundleProgram(_('/some_directive.js')); const host = new Esm5ReflectionHost(new MockLogger(), false, program.getTypeChecker()); @@ -288,6 +363,27 @@ export { SomeDirective }; ]); }); + it('should find the decorated constructor parameters when mixing `ctorParameters` and `__decorate`', + () => { + const {program} = makeTestBundleProgram(_('/some_directive_ctor_parameters.js')); + const host = + new Esm5ReflectionHost(new MockLogger(), false, program.getTypeChecker()); + const classNode = getDeclaration( + program, _('/some_directive_ctor_parameters.js'), 'SomeDirective', + isNamedVariableDeclaration); + const parameters = host.getConstructorParameters(classNode); + + expect(parameters).toBeDefined(); + expect(parameters !.map(parameter => parameter.name)).toEqual([ + '_viewContainer', '_template', 'injected' + ]); + expectTypeValueReferencesForParameters(parameters !, [ + 'ViewContainerRef', + 'TemplateRef', + null, + ]); + }); + describe('(returned parameters `decorators`)', () => { it('should have import information on decorators', () => { const {program} = makeTestBundleProgram(_('/some_directive.js'));