Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(ngcc): use aliased exported types correctly #38666

Closed
wants to merge 1 commit into from
Closed

fix(ngcc): use aliased exported types correctly #38666

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
55 changes: 25 additions & 30 deletions packages/compiler-cli/ngcc/src/host/esm2015_host.ts
View file
Open in desktop
Expand Up @@ -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';
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,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.
Expand Down
63 changes: 63 additions & 0 deletions packages/compiler-cli/ngcc/test/host/commonjs_host_spec.ts
View file
Open in desktop
Expand Up @@ -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);
Expand Down
53 changes: 52 additions & 1 deletion packages/compiler-cli/ngcc/test/host/esm2015_host_spec.ts
View file
Open in desktop
Expand Up @@ -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]);
Expand All @@ -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', () => {
Expand Down
61 changes: 61 additions & 0 deletions packages/compiler-cli/ngcc/test/host/esm5_host_spec.ts
View file
Open in desktop
Expand Up @@ -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);
Expand Down
74 changes: 73 additions & 1 deletion packages/compiler-cli/ngcc/test/host/umd_host_spec.ts
View file
Open in desktop
Expand Up @@ -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);
Expand Down Expand Up @@ -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) :
Expand Down