From b3eda980261e7e8df8247f8cd7837eba3314afb2 Mon Sep 17 00:00:00 2001 From: Pete Bacon Darwin Date: Tue, 1 Sep 2020 16:49:48 +0100 Subject: [PATCH] fix(ngcc): use aliased exported types correctly If a type has been renamed when it was exported, we need to reference the external public alias name rather than the internal original name for the type. Otherwise we will try to import the type by its internal name, which is not publicly accessible. Fixes #38238 --- .../ngcc/src/host/commonjs_host.ts | 5 +- .../ngcc/src/host/esm2015_host.ts | 85 ++++++++++++------- .../compiler-cli/ngcc/src/host/esm5_host.ts | 1 + .../compiler-cli/ngcc/src/host/umd_host.ts | 17 ++-- .../ngcc/test/host/commonjs_host_spec.ts | 3 + .../ngcc/test/host/esm2015_host_spec.ts | 57 ++++++++++++- .../ngcc/test/host/esm5_host_spec.ts | 3 + .../ngcc/test/host/umd_host_spec.ts | 5 +- packages/compiler-cli/ngcc/test/host/util.ts | 22 +++-- .../partial_evaluator/test/evaluator_spec.ts | 1 + .../src/ngtsc/reflection/src/host.ts | 1 + .../src/ngtsc/reflection/src/typescript.ts | 3 + .../src/ngtsc/reflection/test/ts_host_spec.ts | 2 + 13 files changed, 156 insertions(+), 49 deletions(-) diff --git a/packages/compiler-cli/ngcc/src/host/commonjs_host.ts b/packages/compiler-cli/ngcc/src/host/commonjs_host.ts index ca3d6532c6396..b8304951b73ea 100644 --- a/packages/compiler-cli/ngcc/src/host/commonjs_host.ts +++ b/packages/compiler-cli/ngcc/src/host/commonjs_host.ts @@ -181,7 +181,7 @@ export class CommonJsReflectionHost extends Esm5ReflectionHost { } else { return { name, - declaration: {node: null, known: null, expression, viaModule: null}, + declaration: {node: null, known: null, expression, viaModule: null, importName: null}, }; } } @@ -204,7 +204,8 @@ export class CommonJsReflectionHost extends Esm5ReflectionHost { return null; } const viaModule = isExternalImport(importPath) ? importPath : null; - return {node: module, known: null, viaModule, identity: null}; + const importName = viaModule && id.text; + return {node: module, known: null, viaModule, importName, identity: null}; } private resolveModuleName(moduleName: string, containingFile: ts.SourceFile): ts.SourceFile diff --git a/packages/compiler-cli/ngcc/src/host/esm2015_host.ts b/packages/compiler-cli/ngcc/src/host/esm2015_host.ts index 579ab00546a9c..8262ed70b681f 100644 --- a/packages/compiler-cli/ngcc/src/host/esm2015_host.ts +++ b/packages/compiler-cli/ngcc/src/host/esm2015_host.ts @@ -7,10 +7,10 @@ */ import * as ts from 'typescript'; -import {absoluteFromSourceFile} from '../../../src/ngtsc/file_system'; +import {absoluteFromSourceFile} from '../../../src/ngtsc/file_system'; import {Logger} from '../../../src/ngtsc/logging'; -import {ClassDeclaration, ClassMember, ClassMemberKind, CtorParameter, Declaration, Decorator, EnumMember, isDecoratorIdentifier, isNamedClassDeclaration, isNamedFunctionDeclaration, isNamedVariableDeclaration, KnownDeclaration, reflectObjectLiteral, SpecialDeclarationKind, TypeScriptReflectionHost, TypeValueReference, TypeValueReferenceKind, ValueUnavailableKind} from '../../../src/ngtsc/reflection'; +import {ClassDeclaration, ClassMember, ClassMemberKind, CtorParameter, Declaration, Decorator, EnumMember, Import, isDecoratorIdentifier, isNamedClassDeclaration, isNamedFunctionDeclaration, isNamedVariableDeclaration, KnownDeclaration, reflectObjectLiteral, SpecialDeclarationKind, TypeScriptReflectionHost, TypeValueReference, TypeValueReferenceKind, ValueUnavailableKind} from '../../../src/ngtsc/reflection'; import {isWithinPackage} from '../analysis/util'; import {BundleProgram} from '../packages/bundle_program'; import {findAll, getNameText, hasNameIdentifier, isDefined, stripDollarSuffix} from '../utils'; @@ -1593,35 +1593,7 @@ export class Esm2015ReflectionHost extends TypeScriptReflectionHost implements N constructorParamInfo[index] : {decorators: null, typeExpression: null}; const nameNode = node.name; - - let typeValueReference: TypeValueReference; - if (typeExpression !== null) { - // `typeExpression` is an expression in a "type" context. Resolve it to a declared value. - // Either it's a reference to an imported type, or a type declared locally. Distinguish the - // two cases with `getDeclarationOfExpression`. - const decl = this.getDeclarationOfExpression(typeExpression); - if (decl !== null && decl.node !== null && decl.viaModule !== null && - isNamedDeclaration(decl.node)) { - typeValueReference = { - kind: TypeValueReferenceKind.IMPORTED, - valueDeclaration: decl.node, - moduleName: decl.viaModule, - importedName: decl.node.name.text, - nestedPath: null, - }; - } else { - typeValueReference = { - kind: TypeValueReferenceKind.LOCAL, - expression: typeExpression, - defaultImportStatement: null, - }; - } - } else { - typeValueReference = { - kind: TypeValueReferenceKind.UNAVAILABLE, - reason: {kind: ValueUnavailableKind.MISSING_TYPE}, - }; - } + const typeValueReference = this.typeToValue(typeExpression); return { name: getNameText(nameNode), @@ -1633,6 +1605,57 @@ export class Esm2015ReflectionHost extends TypeScriptReflectionHost implements N }); } + private getImportOfTypeExpression(typeExpression: ts.Expression): Import|null { + if (ts.isIdentifier(typeExpression)) { + return this.getImportOfIdentifier(typeExpression); + } + if (ts.isPropertyAccessExpression(typeExpression) && + ts.isIdentifier(typeExpression.expression)) { + return this.getImportOfIdentifier(typeExpression.expression); + } + return null; + } + + /** + * `typeExpression` is an expression in a "type" context. Resolve it to a declared value. + * + * Either it's a reference to an imported type, or a type declared locally. + */ + private typeToValue(typeExpression: ts.Expression|null): TypeValueReference { + if (typeExpression === null) { + return { + kind: TypeValueReferenceKind.UNAVAILABLE, + reason: {kind: ValueUnavailableKind.MISSING_TYPE}, + }; + } + + const imp = this.getImportOfTypeExpression(typeExpression); + if (imp === null) { + return { + kind: TypeValueReferenceKind.LOCAL, + expression: typeExpression, + defaultImportStatement: null, + }; + } + + const decl = this.getDeclarationOfExpression(typeExpression); + if (decl === null || decl.node === null) { + return { + kind: TypeValueReferenceKind.LOCAL, + expression: typeExpression, + defaultImportStatement: null, + }; + } + + return { + kind: TypeValueReferenceKind.IMPORTED, + valueDeclaration: decl.node, + moduleName: imp.from, + importedName: imp.name, + nestedPath: null, + }; + } + /** * Get the parameter type and decorators for the constructor of a class, * where the information is stored on a static property of the class. diff --git a/packages/compiler-cli/ngcc/src/host/esm5_host.ts b/packages/compiler-cli/ngcc/src/host/esm5_host.ts index 5464fbcff4cbe..42f2f61451182 100644 --- a/packages/compiler-cli/ngcc/src/host/esm5_host.ts +++ b/packages/compiler-cli/ngcc/src/host/esm5_host.ts @@ -86,6 +86,7 @@ export class Esm5ReflectionHost extends Esm2015ReflectionHost { known: nonEmittedNorImportedTsHelperDeclaration, node: null, viaModule: null, + importName: null, }; } } diff --git a/packages/compiler-cli/ngcc/src/host/umd_host.ts b/packages/compiler-cli/ngcc/src/host/umd_host.ts index 5a4b73ed70b35..0a18db6a5ca49 100644 --- a/packages/compiler-cli/ngcc/src/host/umd_host.ts +++ b/packages/compiler-cli/ngcc/src/host/umd_host.ts @@ -175,11 +175,15 @@ export class UmdReflectionHost extends Esm5ReflectionHost { if (decl.node !== null) { reexports.push({ name, - declaration: {node: decl.node, known: null, viaModule, identity: decl.identity} + declaration: + {node: decl.node, known: null, viaModule, importName: name, identity: decl.identity} }); } else { - reexports.push( - {name, declaration: {node: null, known: null, expression: decl.expression, viaModule}}); + reexports.push({ + name, + declaration: + {node: null, known: null, expression: decl.expression, viaModule, importName: name} + }); } }); return reexports; @@ -203,7 +207,7 @@ export class UmdReflectionHost extends Esm5ReflectionHost { } return { name, - declaration: {node: null, known: null, expression, viaModule: null}, + declaration: {node: null, known: null, expression, viaModule: null, importName: null}, }; } @@ -245,8 +249,9 @@ export class UmdReflectionHost extends Esm5ReflectionHost { // did not know that we were importing the declaration. const viaModule = declaration.viaModule === null ? moduleDeclaration.viaModule : declaration.viaModule; + const importName = viaModule && id.text; - return {...declaration, viaModule, known: getTsHelperFnFromIdentifier(id)}; + return {...declaration, viaModule, importName, known: getTsHelperFnFromIdentifier(id)}; } private getUmdModuleDeclaration(id: ts.Identifier): Declaration|null { @@ -261,7 +266,7 @@ export class UmdReflectionHost extends Esm5ReflectionHost { } const viaModule = isExternalImport(importPath) ? importPath : null; - return {node: module, viaModule, known: null, identity: null}; + return {node: module, viaModule, known: null, identity: null, importName: null}; } private getImportPathFromParameter(id: ts.Identifier): string|null { 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 d2e1fcf02b76d..a60c06cff7084 100644 --- a/packages/compiler-cli/ngcc/test/host/commonjs_host_spec.ts +++ b/packages/compiler-cli/ngcc/test/host/commonjs_host_spec.ts @@ -1780,6 +1780,7 @@ exports.MissingClass2 = MissingClass2; known: knownAs, node: getHelperDeclaration(helperName), viaModule, + importName: viaModule && helperName, identity: null, }); }; @@ -2104,6 +2105,7 @@ exports.MissingClass2 = MissingClass2; expression: helperIdentifier, node: null, viaModule: null, + importName: null, }); }; @@ -2138,6 +2140,7 @@ exports.MissingClass2 = MissingClass2; expression: helperIdentifier, node: null, viaModule: null, + importName: null, }); }; diff --git a/packages/compiler-cli/ngcc/test/host/esm2015_host_spec.ts b/packages/compiler-cli/ngcc/test/host/esm2015_host_spec.ts index b5db58d50b2b0..be54d4edfa973 100644 --- a/packages/compiler-cli/ngcc/test/host/esm2015_host_spec.ts +++ b/packages/compiler-cli/ngcc/test/host/esm2015_host_spec.ts @@ -1140,6 +1140,60 @@ runInEachFileSystem(() => { }); describe('getConstructorParameters()', () => { + describe('getConstructorParameters()', () => { + it('should find the correct imported identifier for decorated constructor parameter types', + () => { + const files = [ + { + name: _('/node_modules/shared-lib/foo.d.ts'), + contents: ` + declare class Foo {} + export {Foo as Bar}; + `, + }, + { + name: _('/node_modules/shared-lib/index.d.ts'), + contents: ` + export {Bar as Baz} from './foo'; + `, + }, + { + name: _('/local.js'), + contents: ` + class Internal {} + export {Internal as External}; + ` + }, + { + name: _('/main.js'), + contents: ` + import {Baz} from 'shared-lib'; + import {External} from './local'; + export class SameFile {} + + export class SomeClass { + constructor(arg1, arg2, arg3) {} + } + SomeClass.ctorParameters = [{ type: Baz }, { type: External }, { type: SameFile }]; + `, + }, + ]; + + loadTestFiles(files); + const bundle = makeTestBundleProgram(_('/main.js')); + const host = + createHost(bundle, new Esm2015ReflectionHost(new MockLogger(), false, bundle)); + const classNode = getDeclaration( + bundle.program, _('/main.js'), 'SomeClass', isNamedClassDeclaration); + + const parameters = host.getConstructorParameters(classNode)!; + + expect(parameters.map(p => p.name)).toEqual(['arg1', 'arg2', 'arg3']); + expectTypeValueReferencesForParameters( + parameters, ['Baz', 'External', 'SameFile'], ['shared-lib', './local', null]); + }); + }); + it('should find the decorated constructor parameters', () => { loadFakeCore(getFileSystem()); loadTestFiles([SOME_DIRECTIVE_FILE]); @@ -1154,7 +1208,8 @@ runInEachFileSystem(() => { '_viewContainer', '_template', 'injected' ]); expectTypeValueReferencesForParameters( - parameters, ['ViewContainerRef', 'TemplateRef', null], '@angular/core'); + parameters, ['ViewContainerRef', 'TemplateRef', null], + ['@angular/core', '@angular/core', null]); }); it('should accept `ctorParameters` as an array', () => { diff --git a/packages/compiler-cli/ngcc/test/host/esm5_host_spec.ts b/packages/compiler-cli/ngcc/test/host/esm5_host_spec.ts index 0fb7428196efa..13cf0614af938 100644 --- a/packages/compiler-cli/ngcc/test/host/esm5_host_spec.ts +++ b/packages/compiler-cli/ngcc/test/host/esm5_host_spec.ts @@ -1799,6 +1799,7 @@ runInEachFileSystem(() => { known: knownAs, node: getHelperDeclaration(helperName), viaModule, + importName: viaModule && helperName, identity: null, }); }; @@ -2195,6 +2196,7 @@ runInEachFileSystem(() => { expression: helperIdentifier, node: null, viaModule: null, + importName: null, }); }; @@ -2226,6 +2228,7 @@ runInEachFileSystem(() => { expression: helperIdentifier, node: null, viaModule: null, + importName: null, }); }; 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 a25b09fb5a3bc..8a42ccf427a6e 100644 --- a/packages/compiler-cli/ngcc/test/host/umd_host_spec.ts +++ b/packages/compiler-cli/ngcc/test/host/umd_host_spec.ts @@ -1591,7 +1591,7 @@ runInEachFileSystem(() => { `; break; case 'inlined_with_suffix': - fileHeaderWithUmd = ` + fileHeaderWithUmd = ` (function (global, factory) { typeof exports === 'object' && typeof module !== 'undefined' ? factory(exports)) : typeof define === 'function' && define.amd ? define('test', ['exports'], factory) : @@ -1904,6 +1904,7 @@ runInEachFileSystem(() => { known: knownAs, node: getHelperDeclaration(factoryFn, helperName), viaModule, + importName: viaModule && helperName, identity: null, }); }; @@ -2207,6 +2208,7 @@ runInEachFileSystem(() => { expression: helperIdentifier, node: null, viaModule: null, + importName: null, }); }; @@ -2246,6 +2248,7 @@ runInEachFileSystem(() => { expression: helperIdentifier, node: null, viaModule: null, + importName: null, }); }; diff --git a/packages/compiler-cli/ngcc/test/host/util.ts b/packages/compiler-cli/ngcc/test/host/util.ts index c27594c6cf735..acb60a460bca9 100644 --- a/packages/compiler-cli/ngcc/test/host/util.ts +++ b/packages/compiler-cli/ngcc/test/host/util.ts @@ -13,26 +13,32 @@ import {CtorParameter, TypeValueReferenceKind} from '../../../src/ngtsc/reflecti * names. */ export function expectTypeValueReferencesForParameters( - parameters: CtorParameter[], expectedParams: (string|null)[], fromModule: string|null = null) { + parameters: CtorParameter[], expectedParams: (string|null)[], + fromModule: (string|null)[] = []) { parameters!.forEach((param, idx) => { const expected = expectedParams[idx]; if (expected !== null) { if (param.typeValueReference.kind === TypeValueReferenceKind.UNAVAILABLE) { - fail(`Incorrect typeValueReference generated, expected ${expected}`); + fail(`Incorrect typeValueReference generated for ${param.name}, expected ${expected}`); } else if ( - param.typeValueReference.kind === TypeValueReferenceKind.LOCAL && fromModule !== null) { - fail(`Incorrect typeValueReference generated, expected non-local`); + param.typeValueReference.kind === TypeValueReferenceKind.LOCAL && + fromModule[idx] != null) { + fail(`Incorrect typeValueReference generated for ${param.name}, expected non-LOCAL (from ${ + fromModule[idx]}) but was marked LOCAL`); } else if ( - param.typeValueReference.kind !== TypeValueReferenceKind.LOCAL && fromModule === null) { - fail(`Incorrect typeValueReference generated, expected local`); + param.typeValueReference.kind !== TypeValueReferenceKind.LOCAL && + fromModule[idx] == null) { + fail(`Incorrect typeValueReference generated for ${ + param.name}, expected LOCAL but was imported from ${ + param.typeValueReference.moduleName}`); } else if (param.typeValueReference.kind === TypeValueReferenceKind.LOCAL) { if (!ts.isIdentifier(param.typeValueReference.expression)) { - fail(`Incorrect typeValueReference generated, expected identifier`); + fail(`Incorrect typeValueReference generated for ${param.name}, expected identifier`); } else { expect(param.typeValueReference.expression.text).toEqual(expected); } } else if (param.typeValueReference.kind === TypeValueReferenceKind.IMPORTED) { - expect(param.typeValueReference.moduleName).toBe(fromModule!); + expect(param.typeValueReference.moduleName).toBe(fromModule[idx]!); expect(param.typeValueReference.importedName).toBe(expected); } } diff --git a/packages/compiler-cli/src/ngtsc/partial_evaluator/test/evaluator_spec.ts b/packages/compiler-cli/src/ngtsc/partial_evaluator/test/evaluator_spec.ts index 7c8d53e9257ca..2c535d1ec1db1 100644 --- a/packages/compiler-cli/src/ngtsc/partial_evaluator/test/evaluator_spec.ts +++ b/packages/compiler-cli/src/ngtsc/partial_evaluator/test/evaluator_spec.ts @@ -960,6 +960,7 @@ runInEachFileSystem(() => { known: tsHelperFn, node: id, viaModule: null, + importName: null, identity: null, }; } diff --git a/packages/compiler-cli/src/ngtsc/reflection/src/host.ts b/packages/compiler-cli/src/ngtsc/reflection/src/host.ts index 3d28a12d31940..6259576e6ed74 100644 --- a/packages/compiler-cli/src/ngtsc/reflection/src/host.ts +++ b/packages/compiler-cli/src/ngtsc/reflection/src/host.ts @@ -540,6 +540,7 @@ export interface BaseDeclaration { * of the application and was not imported from an absolute path, this will be `null`. */ viaModule: string|null; + importName: string|null; /** * TypeScript reference to the declaration itself, if one exists. diff --git a/packages/compiler-cli/src/ngtsc/reflection/src/typescript.ts b/packages/compiler-cli/src/ngtsc/reflection/src/typescript.ts index 85be99477c5fb..15eacae964b2a 100644 --- a/packages/compiler-cli/src/ngtsc/reflection/src/typescript.ts +++ b/packages/compiler-cli/src/ngtsc/reflection/src/typescript.ts @@ -303,6 +303,7 @@ export class TypeScriptReflectionHost implements ReflectionHost { importInfo !== null && importInfo.from !== null && !importInfo.from.startsWith('.') ? importInfo.from : null; + const importName = importInfo && importInfo.name; // Now, resolve the Symbol to its declaration by following any and all aliases. while (symbol.flags & ts.SymbolFlags.Alias) { @@ -316,6 +317,7 @@ export class TypeScriptReflectionHost implements ReflectionHost { node: symbol.valueDeclaration, known: null, viaModule, + importName, identity: null, }; } else if (symbol.declarations !== undefined && symbol.declarations.length > 0) { @@ -323,6 +325,7 @@ export class TypeScriptReflectionHost implements ReflectionHost { node: symbol.declarations[0], known: null, viaModule, + importName, identity: null, }; } else { diff --git a/packages/compiler-cli/src/ngtsc/reflection/test/ts_host_spec.ts b/packages/compiler-cli/src/ngtsc/reflection/test/ts_host_spec.ts index f4cfd540acc12..16a3159a7ada8 100644 --- a/packages/compiler-cli/src/ngtsc/reflection/test/ts_host_spec.ts +++ b/packages/compiler-cli/src/ngtsc/reflection/test/ts_host_spec.ts @@ -363,6 +363,7 @@ runInEachFileSystem(() => { node: targetDecl, known: null, viaModule: 'absolute', + importName: 'Target', identity: null, }); }); @@ -394,6 +395,7 @@ runInEachFileSystem(() => { node: targetDecl, known: null, viaModule: 'absolute', + importName: 'Target', identity: null, }); });