honeymoose
/
angular-cn
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

fix(ngcc): `viaModule` should be `null` for local imports (#36989) 

In the CommonJS and UMD reflection hosts, the logic for computing the
`viaModule` property of `Declaration` objects was not correct for some
cases when getting the exports of modules.

In these cases it was setting `viaModule` to the path of the local module
rather than `null`.

PR Close #36989
This commit is contained in:
Pete Bacon Darwin 2020-05-12 08:20:01 +01:00 committed by Kara Erickson
parent 2486db7c2b
commit d268d2ad85
4 changed files with 73 additions and 70 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

75
packages/compiler-cli/ngcc/src/host/commonjs_host.ts
View File

@ -12,7 +12,7 @@ import {absoluteFrom} from '../../../src/ngtsc/file_system';
import {Declaration, Import} from '../../../src/ngtsc/reflection'; import {Declaration, Import} from '../../../src/ngtsc/reflection';
import {Logger} from '../logging/logger'; import {Logger} from '../logging/logger';
import {BundleProgram} from '../packages/bundle_program'; import {BundleProgram} from '../packages/bundle_program';
import {FactoryMap, getTsHelperFnFromIdentifier, isDefined, stripExtension} from '../utils'; import {FactoryMap, isDefined} from '../utils';
import {ExportDeclaration, ExportStatement, findNamespaceOfIdentifier, findRequireCallReference, isExportStatement, isRequireCall, isWildcardReexportStatement, RequireCall, WildcardReexportStatement} from './commonjs_umd_utils'; import {ExportDeclaration, ExportStatement, findNamespaceOfIdentifier, findRequireCallReference, isExportStatement, isRequireCall, isWildcardReexportStatement, RequireCall, WildcardReexportStatement} from './commonjs_umd_utils';
import {Esm5ReflectionHost} from './esm5_host'; import {Esm5ReflectionHost} from './esm5_host';
@ -43,7 +43,7 @@ export class CommonJsReflectionHost extends Esm5ReflectionHost {
} }
getDeclarationOfIdentifier(id: ts.Identifier): Declaration|null { getDeclarationOfIdentifier(id: ts.Identifier): Declaration|null {
return this.getCommonJsImportedDeclaration(id) || super.getDeclarationOfIdentifier(id); return this.getCommonJsModuleDeclaration(id) || super.getDeclarationOfIdentifier(id);
} }
getExportsOfModule(module: ts.Node): Map<string, Declaration>|null { getExportsOfModule(module: ts.Node): Map<string, Declaration>|null {
@ -99,7 +99,7 @@ export class CommonJsReflectionHost extends Esm5ReflectionHost {
const moduleMap = new Map<string, Declaration>(); const moduleMap = new Map<string, Declaration>();
for (const statement of this.getModuleStatements(sourceFile)) { for (const statement of this.getModuleStatements(sourceFile)) {
if (isExportStatement(statement)) { if (isExportStatement(statement)) {
const exportDeclaration = this.extractCommonJsExportDeclaration(statement); const exportDeclaration = this.extractBasicCommonJsExportDeclaration(statement);
moduleMap.set(exportDeclaration.name, exportDeclaration.declaration); moduleMap.set(exportDeclaration.name, exportDeclaration.declaration);
} else if (isWildcardReexportStatement(statement)) { } else if (isWildcardReexportStatement(statement)) {
const reexports = this.extractCommonJsWildcardReexports(statement, sourceFile); const reexports = this.extractCommonJsWildcardReexports(statement, sourceFile);
@ -111,23 +111,10 @@ export class CommonJsReflectionHost extends Esm5ReflectionHost {
return moduleMap; return moduleMap;
} }
private extractCommonJsExportDeclaration(statement: ExportStatement): ExportDeclaration { private extractBasicCommonJsExportDeclaration(statement: ExportStatement): ExportDeclaration {
const exportExpression = statement.expression.right; const exportExpression = statement.expression.right;
const declaration = this.getDeclarationOfExpression(exportExpression);
const name = statement.expression.left.name.text; const name = statement.expression.left.name.text;
if (declaration !== null) { return this.extractCommonJsExportDeclaration(name, exportExpression);
return {name, declaration};
} else {
return {
name,
declaration: {
node: null,
known: null,
expression: exportExpression,
viaModule: null,
},
};
}
} }
private extractCommonJsWildcardReexports( private extractCommonJsWildcardReexports(
@ -152,18 +139,13 @@ export class CommonJsReflectionHost extends Esm5ReflectionHost {
return []; return [];
} }
const viaModule = stripExtension(importedFile.fileName); const viaModule = importPath.startsWith('./') ? null : importPath;
const reexports: ExportDeclaration[] = []; const reexports: ExportDeclaration[] = [];
importedExports.forEach((decl, name) => { importedExports.forEach((declaration, name) => {
if (decl.node !== null) { if (viaModule !== null && declaration.viaModule === null) {
reexports.push({ declaration = {...declaration, viaModule};
name,
declaration: {node: decl.node, known: null, viaModule, identity: decl.identity}
});
} else {
reexports.push(
{name, declaration: {node: null, known: null, expression: decl.expression, viaModule}});
} }
reexports.push({name, declaration});
}); });
return reexports; return reexports;
} }
@ -175,19 +157,38 @@ export class CommonJsReflectionHost extends Esm5ReflectionHost {
return nsIdentifier && findRequireCallReference(nsIdentifier, this.checker); return nsIdentifier && findRequireCallReference(nsIdentifier, this.checker);
} }
private getCommonJsImportedDeclaration(id: ts.Identifier): Declaration|null { private extractCommonJsExportDeclaration(name: string, expression: ts.Expression):
const importInfo = this.getImportOfIdentifier(id); ExportDeclaration {
if (importInfo === null) { const declaration = this.getDeclarationOfExpression(expression);
return null; if (declaration !== null) {
return {name, declaration};
} else {
return {
name,
declaration: {node: null, known: null, expression, viaModule: null},
};
}
} }
const importedFile = this.resolveModuleName(importInfo.from, id.getSourceFile()); /**
if (importedFile === undefined) { * Handle the case where the identifier represents a reference to a whole CommonJS
* module, i.e. the result of a call to `require(...)`.
*
* @param id the identifier whose declaration we are looking for.
* @returns a declaration if `id` refers to a CommonJS module, or `null` otherwise.
*/
private getCommonJsModuleDeclaration(id: ts.Identifier): Declaration|null {
const requireCall = findRequireCallReference(id, this.checker);
if (requireCall === null) {
return null; return null;
} }
const importPath = requireCall.arguments[0].text;
const viaModule = !importInfo.from.startsWith('.') ? importInfo.from : null; const module = this.resolveModuleName(importPath, id.getSourceFile());
return {node: importedFile, known: getTsHelperFnFromIdentifier(id), viaModule, identity: null}; if (module === undefined) {
return null;
}
const viaModule = importPath.startsWith('./') ? null : importPath;
return {node: module, known: null, viaModule, identity: null};
} }
private resolveModuleName(moduleName: string, containingFile: ts.SourceFile): ts.SourceFile private resolveModuleName(moduleName: string, containingFile: ts.SourceFile): ts.SourceFile

2
packages/compiler-cli/ngcc/src/host/commonjs_umd_utils.ts
View File

@ -56,7 +56,7 @@ export interface RequireCall extends ts.CallExpression {
* `ts.Identifier` corresponding to `<namespace>` will be returned). Otherwise return `null`. * `ts.Identifier` corresponding to `<namespace>` will be returned). Otherwise return `null`.
*/ */
export function findNamespaceOfIdentifier(id: ts.Identifier): ts.Identifier|null { export function findNamespaceOfIdentifier(id: ts.Identifier): ts.Identifier|null {
return id.parent && ts.isPropertyAccessExpression(id.parent) && return id.parent && ts.isPropertyAccessExpression(id.parent) && id.parent.name === id &&
ts.isIdentifier(id.parent.expression) ? ts.isIdentifier(id.parent.expression) ?
id.parent.expression : id.parent.expression :
null; null;

51
packages/compiler-cli/ngcc/test/host/commonjs_host_spec.ts
View File

@ -1649,7 +1649,7 @@ exports.MissingClass2 = MissingClass2;
bundle.program, SOME_DIRECTIVE_FILE.name, 'SomeDirective', bundle.program, SOME_DIRECTIVE_FILE.name, 'SomeDirective',
isNamedVariableDeclaration); isNamedVariableDeclaration);
const classDecorators = host.getDecoratorsOfDeclaration(classNode)!; const classDecorators = host.getDecoratorsOfDeclaration(classNode)!;
const identifierOfDirective = const namespaceIdentifier =
(((classDecorators[0].node as ts.ObjectLiteralExpression).properties[0] as (((classDecorators[0].node as ts.ObjectLiteralExpression).properties[0] as
ts.PropertyAssignment) ts.PropertyAssignment)
.initializer as ts.PropertyAccessExpression) .initializer as ts.PropertyAccessExpression)
@ -1657,7 +1657,7 @@ exports.MissingClass2 = MissingClass2;
const expectedDeclarationNode = const expectedDeclarationNode =
getSourceFileOrError(bundle.program, _('/node_modules/@angular/core/index.d.ts')); getSourceFileOrError(bundle.program, _('/node_modules/@angular/core/index.d.ts'));
const actualDeclaration = host.getDeclarationOfIdentifier(identifierOfDirective); const actualDeclaration = host.getDeclarationOfIdentifier(namespaceIdentifier);
expect(actualDeclaration).not.toBe(null); expect(actualDeclaration).not.toBe(null);
expect(actualDeclaration!.node).toBe(expectedDeclarationNode); expect(actualDeclaration!.node).toBe(expectedDeclarationNode);
expect(actualDeclaration!.viaModule).toBe('@angular/core'); expect(actualDeclaration!.viaModule).toBe('@angular/core');
@ -1858,10 +1858,11 @@ exports.MissingClass2 = MissingClass2;
const bundle = makeTestBundleProgram(testFile.name); const bundle = makeTestBundleProgram(testFile.name);
const host = const host =
createHost(bundle, new CommonJsReflectionHost(new MockLogger(), false, bundle)); createHost(bundle, new CommonJsReflectionHost(new MockLogger(), false, bundle));
const tslibSourceFile = getSourceFileOrError(bundle.program, tslibFile.name);
const testForHelper = const testForHelper = createTestForTsHelper(
createTestForTsHelper(bundle.program, host, testFile, () => tslibSourceFile); bundle.program, host, testFile,
helperName => getDeclaration(
bundle.program, tslibFile.name, helperName, ts.isFunctionDeclaration));
testForHelper('a', '__assign', KnownDeclaration.TsHelperAssign, 'tslib'); testForHelper('a', '__assign', KnownDeclaration.TsHelperAssign, 'tslib');
testForHelper('b', '__spread', KnownDeclaration.TsHelperSpread, 'tslib'); testForHelper('b', '__spread', KnownDeclaration.TsHelperSpread, 'tslib');
@ -2093,20 +2094,20 @@ exports.MissingClass2 = MissingClass2;
expect(Array.from(exportDeclarations!.entries()) 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([ .toEqual([
['Directive', `Directive: FnWithArg<(clazz: any) => any>`, _('/b_module')], ['Directive', `Directive: FnWithArg<(clazz: any) => any>`, '@angular/core'],
['a', `a = 'a'`, _('/b_module')], ['a', `a = 'a'`, null],
['b', `b = a_module.a`, _('/b_module')], ['b', `b = a_module.a`, null],
['c', `a = 'a'`, _('/b_module')], ['c', `a = 'a'`, null],
['d', `b = a_module.a`, _('/b_module')], ['d', `b = a_module.a`, null],
['e', `e = 'e'`, _('/b_module')], ['e', `e = 'e'`, null],
['DirectiveX', `Directive: FnWithArg<(clazz: any) => any>`, _('/b_module')], ['DirectiveX', `Directive: FnWithArg<(clazz: any) => any>`, '@angular/core'],
[ [
'SomeClass', 'SomeClass',
`SomeClass = (function() {\n function SomeClass() {}\n return SomeClass;\n}())`, `SomeClass = (function() {\n function SomeClass() {}\n return SomeClass;\n}())`,
_('/b_module') null
], ],
['xtra1', `xtra1 = 'xtra1'`, _('/xtra_module')], ['xtra1', `xtra1 = 'xtra1'`, null],
['xtra2', `xtra2 = 'xtra2'`, _('/xtra_module')], ['xtra2', `xtra2 = 'xtra2'`, null],
]); ]);
}); });
@ -2123,20 +2124,20 @@ exports.MissingClass2 = MissingClass2;
expect(Array.from(exportDeclarations!.entries()) 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([ .toEqual([
['Directive', `Directive: FnWithArg<(clazz: any) => any>`, _('/b_module')], ['Directive', `Directive: FnWithArg<(clazz: any) => any>`, '@angular/core'],
['a', `a = 'a'`, _('/b_module')], ['a', `a = 'a'`, null],
['b', `b = a_module.a`, _('/b_module')], ['b', `b = a_module.a`, null],
['c', `a = 'a'`, _('/b_module')], ['c', `a = 'a'`, null],
['d', `b = a_module.a`, _('/b_module')], ['d', `b = a_module.a`, null],
['e', `e = 'e'`, _('/b_module')], ['e', `e = 'e'`, null],
['DirectiveX', `Directive: FnWithArg<(clazz: any) => any>`, _('/b_module')], ['DirectiveX', `Directive: FnWithArg<(clazz: any) => any>`, '@angular/core'],
[ [
'SomeClass', 'SomeClass',
`SomeClass = (function() {\n function SomeClass() {}\n return SomeClass;\n}())`, `SomeClass = (function() {\n function SomeClass() {}\n return SomeClass;\n}())`,
_('/b_module') null
], ],
['xtra1', `xtra1 = 'xtra1'`, _('/xtra_module')], ['xtra1', `xtra1 = 'xtra1'`, null],
['xtra2', `xtra2 = 'xtra2'`, _('/xtra_module')], ['xtra2', `xtra2 = 'xtra2'`, null],
]); ]);
}); });

