From 6a28675a5e04d38e24c510870f6589631224992c 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 (#38666) 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 PR Close #38666 --- .../ngcc/src/host/esm2015_host.ts | 55 +++++++------- .../ngcc/test/host/commonjs_host_spec.ts | 63 ++++++++++++++++ .../ngcc/test/host/esm2015_host_spec.ts | 53 ++++++++++++- .../ngcc/test/host/esm5_host_spec.ts | 61 +++++++++++++++ .../ngcc/test/host/umd_host_spec.ts | 74 ++++++++++++++++++- packages/compiler-cli/ngcc/test/host/util.ts | 30 +++++--- 6 files changed, 294 insertions(+), 42 deletions(-) diff --git a/packages/compiler-cli/ngcc/src/host/esm2015_host.ts b/packages/compiler-cli/ngcc/src/host/esm2015_host.ts index 579ab00546a9c..343880920c4c9 100644 --- a/packages/compiler-cli/ngcc/src/host/esm2015_host.ts +++ b/packages/compiler-cli/ngcc/src/host/esm2015_host.ts @@ -7,8 +7,8 @@ */ 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 {isWithinPackage} from '../analysis/util'; @@ -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,29 @@ export class Esm2015ReflectionHost extends TypeScriptReflectionHost implements N }); } + /** + * Compute the `TypeValueReference` for the given `typeExpression`. + * + * In ngcc, all the `typeExpression` are guaranteed to be "values" because it is working in JS and + * not TS. This means that the TS compiler is not going to remove the "type" import and so we can + * always use a LOCAL `TypeValueReference` kind, rather than trying to force an additional import + * for non-local expressions. + */ + private typeToValue(typeExpression: ts.Expression|null): TypeValueReference { + if (typeExpression === null) { + return { + kind: TypeValueReferenceKind.UNAVAILABLE, + reason: {kind: ValueUnavailableKind.MISSING_TYPE}, + }; + } + + return { + kind: TypeValueReferenceKind.LOCAL, + expression: typeExpression, + defaultImportStatement: 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/test/host/commonjs_host_spec.ts b/packages/compiler-cli/ngcc/test/host/commonjs_host_spec.ts index d2e1fcf02b76d..c7d0f243b0c3e 100644 --- a/packages/compiler-cli/ngcc/test/host/commonjs_host_spec.ts +++ b/packages/compiler-cli/ngcc/test/host/commonjs_host_spec.ts @@ -1211,6 +1211,69 @@ exports.MissingClass2 = MissingClass2; }); describe('getConstructorParameters', () => { + it('should always specify LOCAL type value references 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: ` + var Internal = (function() { + function Internal() { + } + return Internal; + }()); + exports.External = Internal; + ` + }, + { + name: _('/main.js'), + contents: ` + var shared = require('shared-lib'); + var local = require('./local'); + var SameFile = (function() { + function SameFile() { + } + return SameFile; + }()); + exports.SameFile = SameFile; + + var SomeClass = (function() { + function SomeClass(arg1, arg2, arg3) {} + return SomeClass; + }()); + SomeClass.ctorParameters = function() { return [{ type: shared.Baz }, { type: local.External }, { type: SameFile }]; }; + exports.SomeClass = SomeClass; + `, + }, + ]; + + loadTestFiles(files); + const bundle = makeTestBundleProgram(_('/main.js')); + const host = + createHost(bundle, new CommonJsReflectionHost(new MockLogger(), false, bundle)); + const classNode = getDeclaration( + bundle.program, _('/main.js'), 'SomeClass', isNamedVariableDeclaration); + + const parameters = host.getConstructorParameters(classNode)!; + + expect(parameters.map(p => p.name)).toEqual(['arg1', 'arg2', 'arg3']); + expectTypeValueReferencesForParameters( + parameters, ['shared.Baz', 'local.External', 'SameFile']); + }); + it('should find the decorated constructor parameters', () => { loadTestFiles([SOME_DIRECTIVE_FILE]); const bundle = makeTestBundleProgram(SOME_DIRECTIVE_FILE.name); 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..96182d3e4858c 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,57 @@ runInEachFileSystem(() => { }); describe('getConstructorParameters()', () => { + it('should always specify LOCAL type value references 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']); + }); + it('should find the decorated constructor parameters', () => { loadFakeCore(getFileSystem()); loadTestFiles([SOME_DIRECTIVE_FILE]); @@ -1154,7 +1205,7 @@ runInEachFileSystem(() => { '_viewContainer', '_template', 'injected' ]); expectTypeValueReferencesForParameters( - parameters, ['ViewContainerRef', 'TemplateRef', null], '@angular/core'); + parameters, ['ViewContainerRef', 'TemplateRef', 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..dafb07947d6a3 100644 --- a/packages/compiler-cli/ngcc/test/host/esm5_host_spec.ts +++ b/packages/compiler-cli/ngcc/test/host/esm5_host_spec.ts @@ -1252,6 +1252,67 @@ runInEachFileSystem(() => { }); describe('getConstructorParameters()', () => { + it('should always specify LOCAL type value references 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: ` + var Internal = (function() { + function Internal() { + } + return Internal; + }()); + export {Internal as External}; + ` + }, + { + name: _('/main.js'), + contents: ` + import {Baz} from 'shared-lib'; + import {External} from './local'; + var SameFile = (function() { + function SameFile() { + } + return SameFile; + }()); + export SameFile; + + var SomeClass = (function() { + function SomeClass(arg1, arg2, arg3) {} + return SomeClass; + }()); + SomeClass.ctorParameters = function() { return [{ type: Baz }, { type: External }, { type: SameFile }]; }; + export SomeClass; + `, + }, + ]; + + loadTestFiles(files); + const bundle = makeTestBundleProgram(_('/main.js')); + const host = createHost(bundle, new Esm5ReflectionHost(new MockLogger(), false, bundle)); + const classNode = getDeclaration( + bundle.program, _('/main.js'), 'SomeClass', isNamedVariableDeclaration); + + const parameters = host.getConstructorParameters(classNode)!; + + expect(parameters.map(p => p.name)).toEqual(['arg1', 'arg2', 'arg3']); + expectTypeValueReferencesForParameters(parameters, ['Baz', 'External', 'SameFile']); + }); + it('should find the decorated constructor parameters', () => { loadTestFiles([SOME_DIRECTIVE_FILE]); const bundle = makeTestBundleProgram(SOME_DIRECTIVE_FILE.name); 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..cf918fb43f533 100644 --- a/packages/compiler-cli/ngcc/test/host/umd_host_spec.ts +++ b/packages/compiler-cli/ngcc/test/host/umd_host_spec.ts @@ -1332,6 +1332,78 @@ runInEachFileSystem(() => { }); describe('getConstructorParameters', () => { + it('should always specify LOCAL type value references 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: ` + (function (global, factory) { + typeof exports === 'object' && typeof module !== 'undefined' ? factory(exports) : + typeof define === 'function' && define.amd ? define('local', ['exports'], factory) : + (factory(global.local)); + }(this, (function (exports) { 'use strict'; + var Internal = (function() { + function Internal() { + } + return Internal; + }()); + exports.External = Internal; + }))); + ` + }, + { + name: _('/main.js'), + contents: ` + (function (global, factory) { + typeof exports === 'object' && typeof module !== 'undefined' ? factory(exports, require('shared-lib), require('./local')) : + typeof define === 'function' && define.amd ? define('main', ['exports', 'shared-lib', './local'], factory) : + (factory(global.main, global.shared, global.local)); + }(this, (function (exports, shared, local) { 'use strict'; + var SameFile = (function() { + function SameFile() { + } + return SameFile; + }()); + exports.SameFile = SameFile; + + var SomeClass = (function() { + function SomeClass(arg1, arg2, arg3) {} + return SomeClass; + }()); + SomeClass.ctorParameters = function() { return [{ type: shared.Baz }, { type: local.External }, { type: SameFile }]; }; + exports.SomeClass = SomeClass; + }))); + `, + }, + ]; + + loadTestFiles(files); + const bundle = makeTestBundleProgram(_('/main.js')); + const host = createHost(bundle, new UmdReflectionHost(new MockLogger(), false, bundle)); + const classNode = getDeclaration( + bundle.program, _('/main.js'), 'SomeClass', isNamedVariableDeclaration); + + const parameters = host.getConstructorParameters(classNode)!; + + expect(parameters.map(p => p.name)).toEqual(['arg1', 'arg2', 'arg3']); + expectTypeValueReferencesForParameters( + parameters, ['shared.Baz', 'local.External', 'SameFile']); + }); + it('should find the decorated constructor parameters', () => { loadTestFiles([SOME_DIRECTIVE_FILE]); const bundle = makeTestBundleProgram(SOME_DIRECTIVE_FILE.name); @@ -1591,7 +1663,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) : diff --git a/packages/compiler-cli/ngcc/test/host/util.ts b/packages/compiler-cli/ngcc/test/host/util.ts index c27594c6cf735..5767be4bf789f 100644 --- a/packages/compiler-cli/ngcc/test/host/util.ts +++ b/packages/compiler-cli/ngcc/test/host/util.ts @@ -13,26 +13,36 @@ 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}" because "${param.typeValueReference.reason}"`); } 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`); + if (!ts.isIdentifier(param.typeValueReference.expression) && + !ts.isPropertyAccessExpression(param.typeValueReference.expression)) { + fail(`Incorrect typeValueReference generated for ${ + param.name}, expected an identifier but got "${ + param.typeValueReference.expression.getText()}"`); } else { - expect(param.typeValueReference.expression.text).toEqual(expected); + expect(param.typeValueReference.expression.getText()).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); } }