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
This commit is contained in:
Pete Bacon Darwin 2020-10-20 17:29:03 +01:00 committed by Alex Rickabaugh
parent b989ba2502
commit 413b55273b
7 changed files with 135 additions and 19 deletions

View File

@ -14,7 +14,8 @@ import {Declaration, DeclarationKind, Import} from '../../../src/ngtsc/reflectio
import {BundleProgram} from '../packages/bundle_program'; import {BundleProgram} from '../packages/bundle_program';
import {FactoryMap, isDefined} from '../utils'; 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 {Esm5ReflectionHost} from './esm5_host';
import {NgccClassSymbol} from './ngcc_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}; 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 private resolveModuleName(moduleName: string, containingFile: ts.SourceFile): ts.SourceFile
|undefined { |undefined {
if (this.compilerHost.resolveModuleNames) { if (this.compilerHost.resolveModuleNames) {

View File

@ -470,6 +470,27 @@ export class UmdReflectionHost extends Esm5ReflectionHost {
return requireCall.arguments[0].text; 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 private resolveModuleName(moduleName: string, containingFile: ts.SourceFile): ts.SourceFile
|undefined { |undefined {
if (this.compilerHost.resolveModuleNames) { if (this.compilerHost.resolveModuleNames) {

View File

@ -208,6 +208,7 @@ foo.decorators = [
{ type: core.Directive, args: [{ selector: '[ignored]' },] } { type: core.Directive, args: [{ selector: '[ignored]' },] }
]; ];
exports.directives = [foo]; 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 file = getSourceFileOrError(bundle.program, _('/inline_export.js'));
const exportDeclarations = host.getExportsOfModule(file); const exportDeclarations = host.getExportsOfModule(file);
expect(exportDeclarations).not.toBeNull(); expect(exportDeclarations).not.toBeNull();
const decl = exportDeclarations!.get('directives') as InlineDeclaration; const entries: [string, InlineDeclaration][] =
expect(decl).toBeDefined(); Array.from(exportDeclarations!.entries()) as any;
expect(decl.node.getText()).toEqual('exports.directives'); expect(
expect(decl.implementation!.getText()).toEqual('[foo]'); entries.map(
expect(decl.kind).toEqual(DeclarationKind.Inline); ([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', () => { it('should recognize declarations of known TypeScript helpers', () => {

View File

@ -287,6 +287,7 @@ runInEachFileSystem(() => {
{ type: core.Directive, args: [{ selector: '[ignored]' },] } { type: core.Directive, args: [{ selector: '[ignored]' },] }
]; ];
exports.directives = [foo]; exports.directives = [foo];
exports.Inline = (function() { function Inline() {} return Inline; })();
}))); })));
`, `,
}; };
@ -2732,10 +2733,8 @@ runInEachFileSystem(() => {
['e', `e = 'e'`, null], ['e', `e = 'e'`, null],
['DirectiveX', `Directive: FnWithArg<(clazz: any) => any>`, '@angular/core'], ['DirectiveX', `Directive: FnWithArg<(clazz: any) => any>`, '@angular/core'],
[ [
'SomeClass', `SomeClass = (function() { 'SomeClass',
function SomeClass() {} 'SomeClass = (function() {\n function SomeClass() {}\n return SomeClass;\n }())',
return SomeClass;
}())`,
null null
], ],
]); ]);
@ -2824,11 +2823,16 @@ runInEachFileSystem(() => {
const file = getSourceFileOrError(bundle.program, INLINE_EXPORT_FILE.name); const file = getSourceFileOrError(bundle.program, INLINE_EXPORT_FILE.name);
const exportDeclarations = host.getExportsOfModule(file); const exportDeclarations = host.getExportsOfModule(file);
expect(exportDeclarations).not.toBe(null); expect(exportDeclarations).not.toBe(null);
const decl = exportDeclarations!.get('directives') as InlineDeclaration; const entries: [string, InlineDeclaration][] =
expect(decl).toBeDefined(); Array.from(exportDeclarations!.entries()) as any;
expect(decl.node.getText()).toEqual('exports.directives'); expect(
expect(decl.implementation!.getText()).toEqual('[foo]'); entries.map(
expect(decl.kind).toEqual(DeclarationKind.Inline); ([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', () => { it('should recognize declarations of known TypeScript helpers', () => {

View File

@ -467,6 +467,67 @@ runInEachFileSystem(() => {
.not.toThrow(); .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', it('should not be able to evaluate code in external packages when no .d.ts files are present',
() => { () => {
loadTestFiles([ loadTestFiles([

View File

@ -342,10 +342,12 @@ export class StaticInterpreter {
} }
private visitAmbiguousDeclaration(decl: Declaration, declContext: Context) { private visitAmbiguousDeclaration(decl: Declaration, declContext: Context) {
return decl.kind === DeclarationKind.Inline && decl.implementation !== undefined ? return decl.kind === DeclarationKind.Inline && decl.implementation !== undefined &&
// Inline declarations with an `implementation` should be visited as expressions !isDeclaration(decl.implementation) ?
// Inline declarations whose `implementation` is a `ts.Expression` should be visited as
// an expression.
this.visitExpression(decl.implementation, declContext) : this.visitExpression(decl.implementation, declContext) :
// Otherwise just visit the declaration `node` // Otherwise just visit the `node` as a declaration.
this.visitDeclaration(decl.node, declContext); this.visitDeclaration(decl.node, declContext);
} }

View File

@ -625,7 +625,7 @@ export interface DownleveledEnum {
export interface InlineDeclaration extends export interface InlineDeclaration extends
BaseDeclaration<Exclude<DeclarationNode, ts.Declaration>> { BaseDeclaration<Exclude<DeclarationNode, ts.Declaration>> {
kind: DeclarationKind.Inline; kind: DeclarationKind.Inline;
implementation?: ts.Expression; implementation?: DeclarationNode;
} }
/** /**