Sitelet https://github.com/angular/angular/commit/bdaa714
Skip to content

Commit bdaa714

Browse files
petebacondarwinalxhub
petebacondarwin
authored and
alxhub
committed
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
1 parent ca0c670 commit bdaa714

7 files changed

Lines changed: 135 additions & 19 deletions

File tree

‎packages/compiler-cli/ngcc/src/host/commonjs_host.ts‎

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,8 @@ import {Declaration, DeclarationKind, Import} from '../../../src/ngtsc/reflectio
1414
import {BundleProgram} from '../packages/bundle_program';
1515
import {FactoryMap, isDefined} from '../utils';
1616

17-
import {DefinePropertyReexportStatement, ExportDeclaration, ExportsStatement, extractGetterFnExpression, findNamespaceOfIdentifier, findRequireCallReference, isDefinePropertyReexportStatement, isExportsStatement, isExternalImport, isRequireCall, isWildcardReexportStatement, RequireCall, skipAliases, WildcardReexportStatement} from './commonjs_umd_utils';
17+
import {DefinePropertyReexportStatement, ExportDeclaration, ExportsStatement, extractGetterFnExpression, findNamespaceOfIdentifier, findRequireCallReference, isDefinePropertyReexportStatement, isExportsAssignment, isExportsStatement, isExternalImport, isRequireCall, isWildcardReexportStatement, RequireCall, skipAliases, WildcardReexportStatement} from './commonjs_umd_utils';
18+
import {getInnerClassDeclaration, getOuterNodeFromInnerDeclaration} from './esm2015_host';
1819
import {Esm5ReflectionHost} from './esm5_host';
1920
import {NgccClassSymbol} from './ngcc_host';
2021

@@ -216,6 +217,27 @@ export class CommonJsReflectionHost extends Esm5ReflectionHost {
216217
return {node: module, known: null, viaModule, identity: null, kind: DeclarationKind.Concrete};
217218
}
218219

