From 413b55273b50ac1007492646eca76a15816b859e Mon Sep 17 00:00:00 2001 From: Pete Bacon Darwin Date: Tue, 20 Oct 2020 17:29:03 +0100 Subject: [PATCH] fix(ngcc): capture UMD/CommonJS inner class implementation node correctly (#39346) Previously, UMD/CommonJS class inline declarations of the form: ```ts exports.Foo = (function() { function Foo(); return Foo; })(); ``` were capturing the whole IIFE as the implementation, rather than the inner class (i.e. `function Foo() {}` in this case). This caused the interpreter to break when it was trying to access such an export, since it would try to evaluate the IIFE rather than treating it as a class declaration. PR Close #39346 --- .../ngcc/src/host/commonjs_host.ts | 24 +++++++- .../compiler-cli/ngcc/src/host/umd_host.ts | 21 +++++++ .../ngcc/test/host/commonjs_host_spec.ts | 16 +++-- .../ngcc/test/host/umd_host_spec.ts | 22 ++++--- .../ngcc/test/integration/ngcc_spec.ts | 61 +++++++++++++++++++ .../partial_evaluator/src/interpreter.ts | 8 ++- .../src/ngtsc/reflection/src/host.ts | 2 +- 7 files changed, 135 insertions(+), 19 deletions(-) diff --git a/packages/compiler-cli/ngcc/src/host/commonjs_host.ts b/packages/compiler-cli/ngcc/src/host/commonjs_host.ts index 4a501a953a..f467d5beee 100644 --- a/packages/compiler-cli/ngcc/src/host/commonjs_host.ts +++ b/packages/compiler-cli/ngcc/src/host/commonjs_host.ts @@ -14,7 +14,8 @@ import {Declaration, DeclarationKind, Import} from '../../../src/ngtsc/reflectio import {BundleProgram} from '../packages/bundle_program'; import {FactoryMap, isDefined} from '../utils'; -import {DefinePropertyReexportStatement, ExportDeclaration, ExportsStatement, extractGetterFnExpression, findNamespaceOfIdentifier, findRequireCallReference, isDefinePropertyReexportStatement, isExportsStatement, isExternalImport, isRequireCall, isWildcardReexportStatement, RequireCall, skipAliases, WildcardReexportStatement} from './commonjs_umd_utils'; +import {DefinePropertyReexportStatement, ExportDeclaration, ExportsStatement, extractGetterFnExpression, findNamespaceOfIdentifier, findRequireCallReference, isDefinePropertyReexportStatement, isExportsAssignment, isExportsStatement, isExternalImport, isRequireCall, isWildcardReexportStatement, RequireCall, skipAliases, WildcardReexportStatement} from './commonjs_umd_utils'; +import {getInnerClassDeclaration, getOuterNodeFromInnerDeclaration} from './esm2015_host'; import {Esm5ReflectionHost} from './esm5_host'; import {NgccClassSymbol} from './ngcc_host'; @@ -216,6 +217,27 @@ export class CommonJsReflectionHost extends Esm5ReflectionHost { return {node: module, known: null, viaModule, identity: null, kind: DeclarationKind.Concrete}; } + /** + * If this is an IFE then try to grab the outer and inner classes otherwise fallback on the super + * class. + */ + protected getDeclarationOfExpression(expression: ts.Expression): Declaration|null { + const inner = getInnerClassDeclaration(expression); + if (inner !== null) { + const outer = getOuterNodeFromInnerDeclaration(inner); + if (outer !== null && isExportsAssignment(outer)) { + return { + kind: DeclarationKind.Inline, + node: outer.left, + implementation: inner, + known: null, + viaModule: null, + }; + } + } + return super.getDeclarationOfExpression(expression); + } + private resolveModuleName(moduleName: string, containingFile: ts.SourceFile): ts.SourceFile |undefined { if (this.compilerHost.resolveModuleNames) { diff --git a/packages/compiler-cli/ngcc/src/host/umd_host.ts b/packages/compiler-cli/ngcc/src/host/umd_host.ts index c56b95b2d6..fa8080d1e4 100644 --- a/packages/compiler-cli/ngcc/src/host/umd_host.ts +++ b/packages/compiler-cli/ngcc/src/host/umd_host.ts @@ -470,6 +470,27 @@ export class UmdReflectionHost extends Esm5ReflectionHost { return requireCall.arguments[0].text; } + /** + * If this is an IIFE then try to grab the outer and inner classes otherwise fallback on the super + * class. + */ + protected getDeclarationOfExpression(expression: ts.Expression): Declaration|null { + const inner = getInnerClassDeclaration(expression); + if (inner !== null) { + const outer = getOuterNodeFromInnerDeclaration(inner); + if (outer !== null && isExportsAssignment(outer)) { + return { + kind: DeclarationKind.Inline, + node: outer.left, + implementation: inner, + known: null, + viaModule: null, + }; + } + } + return super.getDeclarationOfExpression(expression); + } + private resolveModuleName(moduleName: string, containingFile: ts.SourceFile): ts.SourceFile |undefined { if (this.compilerHost.resolveModuleNames) { diff --git a/packages/compiler-cli/ngcc/test/host/commonjs_host_spec.ts b/packages/compiler-cli/ngcc/test/host/commonjs_host_spec.ts index 0ffe3ed485..c548908896 100644 --- a/packages/compiler-cli/ngcc/test/host/commonjs_host_spec.ts +++ b/packages/compiler-cli/ngcc/test/host/commonjs_host_spec.ts @@ -208,6 +208,7 @@ foo.decorators = [ { type: core.Directive, args: [{ selector: '[ignored]' },] } ]; exports.directives = [foo]; +exports.Inline = (function() { function Inline() {} return Inline; })(); `, }; @@ -2422,11 +2423,16 @@ exports.MissingClass2 = MissingClass2; const file = getSourceFileOrError(bundle.program, _('/inline_export.js')); const exportDeclarations = host.getExportsOfModule(file); expect(exportDeclarations).not.toBeNull(); - const decl = exportDeclarations!.get('directives') as InlineDeclaration; - expect(decl).toBeDefined(); - expect(decl.node.getText()).toEqual('exports.directives'); - expect(decl.implementation!.getText()).toEqual('[foo]'); - expect(decl.kind).toEqual(DeclarationKind.Inline); + const entries: [string, InlineDeclaration][] = + Array.from(exportDeclarations!.entries()) as any; + expect( + entries.map( + ([name, decl]) => + [name, decl.node!.getText(), decl.implementation!.getText(), decl.viaModule])) + .toEqual([ + ['directives', 'exports.directives', '[foo]', null], + ['Inline', 'exports.Inline', 'function Inline() {}', null], + ]); }); it('should recognize declarations of known TypeScript helpers', () => { diff --git a/packages/compiler-cli/ngcc/test/host/umd_host_spec.ts b/packages/compiler-cli/ngcc/test/host/umd_host_spec.ts index 8eda983323..add1647568 100644 --- a/packages/compiler-cli/ngcc/test/host/umd_host_spec.ts +++ b/packages/compiler-cli/ngcc/test/host/umd_host_spec.ts @@ -287,6 +287,7 @@ runInEachFileSystem(() => { { type: core.Directive, args: [{ selector: '[ignored]' },] } ]; exports.directives = [foo]; + exports.Inline = (function() { function Inline() {} return Inline; })(); }))); `, }; @@ -2732,10 +2733,8 @@ runInEachFileSystem(() => { ['e', `e = 'e'`, null], ['DirectiveX', `Directive: FnWithArg<(clazz: any) => any>`, '@angular/core'], [ - 'SomeClass', `SomeClass = (function() { - function SomeClass() {} - return SomeClass; - }())`, + 'SomeClass', + 'SomeClass = (function() {\n function SomeClass() {}\n return SomeClass;\n }())', null ], ]); @@ -2824,11 +2823,16 @@ runInEachFileSystem(() => { const file = getSourceFileOrError(bundle.program, INLINE_EXPORT_FILE.name); const exportDeclarations = host.getExportsOfModule(file); expect(exportDeclarations).not.toBe(null); - const decl = exportDeclarations!.get('directives') as InlineDeclaration; - expect(decl).toBeDefined(); - expect(decl.node.getText()).toEqual('exports.directives'); - expect(decl.implementation!.getText()).toEqual('[foo]'); - expect(decl.kind).toEqual(DeclarationKind.Inline); + const entries: [string, InlineDeclaration][] = + Array.from(exportDeclarations!.entries()) as any; + expect( + entries.map( + ([name, decl]) => + [name, decl.node!.getText(), decl.implementation!.getText(), decl.viaModule])) + .toEqual([ + ['directives', 'exports.directives', '[foo]', null], + ['Inline', 'exports.Inline', 'function Inline() {}', null], + ]); }); it('should recognize declarations of known TypeScript helpers', () => { diff --git a/packages/compiler-cli/ngcc/test/integration/ngcc_spec.ts b/packages/compiler-cli/ngcc/test/integration/ngcc_spec.ts index 7208abe4df..0bd2af3e83 100644 --- a/packages/compiler-cli/ngcc/test/integration/ngcc_spec.ts +++ b/packages/compiler-cli/ngcc/test/integration/ngcc_spec.ts @@ -467,6 +467,67 @@ runInEachFileSystem(() => { .not.toThrow(); }); + it('should support inline UMD/CommonJS exports declarations', () => { + // Setup an Angular entry-point in UMD module format that has an inline exports declaration + // referenced by an NgModule. + loadTestFiles([ + { + name: _('/node_modules/test-package/package.json'), + contents: '{"name": "test-package", "main": "./index.js", "typings": "./index.d.ts"}' + }, + { + name: _('/node_modules/test-package/index.js'), + contents: ` + (function (global, factory) { + typeof exports === 'object' && typeof module !== 'undefined' ? factory(exports, require('@angular/core')) : + typeof define === 'function' && define.amd ? define('test', ['exports', 'core'], factory) : + (factory(global.test, global.core)); + }(this, (function (exports, core) { 'use strict'; + exports.FooModule = /** @class */ (function () { + function FooModule() {} + FooModule = __decorate([ + core.NgModule({declarations: exports.declarations}) + ], FooModule); + return FooModule; + }()); + + exports.declarations = [exports.FooDirective]; + + exports.FooDirective = /** @class */ (function () { + function FooDirective() {} + FooDirective = __decorate([ + core.Directive({selector: '[foo]'}) + ], FooDirective); + return FooDirective; + }()); + }))); + ` + }, + { + name: _('/node_modules/test-package/index.d.ts'), + contents: ` + export declare class FooModule { } + export declare class FooDirective { } + ` + }, + {name: _('/node_modules/test-package/index.metadata.json'), contents: 'DUMMY DATA'}, + ]); + + expect(() => mainNgcc({ + basePath: '/node_modules', + targetEntryPointPath: 'test-package', + propertiesToConsider: ['main'], + })) + .not.toThrow(); + + const processedFile = fs.readFile(_('/node_modules/test-package/index.js')); + expect(processedFile) + .toContain('FooModule.ɵmod = ɵngcc0.ɵɵdefineNgModule({ type: FooModule });'); + expect(processedFile) + .toContain( + 'ɵngcc0.ɵɵsetNgModuleScope(FooModule, { declarations: function () { return [exports.FooDirective]; } });'); + }); + it('should not be able to evaluate code in external packages when no .d.ts files are present', () => { loadTestFiles([ diff --git a/packages/compiler-cli/src/ngtsc/partial_evaluator/src/interpreter.ts b/packages/compiler-cli/src/ngtsc/partial_evaluator/src/interpreter.ts index fcabb15032..20f1932055 100644 --- a/packages/compiler-cli/src/ngtsc/partial_evaluator/src/interpreter.ts +++ b/packages/compiler-cli/src/ngtsc/partial_evaluator/src/interpreter.ts @@ -342,10 +342,12 @@ export class StaticInterpreter { } private visitAmbiguousDeclaration(decl: Declaration, declContext: Context) { - return decl.kind === DeclarationKind.Inline && decl.implementation !== undefined ? - // Inline declarations with an `implementation` should be visited as expressions + return decl.kind === DeclarationKind.Inline && decl.implementation !== undefined && + !isDeclaration(decl.implementation) ? + // Inline declarations whose `implementation` is a `ts.Expression` should be visited as + // an expression. this.visitExpression(decl.implementation, declContext) : - // Otherwise just visit the declaration `node` + // Otherwise just visit the `node` as a declaration. this.visitDeclaration(decl.node, declContext); } diff --git a/packages/compiler-cli/src/ngtsc/reflection/src/host.ts b/packages/compiler-cli/src/ngtsc/reflection/src/host.ts index 3ae572f5c8..cbf28be5b8 100644 --- a/packages/compiler-cli/src/ngtsc/reflection/src/host.ts +++ b/packages/compiler-cli/src/ngtsc/reflection/src/host.ts @@ -625,7 +625,7 @@ export interface DownleveledEnum { export interface InlineDeclaration extends BaseDeclaration> { kind: DeclarationKind.Inline; - implementation?: ts.Expression; + implementation?: DeclarationNode; } /**