Skip to content
Permalink
Browse files

fix(ivy): in ngcc, handle inline exports in commonjs code (#32129)

One of the compiler's tasks is to enumerate the exports of a given ES
module. This can happen for example to resolve `foo.bar` where `foo` is a
namespace import:

```typescript
import * as foo from './foo';

@NgModule({
  directives: [foo.DIRECTIVES],
})
```

In this case, the compiler must enumerate the exports of `foo.ts` in order
to evaluate the expression `foo.DIRECTIVES`.

When this operation occurs under ngcc, it must deal with the different
module formats and types of exports that occur. In commonjs code, a problem
arises when certain exports are downleveled.

```typescript
export const DIRECTIVES = [
  FooDir,
  BarDir,
];
```

can be downleveled to:

```javascript
exports.DIRECTIVES = [
  FooDir,
  BarDir,
```

Previously, ngtsc and ngcc expected that any export would have an associated
`ts.Declaration` node. `export class`, `export function`, etc. all retain
`ts.Declaration`s even when downleveled. But the `export const` construct
above does not. Therefore, ngcc would not detect `DIRECTIVES` as an export
of `foo.ts`, and the evaluation of `foo.DIRECTIVES` would therefore fail.

To solve this problem, the core concept of an exported `Declaration`
according to the `ReflectionHost` API is split into a `ConcreteDeclaration`
which has a `ts.Declaration`, and an `InlineDeclaration` which instead has
a `ts.Expression`. Differentiating between these allows ngcc to return an
`InlineDeclaration` for `DIRECTIVES` and correctly keep track of this
export.

PR Close #32129
  • Loading branch information...
alxhub authored and AndrewKushnir committed Aug 13, 2019
1 parent 69ce1c2 commit 02bab8cf90bba06ee723e87938e436918dc86bb0
@@ -9,7 +9,7 @@ import * as ts from 'typescript';

import {ReferencesRegistry} from '../../../src/ngtsc/annotations';
import {Reference} from '../../../src/ngtsc/imports';
import {ClassDeclaration, Declaration} from '../../../src/ngtsc/reflection';
import {ClassDeclaration, ConcreteDeclaration} from '../../../src/ngtsc/reflection';
import {ModuleWithProvidersFunction, NgccReflectionHost} from '../host/ngcc_host';
import {hasNameIdentifier, isDefined} from '../utils';

@@ -23,7 +23,7 @@ export interface ModuleWithProvidersInfo {
/**
* The NgModule class declaration (in the .d.ts file) to add as a type parameter.
*/
ngModule: Declaration<ClassDeclaration>;
ngModule: ConcreteDeclaration<ClassDeclaration>;
}

export type ModuleWithProvidersAnalyses = Map<ts.SourceFile, ModuleWithProvidersInfo[]>;
@@ -9,7 +9,7 @@
import * as ts from 'typescript';
import {ReferencesRegistry} from '../../../src/ngtsc/annotations';
import {Reference} from '../../../src/ngtsc/imports';
import {Declaration, ReflectionHost} from '../../../src/ngtsc/reflection';
import {ConcreteDeclaration, ReflectionHost} from '../../../src/ngtsc/reflection';
import {hasNameIdentifier} from '../utils';

/**
@@ -20,7 +20,7 @@ import {hasNameIdentifier} from '../utils';
* from libraries that are compiled by ngcc.
*/
export class NgccReferencesRegistry implements ReferencesRegistry {
private map = new Map<ts.Identifier, Declaration>();
private map = new Map<ts.Identifier, ConcreteDeclaration>();

constructor(private host: ReflectionHost) {}

@@ -34,7 +34,7 @@ export class NgccReferencesRegistry implements ReferencesRegistry {
// Only store relative references. We are not interested in literals.
if (ref.bestGuessOwningModule === null && hasNameIdentifier(ref.node)) {
const declaration = this.host.getDeclarationOfIdentifier(ref.node.name);
if (declaration && hasNameIdentifier(declaration.node)) {
if (declaration && declaration.node !== null && hasNameIdentifier(declaration.node)) {
this.map.set(declaration.node.name, declaration);
}
}
@@ -45,5 +45,5 @@ export class NgccReferencesRegistry implements ReferencesRegistry {
* Create and return a mapping for the registered resolved references.
* @returns A map of reference identifiers to reference declarations.
*/
getDeclarationMap(): Map<ts.Identifier, Declaration> { return this.map; }
getDeclarationMap(): Map<ts.Identifier, ConcreteDeclaration> { return this.map; }
}
@@ -8,7 +8,7 @@
import * as ts from 'typescript';

import {AbsoluteFsPath, absoluteFromSourceFile} from '../../../src/ngtsc/file_system';
import {Declaration} from '../../../src/ngtsc/reflection';
import {ConcreteDeclaration} from '../../../src/ngtsc/reflection';
import {NgccReflectionHost} from '../host/ngcc_host';
import {hasNameIdentifier, isDefined} from '../utils';
import {NgccReferencesRegistry} from './ngcc_references_registry';
@@ -40,15 +40,15 @@ export class PrivateDeclarationsAnalyzer {

private getPrivateDeclarations(
rootFiles: ts.SourceFile[],
declarations: Map<ts.Identifier, Declaration>): PrivateDeclarationsAnalyses {
const privateDeclarations: Map<ts.Identifier, Declaration> = new Map(declarations);
declarations: Map<ts.Identifier, ConcreteDeclaration>): PrivateDeclarationsAnalyses {
const privateDeclarations: Map<ts.Identifier, ConcreteDeclaration> = new Map(declarations);
const exportAliasDeclarations: Map<ts.Identifier, string> = new Map();

rootFiles.forEach(f => {
const exports = this.host.getExportsOfModule(f);
if (exports) {
exports.forEach((declaration, exportedName) => {
if (hasNameIdentifier(declaration.node)) {
if (declaration.node !== null && hasNameIdentifier(declaration.node)) {
if (privateDeclarations.has(declaration.node.name)) {
const privateDeclaration = privateDeclarations.get(declaration.node.name) !;
if (privateDeclaration.node !== declaration.node) {
@@ -92,9 +92,7 @@ export class CommonJsReflectionHost extends Esm5ReflectionHost {
for (const statement of this.getModuleStatements(sourceFile)) {
if (isCommonJsExportStatement(statement)) {
const exportDeclaration = this.extractCommonJsExportDeclaration(statement);
if (exportDeclaration !== null) {
moduleMap.set(exportDeclaration.name, exportDeclaration.declaration);
}
moduleMap.set(exportDeclaration.name, exportDeclaration.declaration);
} else if (isReexportStatement(statement)) {
const reexports = this.extractCommonJsReexports(statement, sourceFile);
for (const reexport of reexports) {
@@ -106,14 +104,22 @@ export class CommonJsReflectionHost extends Esm5ReflectionHost {
}

private extractCommonJsExportDeclaration(statement: CommonJsExportStatement):
CommonJsExportDeclaration|null {
CommonJsExportDeclaration {
const exportExpression = statement.expression.right;
const declaration = this.getDeclarationOfExpression(exportExpression);
if (declaration === null) {
return null;
}
const name = statement.expression.left.name.text;
return {name, declaration};
if (declaration !== null) {
return {name, declaration};
} else {
return {
name,
declaration: {
node: null,
expression: exportExpression,
viaModule: null,
},
};
}
}

private extractCommonJsReexports(statement: ReexportStatement, containingFile: ts.SourceFile):
@@ -126,8 +132,14 @@ export class CommonJsReflectionHost extends Esm5ReflectionHost {
const viaModule = stripExtension(importedFile.fileName);
const importedExports = this.getExportsOfModule(importedFile);
if (importedExports !== null) {
importedExports.forEach(
(decl, name) => reexports.push({name, declaration: {node: decl.node, viaModule}}));
importedExports.forEach((decl, name) => {
if (decl.node !== null) {
reexports.push({name, declaration: {node: decl.node, viaModule}});
} else {
reexports.push(
{name, declaration: {node: null, expression: decl.expression, viaModule}});
}
});
}
}
return reexports;
@@ -9,7 +9,7 @@
import * as ts from 'typescript';

import {AbsoluteFsPath} from '../../../src/ngtsc/file_system';
import {ClassDeclaration, ClassMember, ClassMemberKind, ClassSymbol, CtorParameter, Declaration, Decorator, TypeScriptReflectionHost, isDecoratorIdentifier, reflectObjectLiteral} from '../../../src/ngtsc/reflection';
import {ClassDeclaration, ClassMember, ClassMemberKind, ClassSymbol, ConcreteDeclaration, CtorParameter, Declaration, Decorator, TypeScriptReflectionHost, isDecoratorIdentifier, reflectObjectLiteral} from '../../../src/ngtsc/reflection';
import {Logger} from '../logging/logger';
import {BundleProgram} from '../packages/bundle_program';
import {findAll, getNameText, hasNameIdentifier, isDefined, stripDollarSuffix} from '../utils';
@@ -243,7 +243,7 @@ export class Esm2015ReflectionHost extends TypeScriptReflectionHost implements N

// The identifier may have been of an additional class assignment such as `MyClass_1` that was
// present as alias for `MyClass`. If so, resolve such aliases to their original declaration.
if (superDeclaration !== null) {
if (superDeclaration !== null && superDeclaration.node !== null) {
const aliasedIdentifier = this.resolveAliasedClassIdentifier(superDeclaration.node);
if (aliasedIdentifier !== null) {
return this.getDeclarationOfIdentifier(aliasedIdentifier);
@@ -409,6 +409,9 @@ export class Esm2015ReflectionHost extends TypeScriptReflectionHost implements N
if (!exports) return [];
const infos: ModuleWithProvidersFunction[] = [];
exports.forEach((declaration, name) => {
if (declaration.node === null) {
return;
}
if (this.isClass(declaration.node)) {
this.getMembersOfClass(declaration.node).forEach(member => {
if (member.isStatic) {
@@ -497,7 +500,7 @@ export class Esm2015ReflectionHost extends TypeScriptReflectionHost implements N
const aliasedIdentifier = initializer.left;

const aliasedDeclaration = this.getDeclarationOfIdentifier(aliasedIdentifier);
if (aliasedDeclaration === null) {
if (aliasedDeclaration === null || aliasedDeclaration.node === null) {
throw new Error(
`Unable to locate declaration of ${aliasedIdentifier.text} in "${statement.getText()}"`);
}
@@ -1407,7 +1410,7 @@ export class Esm2015ReflectionHost extends TypeScriptReflectionHost implements N
}

const ngModuleDeclaration = this.getDeclarationOfExpression(ngModuleValue);
if (!ngModuleDeclaration) {
if (!ngModuleDeclaration || ngModuleDeclaration.node === null) {
throw new Error(
`Cannot find a declaration for NgModule ${ngModuleValue.getText()} referenced in "${declaration!.getText()}"`);
}
@@ -1416,7 +1419,7 @@ export class Esm2015ReflectionHost extends TypeScriptReflectionHost implements N
}
return {
name,
ngModule: ngModuleDeclaration as Declaration<ClassDeclaration>, declaration, container
ngModule: ngModuleDeclaration as ConcreteDeclaration<ClassDeclaration>, declaration, container
};
}

@@ -1430,7 +1433,7 @@ export class Esm2015ReflectionHost extends TypeScriptReflectionHost implements N
}

const namespaceDecl = this.getDeclarationOfIdentifier(expression.expression);
if (!namespaceDecl || !ts.isSourceFile(namespaceDecl.node)) {
if (!namespaceDecl || namespaceDecl.node === null || !ts.isSourceFile(namespaceDecl.node)) {
return null;
}

@@ -140,7 +140,7 @@ export class Esm5ReflectionHost extends Esm2015ReflectionHost {
const outerClassNode = getClassDeclarationFromInnerFunctionDeclaration(id.parent);
const declaration = super.getDeclarationOfIdentifier(outerClassNode ? outerClassNode.name : id);

if (!declaration || !ts.isVariableDeclaration(declaration.node) ||
if (!declaration || declaration.node === null || !ts.isVariableDeclaration(declaration.node) ||
declaration.node.initializer !== undefined ||
// VariableDeclaration => VariableDeclarationList => VariableStatement => IIFE Block
!ts.isBlock(declaration.node.parent.parent.parent)) {
@@ -6,7 +6,8 @@
* found in the LICENSE file at https://angular.io/license
*/
import * as ts from 'typescript';
import {ClassDeclaration, ClassSymbol, Declaration, Decorator, ReflectionHost} from '../../../src/ngtsc/reflection';

import {ClassDeclaration, ClassSymbol, ConcreteDeclaration, Decorator, ReflectionHost} from '../../../src/ngtsc/reflection';

export const PRE_R3_MARKER = '__PRE_R3__';
export const POST_R3_MARKER = '__POST_R3__';
@@ -39,7 +40,7 @@ export interface ModuleWithProvidersFunction {
* The declaration of the class that the `ngModule` property on the `ModuleWithProviders` object
* refers to.
*/
ngModule: Declaration<ClassDeclaration>;
ngModule: ConcreteDeclaration<ClassDeclaration>;
}

/**
@@ -72,7 +72,7 @@ export class UndecoratedParentMigration implements Migration {
}

const baseClazz = host.reflectionHost.getDeclarationOfIdentifier(baseClassExpr) !.node;
if (!isClassDeclaration(baseClazz)) {
if (baseClazz === null || !isClassDeclaration(baseClazz)) {
return null;
}

@@ -9,7 +9,7 @@ import * as ts from 'typescript';

import {absoluteFrom, getFileSystem, getSourceFileOrError} from '../../../src/ngtsc/file_system';
import {TestFile, runInEachFileSystem} from '../../../src/ngtsc/file_system/testing';
import {ClassMemberKind, CtorParameter, Import, isNamedClassDeclaration, isNamedFunctionDeclaration, isNamedVariableDeclaration} from '../../../src/ngtsc/reflection';
import {ClassMemberKind, CtorParameter, Import, InlineDeclaration, isNamedClassDeclaration, isNamedFunctionDeclaration, isNamedVariableDeclaration} from '../../../src/ngtsc/reflection';
import {getDeclaration} from '../../../src/ngtsc/testing';
import {loadFakeCore, loadTestFiles} from '../../../test/helpers';
import {CommonJsReflectionHost} from '../../src/host/commonjs_host';
@@ -31,6 +31,7 @@ runInEachFileSystem(() => {
let SIMPLE_ES2015_CLASS_FILE: TestFile;
let SIMPLE_CLASS_FILE: TestFile;
let FOO_FUNCTION_FILE: TestFile;
let INLINE_EXPORT_FILE: TestFile;
let INVALID_DECORATORS_FILE: TestFile;
let INVALID_DECORATOR_ARGS_FILE: TestFile;
let INVALID_PROP_DECORATORS_FILE: TestFile;
@@ -164,6 +165,18 @@ exports.foo = foo;
`,
};

INLINE_EXPORT_FILE = {
name: _('/inline_export.js'),
contents: `
var core = require('@angular/core');
function foo() {}
foo.decorators = [
{ type: core.Directive, args: [{ selector: '[ignored]' },] }
];
exports.directives = [foo];
`,
};

INVALID_DECORATORS_FILE = {
name: _('/invalid_decorators.js'),
contents: `
@@ -1629,7 +1642,7 @@ exports.ExternalModule = ExternalModule;
const exportDeclarations = host.getExportsOfModule(file);
expect(exportDeclarations).not.toBe(null);
expect(Array.from(exportDeclarations !.entries())
.map(entry => [entry[0], entry[1].node.getText(), entry[1].viaModule]))
.map(entry => [entry[0], entry[1].node !.getText(), entry[1].viaModule]))
.toEqual([
['Directive', `Directive: FnWithArg<(clazz: any) => any>`, '@angular/core'],
['a', `a = 'a'`, './a_module'],
@@ -1655,7 +1668,7 @@ exports.ExternalModule = ExternalModule;
const exportDeclarations = host.getExportsOfModule(file);
expect(exportDeclarations).not.toBe(null);
expect(Array.from(exportDeclarations !.entries())
.map(entry => [entry[0], entry[1].node.getText(), entry[1].viaModule]))
.map(entry => [entry[0], entry[1].node !.getText(), entry[1].viaModule]))
.toEqual([
['Directive', `Directive: FnWithArg<(clazz: any) => any>`, _('/b_module')],
['a', `a = 'a'`, _('/b_module')],
@@ -1673,6 +1686,19 @@ exports.ExternalModule = ExternalModule;
['xtra2', `xtra2 = 'xtra2'`, _('/xtra_module')],
]);
});

it('should handle inline exports', () => {
loadTestFiles([INLINE_EXPORT_FILE]);
const {program, host: compilerHost} = makeTestBundleProgram(_('/inline_export.js'));
const host = new CommonJsReflectionHost(new MockLogger(), false, program, compilerHost);
const file = getSourceFileOrError(program, _('/inline_export.js'));
const exportDeclarations = host.getExportsOfModule(file);
expect(exportDeclarations).not.toBeNull();
const decl = exportDeclarations !.get('directives') as InlineDeclaration;
expect(decl).not.toBeUndefined();
expect(decl.node).toBeNull();
expect(decl.expression).toBeDefined();
});
});

describe('getClassSymbol()', () => {
@@ -1538,8 +1538,9 @@ runInEachFileSystem(() => {
'SomeClass',
]);

const values = Array.from(exportDeclarations !.values())
.map(declaration => [declaration.node.getText(), declaration.viaModule]);
const values =
Array.from(exportDeclarations !.values())
.map(declaration => [declaration.node !.getText(), declaration.viaModule]);
expect(values).toEqual([
[`Directive: FnWithArg<(clazz: any) => any>`, null],
[`a = 'a'`, null],
@@ -399,7 +399,7 @@ export { SomeDirective };

const declaration = host.getDeclarationOfIdentifier(ngModuleRef !);
expect(declaration).not.toBe(null);
expect(declaration !.node.getText()).toContain('function HttpClientXsrfModule()');
expect(declaration !.node !.getText()).toContain('function HttpClientXsrfModule()');
});
});
describe('getVariableValue', () => {
@@ -1842,8 +1842,9 @@ runInEachFileSystem(() => {
'SomeClass',
]);

const values = Array.from(exportDeclarations !.values())
.map(declaration => [declaration.node.getText(), declaration.viaModule]);
const values =
Array.from(exportDeclarations !.values())
.map(declaration => [declaration.node !.getText(), declaration.viaModule]);
expect(values).toEqual([
[`Directive: FnWithArg<(clazz: any) => any>`, null],
[`a = 'a'`, null],
@@ -1741,7 +1741,7 @@ runInEachFileSystem(() => {
const exportDeclarations = host.getExportsOfModule(file);
expect(exportDeclarations).not.toBe(null);
expect(Array.from(exportDeclarations !.entries())
.map(entry => [entry[0], entry[1].node.getText(), entry[1].viaModule]))
.map(entry => [entry[0], entry[1].node !.getText(), entry[1].viaModule]))
.toEqual([
['Directive', `Directive: FnWithArg<(clazz: any) => any>`, '@angular/core'],
['a', `a = 'a'`, '/a_module'],
@@ -176,7 +176,13 @@ export class AbsoluteModuleStrategy implements ReferenceEmitStrategy {
return null;
}
const exportMap = new Map<ts.Declaration, string>();
exports.forEach((declaration, name) => { exportMap.set(declaration.node, name); });
exports.forEach((declaration, name) => {
// It's okay to skip inline declarations, since by definition they're not target-able with a
// ts.Declaration anyway.
if (declaration.node !== null) {
exportMap.set(declaration.node, name);
}
});
return exportMap;
}
}

0 comments on commit 02bab8c

Please sign in to comment.
You can’t perform that action at this time.