220+
/**
221+
* If this is an IFE then try to grab the outer and inner classes otherwise fallback on the super
222+
* class.
223+
*/
224+
protected getDeclarationOfExpression(expression: ts.Expression): Declaration|null {
225+
const inner = getInnerClassDeclaration(expression);
226+
if (inner !== null) {
227+
const outer = getOuterNodeFromInnerDeclaration(inner);
228+
if (outer !== null && isExportsAssignment(outer)) {
229+
return {
230+
kind: DeclarationKind.Inline,
231+
node: outer.left,
232+
implementation: inner,
233+
known: null,
234+
viaModule: null,
235+
};
236+
}
237+
}
238+
return super.getDeclarationOfExpression(expression);
239+
}
240+
219241
private resolveModuleName(moduleName: string, containingFile: ts.SourceFile): ts.SourceFile
220242
|undefined {
221243
if (this.compilerHost.resolveModuleNames) {

‎packages/compiler-cli/ngcc/src/host/umd_host.ts‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -470,6 +470,27 @@ export class UmdReflectionHost extends Esm5ReflectionHost {
470470
return requireCall.arguments[0].text;
471471
}
472472

473+
/**
474+
* If this is an IIFE then try to grab the outer and inner classes otherwise fallback on the super
475+
* class.
476+
*/
477+
protected getDeclarationOfExpression(expression: ts.Expression): Declaration|null {
478+
const inner = getInnerClassDeclaration(expression);
479+
if (inner !== null) {
480+
const outer = getOuterNodeFromInnerDeclaration(inner);
481+
if (outer !== null && isExportsAssignment(outer)) {
482+
return {
483+
kind: DeclarationKind.Inline,
484+
node: outer.left,
485+
implementation: inner,
486+
known: null,
487+
viaModule: null,
488+
};
489+
}
490+
}
491+
return super.getDeclarationOfExpression(expression);
492+
}
493+
473494
private resolveModuleName(moduleName: string, containingFile: ts.SourceFile): ts.SourceFile
474495
|undefined {
475496
if (this.compilerHost.resolveModuleNames) {

‎packages/compiler-cli/ngcc/test/host/commonjs_host_spec.ts‎

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -208,6 +208,7 @@ foo.decorators = [
208208
{ type: core.Directive, args: [{ selector: '[ignored]' },] }
209209
];
210210
exports.directives = [foo];
211+
exports.Inline = (function() { function Inline() {} return Inline; })();
211212
`,
212213
};
213214

@@ -2422,11 +2423,16 @@ exports.MissingClass2 = MissingClass2;
24222423
const file = getSourceFileOrError(bundle.program, _('/inline_export.js'));
24232424
const exportDeclarations = host.getExportsOfModule(file);
24242425
expect(exportDeclarations).not.toBeNull();
2425-
const decl = exportDeclarations!.get('directives') as InlineDeclaration;
2426-
expect(decl).toBeDefined();
2427-
expect(decl.node.getText()).toEqual('exports.directives');
2428-
expect(decl.implementation!.getText()).toEqual('[foo]');
2429-
expect(decl.kind).toEqual(DeclarationKind.Inline);
2426+
const entries: [string, InlineDeclaration][] =
2427+
Array.from(exportDeclarations!.entries()) as any;
2428+
expect(
2429+
entries.map(
2430+
([name, decl]) =>
2431+
[name, decl.node!.getText(), decl.implementation!.getText(), decl.viaModule]))
2432+
.toEqual([
2433+
['directives', 'exports.directives', '[foo]', null],
2434+
['Inline', 'exports.Inline', 'function Inline() {}', null],
2435+
]);
24302436
});
24312437

24322438
it('should recognize declarations of known TypeScript helpers', () => {

‎packages/compiler-cli/ngcc/test/host/umd_host_spec.ts‎

Lines changed: 13 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -287,6 +287,7 @@ runInEachFileSystem(() => {
287287
{ type: core.Directive, args: [{ selector: '[ignored]' },] }
288288
];
289289
exports.directives = [foo];
290+
exports.Inline = (function() { function Inline() {} return Inline; })();
290291
})));
291292
`,
292293
};
@@ -2732,10 +2733,8 @@ runInEachFileSystem(() => {
27322733
['e', `e = 'e'`, null],
27332734
['DirectiveX', `Directive: FnWithArg<(clazz: any) => any>`, '@angular/core'],
27342735
[
2735-
'SomeClass', `SomeClass = (function() {
2736-
function SomeClass() {}
2737-
return SomeClass;
2738-
}())`,
2736+
'SomeClass',
2737+
'SomeClass = (function() {\n function SomeClass() {}\n return SomeClass;\n }())',
27392738
null
27402739
],
27412740
]);
@@ -2824,11 +2823,16 @@ runInEachFileSystem(() => {
28242823
const file = getSourceFileOrError(bundle.program, INLINE_EXPORT_FILE.name);
28252824
const exportDeclarations = host.getExportsOfModule(file);
28262825
expect(exportDeclarations).not.toBe(null);
2827-
const decl = exportDeclarations!.get('directives') as InlineDeclaration;
2828-
expect(decl).toBeDefined();
2829-
expect(decl.node.getText()).toEqual('exports.directives');
2830-
expect(decl.implementation!.getText()).toEqual('[foo]');
2831-
expect(decl.kind).toEqual(DeclarationKind.Inline);
2826+
const entries: [string, InlineDeclaration][] =
2827+
Array.from(exportDeclarations!.entries()) as any;
2828+
expect(
2829+
entries.map(
2830+
([name, decl]) =>
2831+
[name, decl.node!.getText(), decl.implementation!.getText(), decl.viaModule]))
2832+
.toEqual([
2833+
['directives', 'exports.directives', '[foo]', null],
2834+
['Inline', 'exports.Inline', 'function Inline() {}', null],
2835+
]);
28322836
});
28332837

28342838
it('should recognize declarations of known TypeScript helpers', () => {

‎packages/compiler-cli/ngcc/test/integration/ngcc_spec.ts‎

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -467,6 +467,67 @@ runInEachFileSystem(() => {
467467
.not.toThrow();
468468
});
469469

470+
it('should support inline UMD/CommonJS exports declarations', () => {
471+
// Setup an Angular entry-point in UMD module format that has an inline exports declaration
472+
// referenced by an NgModule.
473+
loadTestFiles([
474+
{
475+
name: _('/node_modules/test-package/package.json'),
476+
contents: '{"name": "test-package", "main": "./index.js", "typings": "./index.d.ts"}'
477+
},
478+
{
479+
name: _('/node_modules/test-package/index.js'),
480+
contents: `
481+
(function (global, factory) {
482+
typeof exports === 'object' && typeof module !== 'undefined' ? factory(exports, require('@angular/core')) :
483+
typeof define === 'function' && define.amd ? define('test', ['exports', 'core'], factory) :
484+
(factory(global.test, global.core));
485+
}(this, (function (exports, core) { 'use strict';
486+
exports.FooModule = /** @class */ (function () {
487+
function FooModule() {}
488+
FooModule = __decorate([
489+
core.NgModule({declarations: exports.declarations})
490+
], FooModule);
491+
return FooModule;
492+
}());
493+
494+
exports.declarations = [exports.FooDirective];
495+
496+
exports.FooDirective = /** @class */ (function () {
497+
function FooDirective() {}
498+
FooDirective = __decorate([
499+
core.Directive({selector: '[foo]'})
500+
], FooDirective);
501+
return FooDirective;
502+
}());
503+
})));
504+
`
505+
},
506+
{
507+
name: _('/node_modules/test-package/index.d.ts'),
508+
contents: `
509+
export declare class FooModule { }
510+
export declare class FooDirective { }
511+
`
512+
},
513+
{name: _('/node_modules/test-package/index.metadata.json'), contents: 'DUMMY DATA'},
514+
]);
515+
516+
expect(() => mainNgcc({
517+
basePath: '/node_modules',
518+
targetEntryPointPath: 'test-package',
519+
propertiesToConsider: ['main'],
520+
}))
521+
.not.toThrow();
522+
523+
const processedFile = fs.readFile(_('/node_modules/test-package/index.js'));
524+
expect(processedFile)
525+
.toContain('FooModule.ɵmod = ɵngcc0.ɵɵdefineNgModule({ type: FooModule });');
526+
expect(processedFile)
527+
.toContain(
528+
'ɵngcc0.ɵɵsetNgModuleScope(FooModule, { declarations: function () { return [exports.FooDirective]; } });');
529+
});
530+
470531
it('should not be able to evaluate code in external packages when no .d.ts files are present',
471532
() => {
472533
loadTestFiles([

‎packages/compiler-cli/src/ngtsc/partial_evaluator/src/interpreter.ts‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -342,10 +342,12 @@ export class StaticInterpreter {
342342
}
343343

344344
private visitAmbiguousDeclaration(decl: Declaration, declContext: Context) {
345-
return decl.kind === DeclarationKind.Inline && decl.implementation !== undefined ?
346-
// Inline declarations with an `implementation` should be visited as expressions
345+
return decl.kind === DeclarationKind.Inline && decl.implementation !== undefined &&
346+
!isDeclaration(decl.implementation) ?
347+
// Inline declarations whose `implementation` is a `ts.Expression` should be visited as
348+
// an expression.
347349
this.visitExpression(decl.implementation, declContext) :
348-
// Otherwise just visit the declaration `node`
350+
// Otherwise just visit the `node` as a declaration.
349351
this.visitDeclaration(decl.node, declContext);
350352
}
351353

‎packages/compiler-cli/src/ngtsc/reflection/src/host.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -625,7 +625,7 @@ export interface DownleveledEnum {
625625
export interface InlineDeclaration extends
626626
BaseDeclaration<Exclude<DeclarationNode, ts.Declaration>> {
627627
kind: DeclarationKind.Inline;
628-
implementation?: ts.Expression;
628+
implementation?: DeclarationNode;
629629
}
630630

631631
/**

0 commit comments

Comments
 (0)