fix(ngcc): consistently delegate to TypeScript host for typing files (#36089)
When ngcc is compiling an entry-point, it uses a `ReflectionHost` that is specific to its format, e.g. ES2015, ES5, UMD or CommonJS. During the compilation of that entry-point however, the reflector may be used to reflect into external libraries using their declaration files. Up until now this was achieved by letting all `ReflectionHost` classes consider their parent class for reflector queries, thereby ending up in the `TypeScriptReflectionHost` that is a common base class for all reflector hosts. This approach has proven to be prone to bugs, as failing to call into the base class would cause incompatibilities with reading from declaration files. The observation can be made that there's only two distinct kinds of reflection host queries: 1. the reflector query is about code that is part of the entry-point that is being compiled, or 2. the reflector query is for an external library that the entry-point depends on, in which case the information is reflected from the declaration files. The `ReflectionHost` that was chosen for the entry-point should serve only reflector queries for the first case, whereas a regular `TypeScriptReflectionHost` should be used for the second case. This avoids the problem where a format-specific `ReflectionHost` fails to handle the second case correctly, as it isn't even considered for such reflector queries. This commit introduces a `ReflectionHost` that delegates to the `TypeScriptReflectionHost` for AST nodes within declaration files, otherwise delegating to the format-specific `ReflectionHost`. Fixes #35078 Resolves FW-1859 PR Close #36089
This commit is contained in:
JoostK
Andrew Kushnir
parent
1bc3893c65
commit
9e70bcb34f
|
|
@ -34,11 +34,6 @@ export class CommonJsReflectionHost extends Esm5ReflectionHost {
|
|||
}
|
||||
|
||||
getImportOfIdentifier(id: ts.Identifier): Import|null {
|
||||
const superImport = super.getImportOfIdentifier(id);
|
||||
if (superImport !== null) {
|
||||
return superImport;
|
||||
}
|
||||
|
||||
const requireCall = this.findCommonJsImport(id);
|
||||
if (requireCall === null) {
|
||||
return null;
|
||||
|
|
@ -47,8 +42,7 @@ export class CommonJsReflectionHost extends Esm5ReflectionHost {
|
|||
}
|
||||
|
||||
getDeclarationOfIdentifier(id: ts.Identifier): Declaration|null {
|
||||
return (!id.getSourceFile().isDeclarationFile && this.getCommonJsImportedDeclaration(id)) ||
|
||||
super.getDeclarationOfIdentifier(id);
|
||||
return this.getCommonJsImportedDeclaration(id) || super.getDeclarationOfIdentifier(id);
|
||||
}
|
||||
|
||||
getExportsOfModule(module: ts.Node): Map<string, Declaration>|null {
|
||||
|
|
|
|||
|
|
@ -0,0 +1,157 @@
|
|||
/**
|
||||
* @license
|
||||
* Copyright Google Inc. All Rights Reserved.
|
||||
*
|
||||
* Use of this source code is governed by an MIT-style license that can be
|
||||
* found in the LICENSE file at https://angular.io/license
|
||||
*/
|
||||
|
||||
import * as ts from 'typescript';
|
||||
|
||||
import {ClassDeclaration, ClassMember, CtorParameter, Declaration, Decorator, FunctionDefinition, Import, ReflectionHost} from '../../../src/ngtsc/reflection';
|
||||
import {isFromDtsFile} from '../../../src/ngtsc/util/src/typescript';
|
||||
|
||||
import {ModuleWithProvidersFunction, NgccClassSymbol, NgccReflectionHost, SwitchableVariableDeclaration} from './ngcc_host';
|
||||
|
||||
/**
|
||||
* A reflection host implementation that delegates reflector queries depending on whether they
|
||||
* reflect on declaration files (for dependent libraries) or source files within the entry-point
|
||||
* that is being compiled. The first type of queries are handled by the regular TypeScript
|
||||
* reflection host, whereas the other queries are handled by an `NgccReflectionHost` that is
|
||||
* specific to the entry-point's format.
|
||||
*/
|
||||
export class DelegatingReflectionHost implements NgccReflectionHost {
|
||||
constructor(private tsHost: ReflectionHost, private ngccHost: NgccReflectionHost) {}
|
||||
|
||||
getConstructorParameters(clazz: ClassDeclaration): CtorParameter[]|null {
|
||||
if (isFromDtsFile(clazz)) {
|
||||
return this.tsHost.getConstructorParameters(clazz);
|
||||
}
|
||||
return this.ngccHost.getConstructorParameters(clazz);
|
||||
}
|
||||
|
||||
getDeclarationOfIdentifier(id: ts.Identifier): Declaration|null {
|
||||
if (isFromDtsFile(id)) {
|
||||
return this.tsHost.getDeclarationOfIdentifier(id);
|
||||
}
|
||||
return this.ngccHost.getDeclarationOfIdentifier(id);
|
||||
}
|
||||
|
||||
getDecoratorsOfDeclaration(declaration: ts.Declaration): Decorator[]|null {
|
||||
if (isFromDtsFile(declaration)) {
|
||||
return this.tsHost.getDecoratorsOfDeclaration(declaration);
|
||||
}
|
||||
return this.ngccHost.getDecoratorsOfDeclaration(declaration);
|
||||
}
|
||||
|
||||
getDefinitionOfFunction(fn: ts.Node): FunctionDefinition|null {
|
||||
if (isFromDtsFile(fn)) {
|
||||
return this.tsHost.getDefinitionOfFunction(fn);
|
||||
}
|
||||
return this.ngccHost.getDefinitionOfFunction(fn);
|
||||
}
|
||||
|
||||
getDtsDeclaration(declaration: ts.Declaration): ts.Declaration|null {
|
||||
if (isFromDtsFile(declaration)) {
|
||||
return this.tsHost.getDtsDeclaration(declaration);
|
||||
}
|
||||
return this.ngccHost.getDtsDeclaration(declaration);
|
||||
}
|
||||
|
||||
getExportsOfModule(module: ts.Node): Map<string, Declaration>|null {
|
||||
if (isFromDtsFile(module)) {
|
||||
return this.tsHost.getExportsOfModule(module);
|
||||
}
|
||||
return this.ngccHost.getExportsOfModule(module);
|
||||
}
|
||||
|
||||
getGenericArityOfClass(clazz: ClassDeclaration): number|null {
|
||||
if (isFromDtsFile(clazz)) {
|
||||
return this.tsHost.getGenericArityOfClass(clazz);
|
||||
}
|
||||
return this.ngccHost.getGenericArityOfClass(clazz);
|
||||
}
|
||||
|
||||
getImportOfIdentifier(id: ts.Identifier): Import|null {
|
||||
if (isFromDtsFile(id)) {
|
||||
return this.tsHost.getImportOfIdentifier(id);
|
||||
}
|
||||
return this.ngccHost.getImportOfIdentifier(id);
|
||||
}
|
||||
|
||||
getInternalNameOfClass(clazz: ClassDeclaration): ts.Identifier {
|
||||
if (isFromDtsFile(clazz)) {
|
||||
return this.tsHost.getInternalNameOfClass(clazz);
|
||||
}
|
||||
return this.ngccHost.getInternalNameOfClass(clazz);
|
||||
}
|
||||
|
||||
getAdjacentNameOfClass(clazz: ClassDeclaration): ts.Identifier {
|
||||
if (isFromDtsFile(clazz)) {
|
||||
return this.tsHost.getAdjacentNameOfClass(clazz);
|
||||
}
|
||||
return this.ngccHost.getAdjacentNameOfClass(clazz);
|
||||
}
|
||||
|
||||
getMembersOfClass(clazz: ClassDeclaration): ClassMember[] {
|
||||
if (isFromDtsFile(clazz)) {
|
||||
return this.tsHost.getMembersOfClass(clazz);
|
||||
}
|
||||
return this.ngccHost.getMembersOfClass(clazz);
|
||||
}
|
||||
|
||||
getVariableValue(declaration: ts.VariableDeclaration): ts.Expression|null {
|
||||
if (isFromDtsFile(declaration)) {
|
||||
return this.tsHost.getVariableValue(declaration);
|
||||
}
|
||||
return this.ngccHost.getVariableValue(declaration);
|
||||
}
|
||||
|
||||
hasBaseClass(clazz: ClassDeclaration): boolean {
|
||||
if (isFromDtsFile(clazz)) {
|
||||
return this.tsHost.hasBaseClass(clazz);
|
||||
}
|
||||
return this.ngccHost.hasBaseClass(clazz);
|
||||
}
|
||||
|
||||
getBaseClassExpression(clazz: ClassDeclaration): ts.Expression|null {
|
||||
if (isFromDtsFile(clazz)) {
|
||||
return this.tsHost.getBaseClassExpression(clazz);
|
||||
}
|
||||
return this.ngccHost.getBaseClassExpression(clazz);
|
||||
}
|
||||
|
||||
isClass(node: ts.Node): node is ClassDeclaration {
|
||||
if (isFromDtsFile(node)) {
|
||||
return this.tsHost.isClass(node);
|
||||
}
|
||||
return this.ngccHost.isClass(node);
|
||||
}
|
||||
|
||||
// Note: the methods below are specific to ngcc and the entry-point that is being compiled, so
|
||||
// they don't take declaration files into account.
|
||||
|
||||
findClassSymbols(sourceFile: ts.SourceFile): NgccClassSymbol[] {
|
||||
return this.ngccHost.findClassSymbols(sourceFile);
|
||||
}
|
||||
|
||||
getClassSymbol(node: ts.Node): NgccClassSymbol|undefined {
|
||||
return this.ngccHost.getClassSymbol(node);
|
||||
}
|
||||
|
||||
getDecoratorsOfSymbol(symbol: NgccClassSymbol): Decorator[]|null {
|
||||
return this.ngccHost.getDecoratorsOfSymbol(symbol);
|
||||
}
|
||||
|
||||
getModuleWithProvidersFunctions(sf: ts.SourceFile): ModuleWithProvidersFunction[] {
|
||||
return this.ngccHost.getModuleWithProvidersFunctions(sf);
|
||||
}
|
||||
|
||||
getSwitchableDeclarations(module: ts.Node): SwitchableVariableDeclaration[] {
|
||||
return this.ngccHost.getSwitchableDeclarations(module);
|
||||
}
|
||||
|
||||
getEndOfClass(classSymbol: NgccClassSymbol): ts.Node {
|
||||
return this.ngccHost.getEndOfClass(classSymbol);
|
||||
}
|
||||
}
|
||||
|
|
@ -41,8 +41,6 @@ export class Esm5ReflectionHost extends Esm2015ReflectionHost {
|
|||
* @param clazz a `ClassDeclaration` representing the class over which to reflect.
|
||||
*/
|
||||
hasBaseClass(clazz: ClassDeclaration): boolean {
|
||||
if (super.hasBaseClass(clazz)) return true;
|
||||
|
||||
const classSymbol = this.getClassSymbol(clazz);
|
||||
if (classSymbol === undefined) {
|
||||
return false;
|
||||
|
|
@ -58,11 +56,6 @@ export class Esm5ReflectionHost extends Esm2015ReflectionHost {
|
|||
}
|
||||
|
||||
getBaseClassExpression(clazz: ClassDeclaration): ts.Expression|null {
|
||||
const superBaseClassIdentifier = super.getBaseClassExpression(clazz);
|
||||
if (superBaseClassIdentifier) {
|
||||
return superBaseClassIdentifier;
|
||||
}
|
||||
|
||||
const classSymbol = this.getClassSymbol(clazz);
|
||||
if (classSymbol === undefined) {
|
||||
return null;
|
||||
|
|
|
|||
|
|
@ -33,11 +33,6 @@ export class UmdReflectionHost extends Esm5ReflectionHost {
|
|||
}
|
||||
|
||||
getImportOfIdentifier(id: ts.Identifier): Import|null {
|
||||
const superImport = super.getImportOfIdentifier(id);
|
||||
if (superImport !== null) {
|
||||
return superImport;
|
||||
}
|
||||
|
||||
// Is `id` a namespaced property access, e.g. `Directive` in `core.Directive`?
|
||||
// If so capture the symbol of the namespace, e.g. `core`.
|
||||
const nsIdentifier = findNamespaceOfIdentifier(id);
|
||||
|
|
@ -47,8 +42,7 @@ export class UmdReflectionHost extends Esm5ReflectionHost {
|
|||
}
|
||||
|
||||
getDeclarationOfIdentifier(id: ts.Identifier): Declaration|null {
|
||||
return (!id.getSourceFile().isDeclarationFile && this.getUmdImportedDeclaration(id)) ||
|
||||
super.getDeclarationOfIdentifier(id);
|
||||
return this.getUmdImportedDeclaration(id) || super.getDeclarationOfIdentifier(id);
|
||||
}
|
||||
|
||||
getExportsOfModule(module: ts.Node): Map<string, Declaration>|null {
|
||||
|
|
|
|||
|
|
@ -8,6 +8,7 @@
|
|||
import * as ts from 'typescript';
|
||||
|
||||
import {FileSystem} from '../../../src/ngtsc/file_system';
|
||||
import {TypeScriptReflectionHost} from '../../../src/ngtsc/reflection';
|
||||
import {DecorationAnalyzer} from '../analysis/decoration_analyzer';
|
||||
import {ModuleWithProvidersAnalyses, ModuleWithProvidersAnalyzer} from '../analysis/module_with_providers_analyzer';
|
||||
import {NgccReferencesRegistry} from '../analysis/ngcc_references_registry';
|
||||
|
|
@ -15,6 +16,7 @@ import {ExportInfo, PrivateDeclarationsAnalyzer} from '../analysis/private_decla
|
|||
import {SwitchMarkerAnalyses, SwitchMarkerAnalyzer} from '../analysis/switch_marker_analyzer';
|
||||
import {CompiledFile} from '../analysis/types';
|
||||
import {CommonJsReflectionHost} from '../host/commonjs_host';
|
||||
import {DelegatingReflectionHost} from '../host/delegating_host';
|
||||
import {Esm2015ReflectionHost} from '../host/esm2015_host';
|
||||
import {Esm5ReflectionHost} from '../host/esm5_host';
|
||||
import {NgccReflectionHost} from '../host/ngcc_host';
|
||||
|
|
@ -69,7 +71,9 @@ export class Transformer {
|
|||
* @returns information about the files that were transformed.
|
||||
*/
|
||||
transform(bundle: EntryPointBundle): TransformResult {
|
||||
const reflectionHost = this.getHost(bundle);
|
||||
const ngccReflectionHost = this.getHost(bundle);
|
||||
const tsReflectionHost = new TypeScriptReflectionHost(bundle.src.program.getTypeChecker());
|
||||
const reflectionHost = new DelegatingReflectionHost(tsReflectionHost, ngccReflectionHost);
|
||||
|
||||
// Parse and analyze the files.
|
||||
const {decorationAnalyses, switchMarkerAnalyses, privateDeclarationsAnalyses,
|
||||
|
|
@ -81,7 +85,7 @@ export class Transformer {
|
|||
}
|
||||
|
||||
// Transform the source files and source maps.
|
||||
const srcFormatter = this.getRenderingFormatter(reflectionHost, bundle);
|
||||
const srcFormatter = this.getRenderingFormatter(ngccReflectionHost, bundle);
|
||||
|
||||
const renderer = new Renderer(reflectionHost, srcFormatter, this.fs, this.logger, bundle);
|
||||
let renderedFiles = renderer.renderProgram(
|
||||
|
|
|
|||
|
|
@ -1607,30 +1607,6 @@ exports.ExternalModule = ExternalModule;
|
|||
expect(importOfIdent).toEqual({name: 'a', from: './file_a'});
|
||||
});
|
||||
|
||||
it('should find the import of an identifier in a declaration file', () => {
|
||||
loadTestFiles([
|
||||
{
|
||||
name: _('/index.d.ts'),
|
||||
contents: `
|
||||
import {MyClass} from './myclass.d.ts';
|
||||
export declare const a: MyClass;`
|
||||
},
|
||||
{
|
||||
name: _('/myclass.d.ts'),
|
||||
contents: `export declare class MyClass {}`,
|
||||
}
|
||||
]);
|
||||
const bundle = makeTestBundleProgram(_('/index.d.ts'));
|
||||
const host = new CommonJsReflectionHost(new MockLogger(), false, bundle);
|
||||
const variableNode =
|
||||
getDeclaration(bundle.program, _('/index.d.ts'), 'a', isNamedVariableDeclaration);
|
||||
const identifier =
|
||||
((variableNode.type as ts.TypeReferenceNode).typeName as ts.Identifier);
|
||||
|
||||
const importOfIdent = host.getImportOfIdentifier(identifier !);
|
||||
expect(importOfIdent).toEqual({name: 'MyClass', from: './myclass.d.ts'});
|
||||
});
|
||||
|
||||
it('should return null if the identifier was not imported', () => {
|
||||
loadTestFiles(IMPORTS_FILES);
|
||||
const bundle = makeTestBundleProgram(_('/index.js'));
|
||||
|
|
@ -1816,39 +1792,6 @@ exports.ExternalModule = ExternalModule;
|
|||
expect(importOfIdent.viaModule).toBe('lib');
|
||||
});
|
||||
|
||||
it('should return the correct declaration of an identifier imported in a typings file',
|
||||
() => {
|
||||
const files = [
|
||||
{
|
||||
name: _('/node_modules/test-package/index.d.ts'),
|
||||
contents: `
|
||||
import {SubModule} from 'sub_module';
|
||||
export const x = SubModule;
|
||||
`,
|
||||
},
|
||||
{
|
||||
name: _('/node_modules/sub_module/index.d.ts'),
|
||||
contents: 'export class SubModule {}',
|
||||
}
|
||||
];
|
||||
loadTestFiles(files);
|
||||
const bundle = makeTestBundleProgram(files[0].name);
|
||||
const host = new CommonJsReflectionHost(new MockLogger(), false, bundle);
|
||||
const expectedDeclaration = getDeclaration(
|
||||
bundle.program, files[1].name, 'SubModule', isNamedClassDeclaration);
|
||||
const x =
|
||||
getDeclaration(bundle.program, files[0].name, 'x', isNamedVariableDeclaration);
|
||||
if (x.initializer === undefined || !ts.isIdentifier(x.initializer)) {
|
||||
return fail('Expected constant `x` to have an identifer as an initializer.');
|
||||
}
|
||||
const decl = host.getDeclarationOfIdentifier(x.initializer);
|
||||
if (decl === null) {
|
||||
return fail('Expected to find a declaration for ' + x.initializer.getText());
|
||||
}
|
||||
expect(decl.viaModule).toEqual('sub_module');
|
||||
expect(decl.node).toBe(expectedDeclaration);
|
||||
});
|
||||
|
||||
it('should recognize TypeScript helpers (as function declarations)', () => {
|
||||
const file: TestFile = {
|
||||
name: _('/test.js'),
|
||||
|
|
|
|||
|
|
@ -1776,29 +1776,6 @@ runInEachFileSystem(() => {
|
|||
expect(importOfIdent).toEqual({name: 'a', from: './file_a'});
|
||||
});
|
||||
|
||||
it('should find the import of an identifier in a declaration file', () => {
|
||||
loadTestFiles([
|
||||
{
|
||||
name: _('/index.d.ts'),
|
||||
contents: `
|
||||
import {MyClass} from './myclass.d.ts';
|
||||
export declare const a: MyClass;`
|
||||
},
|
||||
{
|
||||
name: _('/myclass.d.ts'),
|
||||
contents: `export declare class MyClass {}`,
|
||||
}
|
||||
]);
|
||||
const bundle = makeTestBundleProgram(_('/index.d.ts'));
|
||||
const host = new UmdReflectionHost(new MockLogger(), false, bundle);
|
||||
const variableNode =
|
||||
getDeclaration(bundle.program, _('/index.d.ts'), 'a', isNamedVariableDeclaration);
|
||||
const identifier = ((variableNode.type as ts.TypeReferenceNode).typeName as ts.Identifier);
|
||||
|
||||
const importOfIdent = host.getImportOfIdentifier(identifier !);
|
||||
expect(importOfIdent).toEqual({name: 'MyClass', from: './myclass.d.ts'});
|
||||
});
|
||||
|
||||
it('should return null if the identifier was not imported', () => {
|
||||
loadTestFiles(IMPORTS_FILES);
|
||||
const bundle = makeTestBundleProgram(_('/index.js'));
|
||||
|
|
@ -1942,39 +1919,6 @@ runInEachFileSystem(() => {
|
|||
expect(actualDeclaration !.viaModule).toBe('@angular/core');
|
||||
});
|
||||
|
||||
it('should return the correct declaration of an identifier imported in a typings file',
|
||||
() => {
|
||||
|
||||
const FILES = [
|
||||
{
|
||||
name: _('/node_modules/test-package/index.d.ts'),
|
||||
contents: `
|
||||
import {SubModule} from 'sub_module';
|
||||
export const x = SubModule;
|
||||
`,
|
||||
},
|
||||
{
|
||||
name: _('/node_modules/sub_module/index.d.ts'),
|
||||
contents: `export class SubModule {}`,
|
||||
}
|
||||
];
|
||||
loadTestFiles(FILES);
|
||||
const bundle = makeTestBundleProgram(FILES[0].name);
|
||||
const host = new UmdReflectionHost(new MockLogger(), false, bundle);
|
||||
const expectedDeclaration =
|
||||
getDeclaration(bundle.program, FILES[1].name, 'SubModule', isNamedClassDeclaration);
|
||||
const x = getDeclaration(bundle.program, FILES[0].name, 'x', isNamedVariableDeclaration);
|
||||
if (x.initializer === undefined || !ts.isIdentifier(x.initializer)) {
|
||||
return fail('Expected constant `x` to have an identifer as an initializer.');
|
||||
}
|
||||
const decl = host.getDeclarationOfIdentifier(x.initializer);
|
||||
if (decl === null) {
|
||||
return fail('Expected to find a declaration for ' + x.initializer.getText());
|
||||
}
|
||||
expect(decl.viaModule).toEqual('sub_module');
|
||||
expect(decl.node).toBe(expectedDeclaration);
|
||||
});
|
||||
|
||||
it('should recognize TypeScript helpers (as function declarations)', () => {
|
||||
const file: TestFile = {
|
||||
name: _('/test.js'),
|
||||
|
|
|
|||
|
|
@ -249,6 +249,70 @@ runInEachFileSystem(() => {
|
|||
expect(jsContents).not.toMatch(/\bconst \w+\s*=/);
|
||||
});
|
||||
|
||||
it('should be able to reflect into external libraries', () => {
|
||||
compileIntoApf('lib', {
|
||||
'/index.ts': `
|
||||
export * from './constants';
|
||||
export * from './module';
|
||||
`,
|
||||
'/constants.ts': `
|
||||
export const selectorA = '[selector-a]';
|
||||
|
||||
export class Selectors {
|
||||
static readonly B = '[selector-b]';
|
||||
}
|
||||
`,
|
||||
'/module.ts': `
|
||||
import {NgModule, ModuleWithProviders} from '@angular/core';
|
||||
|
||||
@NgModule()
|
||||
export class MyOtherModule {}
|
||||
|
||||
export class MyModule {
|
||||
static forRoot(): ModuleWithProviders<MyOtherModule> {
|
||||
return {ngModule: MyOtherModule};
|
||||
}
|
||||
}
|
||||
`
|
||||
});
|
||||
|
||||
compileIntoFlatEs5Package('test-package', {
|
||||
'/index.ts': `
|
||||
import {Directive, Input, NgModule} from '@angular/core';
|
||||
import * as lib from 'lib';
|
||||
|
||||
@Directive({
|
||||
selector: lib.selectorA,
|
||||
})
|
||||
export class DirectiveA {
|
||||
}
|
||||
|
||||
@Directive({
|
||||
selector: lib.Selectors.B,
|
||||
})
|
||||
export class DirectiveB {
|
||||
}
|
||||
|
||||
@NgModule({
|
||||
imports: [lib.MyModule.forRoot()],
|
||||
declarations: [DirectiveA, DirectiveB],
|
||||
})
|
||||
export class FooModule {}
|
||||
`,
|
||||
});
|
||||
|
||||
mainNgcc({
|
||||
basePath: '/node_modules',
|
||||
targetEntryPointPath: 'test-package',
|
||||
propertiesToConsider: ['module'],
|
||||
});
|
||||
|
||||
const jsContents = fs.readFile(_(`/node_modules/test-package/index.js`));
|
||||
expect(jsContents).toContain('"selector-a"');
|
||||
expect(jsContents).toContain('"selector-b"');
|
||||
expect(jsContents).toContain('imports: [ɵngcc1.MyOtherModule]');
|
||||
});
|
||||
|
||||
it('should add ɵfac but not duplicate ɵprov properties on injectables', () => {
|
||||
compileIntoFlatEs5Package('test-package', {
|
||||
'/index.ts': `
|
||||
|
|
|
|||
Loading…
Reference in New Issue