Skip to content

Commit

Permalink
fix(ngcc): use aliased exported types correctly
Browse files Browse the repository at this point in the history
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
  • Loading branch information
petebacondarwin committed Sep 4, 2020
1 parent c90eb54 commit b3eda98
Show file tree
Hide file tree
Showing 13 changed files with 156 additions and 49 deletions.
5 changes: 3 additions & 2 deletions packages/compiler-cli/ngcc/src/host/commonjs_host.ts
Expand Up @@ -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},
};
}
}
Expand All @@ -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
Expand Down
85 changes: 54 additions & 31 deletions packages/compiler-cli/ngcc/src/host/esm2015_host.ts
Expand Up @@ -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';
Expand Down Expand Up @@ -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),
Expand All @@ -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.
Expand Down
1 change: 1 addition & 0 deletions packages/compiler-cli/ngcc/src/host/esm5_host.ts
Expand Up @@ -86,6 +86,7 @@ export class Esm5ReflectionHost extends Esm2015ReflectionHost {
known: nonEmittedNorImportedTsHelperDeclaration,
node: null,
viaModule: null,
importName: null,
};
}
}
Expand Down
17 changes: 11 additions & 6 deletions packages/compiler-cli/ngcc/src/host/umd_host.ts
Expand Up @@ -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;
Expand All @@ -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},
};
}

Expand Down Expand Up @@ -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 {
Expand All @@ -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 {
Expand Down
3 changes: 3 additions & 0 deletions packages/compiler-cli/ngcc/test/host/commonjs_host_spec.ts
Expand Up @@ -1780,6 +1780,7 @@ exports.MissingClass2 = MissingClass2;
known: knownAs,
node: getHelperDeclaration(helperName),
viaModule,
importName: viaModule && helperName,
identity: null,
});
};
Expand Down Expand Up @@ -2104,6 +2105,7 @@ exports.MissingClass2 = MissingClass2;
expression: helperIdentifier,
node: null,
viaModule: null,
importName: null,
});
};

Expand Down Expand Up @@ -2138,6 +2140,7 @@ exports.MissingClass2 = MissingClass2;
expression: helperIdentifier,
node: null,
viaModule: null,
importName: null,
});
};

Expand Down
57 changes: 56 additions & 1 deletion packages/compiler-cli/ngcc/test/host/esm2015_host_spec.ts
Expand Up @@ -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]);
Expand All @@ -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', () => {
Expand Down
3 changes: 3 additions & 0 deletions packages/compiler-cli/ngcc/test/host/esm5_host_spec.ts
Expand Up @@ -1799,6 +1799,7 @@ runInEachFileSystem(() => {
known: knownAs,
node: getHelperDeclaration(helperName),
viaModule,
importName: viaModule && helperName,
identity: null,
});
};
Expand Down Expand Up @@ -2195,6 +2196,7 @@ runInEachFileSystem(() => {
expression: helperIdentifier,
node: null,
viaModule: null,
importName: null,
});
};

Expand Down Expand Up @@ -2226,6 +2228,7 @@ runInEachFileSystem(() => {
expression: helperIdentifier,
node: null,
viaModule: null,
importName: null,
});
};

Expand Down
5 changes: 4 additions & 1 deletion packages/compiler-cli/ngcc/test/host/umd_host_spec.ts
Expand Up @@ -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) :
Expand Down Expand Up @@ -1904,6 +1904,7 @@ runInEachFileSystem(() => {
known: knownAs,
node: getHelperDeclaration(factoryFn, helperName),
viaModule,
importName: viaModule && helperName,
identity: null,
});
};
Expand Down Expand Up @@ -2207,6 +2208,7 @@ runInEachFileSystem(() => {
expression: helperIdentifier,
node: null,
viaModule: null,
importName: null,
});
};

Expand Down Expand Up @@ -2246,6 +2248,7 @@ runInEachFileSystem(() => {
expression: helperIdentifier,
node: null,
viaModule: null,
importName: null,
});
};

Expand Down
22 changes: 14 additions & 8 deletions packages/compiler-cli/ngcc/test/host/util.ts
Expand Up @@ -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);
}
}
Expand Down
Expand Up @@ -960,6 +960,7 @@ runInEachFileSystem(() => {
known: tsHelperFn,
node: id,
viaModule: null,
importName: null,
identity: null,
};
}
Expand Down
1 change: 1 addition & 0 deletions packages/compiler-cli/src/ngtsc/reflection/src/host.ts
Expand Up @@ -540,6 +540,7 @@ export interface BaseDeclaration<T extends ts.Declaration = ts.Declaration> {
* 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.
Expand Down

0 comments on commit b3eda98

Please sign in to comment.