15
packages/compiler-cli/ngcc/test/host/umd_host_spec.ts
View File

@ -591,8 +591,8 @@ runInEachFileSystem(() => {
{ {
name: _('/b_module.js'), name: _('/b_module.js'),
contents: `(function (global, factory) {\n` + contents: `(function (global, factory) {\n` +
` typeof exports === 'object' && typeof module !== 'undefined' ? factory(exports, require('@angular/core'), require('/a_module')) :\n` + ` typeof exports === 'object' && typeof module !== 'undefined' ? factory(exports, require('@angular/core'), require('./a_module')) :\n` +
` typeof define === 'function' && define.amd ? define('b_module', ['exports', '@angular/core', 'a_module'], factory) :\n` + ` typeof define === 'function' && define.amd ? define('b_module', ['exports', '@angular/core', './a_module'], factory) :\n` +
` (factory(global.b_module));\n` + ` (factory(global.b_module));\n` +
`}(this, (function (exports, core, a_module) { 'use strict';\n` + `}(this, (function (exports, core, a_module) { 'use strict';\n` +
` var b = a_module.a;\n` + ` var b = a_module.a;\n` +
@ -1931,9 +1931,10 @@ runInEachFileSystem(() => {
const host = createHost(bundle, new UmdReflectionHost(new MockLogger(), false, bundle)); const host = createHost(bundle, new UmdReflectionHost(new MockLogger(), false, bundle));
const {factoryFn} = parseStatementForUmdModule( const {factoryFn} = parseStatementForUmdModule(
getSourceFileOrError(bundle.program, testFile.name).statements[0])!; getSourceFileOrError(bundle.program, testFile.name).statements[0])!;
const tslibSourceFile = getSourceFileOrError(bundle.program, tslibFile.name); const testForHelper = createTestForTsHelper(
host, factoryFn,
const testForHelper = createTestForTsHelper(host, factoryFn, () => tslibSourceFile); (_fn, helperName) => getDeclaration(
bundle.program, tslibFile.name, helperName, ts.isFunctionDeclaration));
testForHelper('a', '__assign', KnownDeclaration.TsHelperAssign, 'tslib'); testForHelper('a', '__assign', KnownDeclaration.TsHelperAssign, 'tslib');
testForHelper('b', '__spread', KnownDeclaration.TsHelperSpread, 'tslib'); testForHelper('b', '__spread', KnownDeclaration.TsHelperSpread, 'tslib');
@ -2173,9 +2174,9 @@ runInEachFileSystem(() => {
.map(entry => [entry[0], entry[1].node!.getText(), entry[1].viaModule])) .map(entry => [entry[0], entry[1].node!.getText(), entry[1].viaModule]))
.toEqual([ .toEqual([
['Directive', `Directive: FnWithArg<(clazz: any) => any>`, '@angular/core'], ['Directive', `Directive: FnWithArg<(clazz: any) => any>`, '@angular/core'],
['a', `a = 'a'`, '/a_module'], ['a', `a = 'a'`, null],
['b', `b = a_module.a`, null], ['b', `b = a_module.a`, null],
['c', `a = 'a'`, '/a_module'], ['c', `a = 'a'`, null],
['d', `b = a_module.a`, null], ['d', `b = a_module.a`, null],
['e', `e = 'e'`, null], ['e', `e = 'e'`, null],
['DirectiveX', `Directive: FnWithArg<(clazz: any) => any>`, '@angular/core'], ['DirectiveX', `Directive: FnWithArg<(clazz: any) => any>`, '@angular/core'],