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); } }