From e666d283dd48323b5f17bae4a3406a4875458080 Mon Sep 17 00:00:00 2001 From: JoostK Date: Sat, 16 Nov 2019 23:57:57 +0100 Subject: [PATCH] fix(ngcc): correctly associate decorators with aliased classes (#33878) In flat bundle formats, multiple classes that have the same name can be suffixed to become unique. In ES5-like bundles this results in the outer declaration from having a different name from the "implementation" declaration within the class' IIFE, as the implementation declaration may not have been suffixed. As an example, the following code would fail to have a `Directive` decorator as ngcc would search for `__decorate` calls that refer to `AliasedDirective$1` by name, whereas the `__decorate` call actually uses the `AliasedDirective` name. ```javascript var AliasedDirective$1 = /** @class */ (function () { function AliasedDirective() {} AliasedDirective = tslib_1.__decorate([ Directive({ selector: '[someDirective]' }), ], AliasedDirective); return AliasedDirective; }()); ``` This commit fixes the problem by not relying on comparing names, but instead finding the declaration and matching it with both the outer and inner declaration. PR Close #33878 --- .../ngcc/src/host/esm2015_host.ts | 31 +++++++++++---- .../host/commonjs_host_import_helper_spec.ts | 30 ++++++++++++++ .../test/host/esm5_host_import_helper_spec.ts | 39 ++++++++++++++++++- .../test/host/umd_host_import_helper_spec.ts | 29 ++++++++++++++ 4 files changed, 119 insertions(+), 10 deletions(-) diff --git a/packages/compiler-cli/ngcc/src/host/esm2015_host.ts b/packages/compiler-cli/ngcc/src/host/esm2015_host.ts index 827eaf5a57..f926f216d8 100644 --- a/packages/compiler-cli/ngcc/src/host/esm2015_host.ts +++ b/packages/compiler-cli/ngcc/src/host/esm2015_host.ts @@ -931,8 +931,21 @@ export class Esm2015ReflectionHost extends TypeScriptReflectionHost implements N // has to be checked to actually correspond with the class symbol. const helperCalls = this.getHelperCallsForClass(classSymbol, ['__decorate']); + const outerDeclaration = classSymbol.declaration.valueDeclaration; + const innerDeclaration = classSymbol.implementation.valueDeclaration; + const matchesClass = (identifier: ts.Identifier) => { + const decl = this.getDeclarationOfIdentifier(identifier); + if (decl === null) { + return false; + } + + // The identifier corresponds with the class if its declaration is either the outer or inner + // declaration. + return decl.node === outerDeclaration || decl.node === innerDeclaration; + }; + for (const helperCall of helperCalls) { - if (isClassDecorateCall(helperCall, classSymbol.name)) { + if (isClassDecorateCall(helperCall, matchesClass)) { // This `__decorate` call is targeting the class itself. const helperArgs = helperCall.arguments[0]; @@ -959,7 +972,7 @@ export class Esm2015ReflectionHost extends TypeScriptReflectionHost implements N (type, index) => getConstructorParamInfo(index).typeExpression = type); } } - } else if (isMemberDecorateCall(helperCall, classSymbol.name)) { + } else if (isMemberDecorateCall(helperCall, matchesClass)) { // The `__decorate` call is targeting a member of the class const helperArgs = helperCall.arguments[0]; const memberName = helperCall.arguments[2].text; @@ -1688,9 +1701,10 @@ export function isAssignment(node: ts.Node): node is ts.AssignmentExpression boolean): call is ts.CallExpression&{arguments: [ts.ArrayLiteralExpression, ts.Expression]} { const helperArgs = call.arguments[0]; if (helperArgs === undefined || !ts.isArrayLiteralExpression(helperArgs)) { @@ -1698,7 +1712,7 @@ export function isClassDecorateCall(call: ts.CallExpression, className: string): } const target = call.arguments[1]; - return target !== undefined && ts.isIdentifier(target) && target.text === className; + return target !== undefined && ts.isIdentifier(target) && matches(target); } /** @@ -1710,9 +1724,10 @@ export function isClassDecorateCall(call: ts.CallExpression, className: string): * ``` * * @param call the call expression that is tested to represent a member decorator call. - * @param className the name of the class that the call needs to correspond with. + * @param matches predicate function to test whether the call is associated with the desired class. */ -export function isMemberDecorateCall(call: ts.CallExpression, className: string): +export function isMemberDecorateCall( + call: ts.CallExpression, matches: (identifier: ts.Identifier) => boolean): call is ts.CallExpression& {arguments: [ts.ArrayLiteralExpression, ts.StringLiteral, ts.StringLiteral]} { const helperArgs = call.arguments[0]; @@ -1722,7 +1737,7 @@ export function isMemberDecorateCall(call: ts.CallExpression, className: string) const target = call.arguments[1]; if (target === undefined || !ts.isPropertyAccessExpression(target) || - !ts.isIdentifier(target.expression) || target.expression.text !== className || + !ts.isIdentifier(target.expression) || !matches(target.expression) || target.name.text !== 'prototype') { return false; } diff --git a/packages/compiler-cli/ngcc/test/host/commonjs_host_import_helper_spec.ts b/packages/compiler-cli/ngcc/test/host/commonjs_host_import_helper_spec.ts index b9bce75af3..29963c56ed 100644 --- a/packages/compiler-cli/ngcc/test/host/commonjs_host_import_helper_spec.ts +++ b/packages/compiler-cli/ngcc/test/host/commonjs_host_import_helper_spec.ts @@ -68,6 +68,15 @@ __decorate([ core.Input(), ], OtherDirective.prototype, "input2", void 0); exports.OtherDirective = OtherDirective; + +var AliasedDirective$1 = (function () { + function AliasedDirective() {} + return AliasedDirective; +}()); +AliasedDirective$1 = __decorate([ + core.Directive({ selector: '[someDirective]' }), +], AliasedDirective$1); +exports.AliasedDirective$1 = AliasedDirective$1; ` }; }); @@ -92,6 +101,27 @@ exports.OtherDirective = OtherDirective; '{ selector: \'[someDirective]\' }', ]); }); + + it('should find the decorators on an aliased class at the top level', () => { + loadFakeCore(getFileSystem()); + loadTestFiles([TOPLEVEL_DECORATORS_FILE]); + const {program, host: compilerHost} = makeTestBundleProgram(TOPLEVEL_DECORATORS_FILE.name); + const host = new CommonJsReflectionHost(new MockLogger(), false, program, compilerHost); + const classNode = getDeclaration( + program, TOPLEVEL_DECORATORS_FILE.name, 'AliasedDirective$1', + isNamedVariableDeclaration); + const decorators = host.getDecoratorsOfDeclaration(classNode) !; + + expect(decorators.length).toEqual(1); + + const decorator = decorators[0]; + expect(decorator.name).toEqual('Directive'); + expect(decorator.identifier !.getText()).toEqual('core.Directive'); + expect(decorator.import).toEqual({name: 'Directive', from: '@angular/core'}); + expect(decorator.args !.map(arg => arg.getText())).toEqual([ + '{ selector: \'[someDirective]\' }', + ]); + }); }); }); }); 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 9abcff4561..c80f5b9d3e 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 @@ -174,12 +174,27 @@ import { Directive } from '@angular/core'; // Note that the IIFE is not in parentheses var SomeDirective = function () { function SomeDirective() {} - // Note that the decorator is combined with the return statment + // Note that the decorator is combined with the return statement return SomeDirective = tslib_1.__decorate([ Directive({ selector: '[someDirective]' }), ], SomeDirective); -}()); +}(); export { SomeDirective }; +`, + }, + { + name: _('/some_aliased_directive.js'), + contents: ` +import * as tslib_1 from 'tslib'; +import { Directive } from '@angular/core'; +var AliasedDirective$1 = /** @class */ (function () { + function AliasedDirective() {} + AliasedDirective = tslib_1.__decorate([ + Directive({ selector: '[someDirective]' }), + ], AliasedDirective); + return AliasedDirective; +}()); +export { AliasedDirective$1 }; `, }, ]; @@ -247,6 +262,26 @@ export { SomeDirective }; }); + it('should find the decorators on an aliased class', () => { + const {program} = makeTestBundleProgram(_('/some_aliased_directive.js')); + const host = new Esm5ReflectionHost(new MockLogger(), false, program.getTypeChecker()); + const classNode = getDeclaration( + program, _('/some_aliased_directive.js'), 'AliasedDirective$1', + 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 find the decorators on a class when mixing `ctorParameters` and `__decorate`', () => { const {program} = makeTestBundleProgram(_('/some_directive_ctor_parameters.js')); diff --git a/packages/compiler-cli/ngcc/test/host/umd_host_import_helper_spec.ts b/packages/compiler-cli/ngcc/test/host/umd_host_import_helper_spec.ts index 1cd4ee7a27..9f6753844f 100644 --- a/packages/compiler-cli/ngcc/test/host/umd_host_import_helper_spec.ts +++ b/packages/compiler-cli/ngcc/test/host/umd_host_import_helper_spec.ts @@ -61,6 +61,15 @@ runInEachFileSystem(() => { return SomeDirective; }()); exports.SomeDirective = SomeDirective; + + var AliasedDirective$1 = /** @class */ (function () { + function AliasedDirective() {} + AliasedDirective = __decorate([ + core.Directive({ selector: '[someDirective]' }), + ], AliasedDirective); + return AliasedDirective; + }()); + exports.AliasedDirective$1 = AliasedDirective$1; })));`, }; }); @@ -85,6 +94,26 @@ runInEachFileSystem(() => { '{ selector: \'[someDirective]\' }', ]); }); + + it('should find the decorators on an aliased class', () => { + loadTestFiles([SOME_DIRECTIVE_FILE]); + const {program, host: compilerHost} = makeTestBundleProgram(SOME_DIRECTIVE_FILE.name); + const host = new UmdReflectionHost(new MockLogger(), false, program, compilerHost); + const classNode = getDeclaration( + program, SOME_DIRECTIVE_FILE.name, 'AliasedDirective$1', 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('core.Directive'); + expect(decorator.import).toEqual({name: 'Directive', from: '@angular/core'}); + expect(decorator.args !.map(arg => arg.getText())).toEqual([ + '{ selector: \'[someDirective]\' }', + ]); + }); }); describe('getMembersOfClass()', () => {