From 1d141a8ab16c6db310b9a048a5017d3b6d013db8 Mon Sep 17 00:00:00 2001 From: Pete Bacon Darwin Date: Wed, 30 Oct 2019 19:02:30 +0000 Subject: [PATCH] fix(compiler-cli): attach the correct `viaModule` to namespace imports (#33495) Previously declarations that were imported via a namespace import were given the same `bestGuessOwningModule` as the context where they were imported to. This causes problems with resolving `ModuleWithProviders` that have a type that has been imported in this way, causing errors like: ``` ERROR in Symbol UIRouterModule declared in .../@uirouter/angular/uiRouterNgModule.d.ts is not exported from .../@uirouter/angular/uirouter-angular.d.ts (import into .../src/app/child.module.ts) ``` This commit modifies the `TypescriptReflectionHost.getDirectImportOfIdentifier()` method so that it also understands how to attach the correct `viaModule` to the identifier of the namespace import. Resolves #32166 PR Close #33495 --- .../ngcc/test/host/esm2015_host_spec.ts | 2 +- .../ngcc/test/host/esm5_host_spec.ts | 2 +- .../src/ngtsc/reflection/src/typescript.ts | 37 +++++++++++++------ .../compiler-cli/test/ngtsc/ngtsc_spec.ts | 35 ++++++++++++++++++ 4 files changed, 62 insertions(+), 14 deletions(-) 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 c29afece754e9..f17eea2e9dc77 100644 --- a/packages/compiler-cli/ngcc/test/host/esm2015_host_spec.ts +++ b/packages/compiler-cli/ngcc/test/host/esm2015_host_spec.ts @@ -1558,7 +1558,7 @@ runInEachFileSystem(() => { const actualDeclaration = host.getDeclarationOfIdentifier(identifier); expect(actualDeclaration).not.toBe(null); expect(actualDeclaration !.node).toBe(expectedDeclarationNode); - expect(actualDeclaration !.viaModule).toBe(null); + expect(actualDeclaration !.viaModule).toBe('@angular/core'); }); it('should return the original declaration of an aliased class', () => { 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 8865c5b03e94d..d46e6a0639295 100644 --- a/packages/compiler-cli/ngcc/test/host/esm5_host_spec.ts +++ b/packages/compiler-cli/ngcc/test/host/esm5_host_spec.ts @@ -1788,7 +1788,7 @@ runInEachFileSystem(() => { const actualDeclaration = host.getDeclarationOfIdentifier(identifier); expect(actualDeclaration).not.toBe(null); expect(actualDeclaration !.node).toBe(expectedDeclarationNode); - expect(actualDeclaration !.viaModule).toBe(null); + expect(actualDeclaration !.viaModule).toBe('@angular/core'); }); it('should return the correct declaration for an inner function identifier inside an ES5 IIFE', diff --git a/packages/compiler-cli/src/ngtsc/reflection/src/typescript.ts b/packages/compiler-cli/src/ngtsc/reflection/src/typescript.ts index e0e092d5cbe39..7853bc08858bc 100644 --- a/packages/compiler-cli/src/ngtsc/reflection/src/typescript.ts +++ b/packages/compiler-cli/src/ngtsc/reflection/src/typescript.ts @@ -191,28 +191,21 @@ export class TypeScriptReflectionHost implements ReflectionHost { return null; } - // Ignore decorators that are defined locally (not imported). const decl: ts.Declaration = symbol.declarations[0]; - if (!ts.isImportSpecifier(decl)) { + const importDecl = getContainingImportDeclaration(decl); + + // Ignore declarations that are defined locally (not imported). + if (importDecl === null) { return null; } - // Walk back from the specifier to find the declaration, which carries the module specifier. - const importDecl = decl.parent !.parent !.parent !; - // The module specifier is guaranteed to be a string literal, so this should always pass. if (!ts.isStringLiteral(importDecl.moduleSpecifier)) { // Not allowed to happen in TypeScript ASTs. return null; } - // Read the module specifier. - const from = importDecl.moduleSpecifier.text; - - // Compute the name by which the decorator was exported, not imported. - const name = (decl.propertyName !== undefined ? decl.propertyName : decl.name).text; - - return {from, name}; + return {from: importDecl.moduleSpecifier.text, name: getExportedName(decl, id)}; } /** @@ -549,3 +542,23 @@ function getFarLeftIdentifier(propertyAccess: ts.PropertyAccessExpression): ts.I } return ts.isIdentifier(propertyAccess.expression) ? propertyAccess.expression : null; } + +/** + * Return the ImportDeclaration for the given `node` if it is either an `ImportSpecifier` or a + * `NamespaceImport`. If not return `null`. + */ +function getContainingImportDeclaration(node: ts.Node): ts.ImportDeclaration|null { + return ts.isImportSpecifier(node) ? node.parent !.parent !.parent ! : + ts.isNamespaceImport(node) ? node.parent.parent : null; +} + +/** + * Compute the name by which the `decl` was exported, not imported. + * If no such declaration can be found (e.g. it is a namespace import) + * then fallback to the `originalId`. + */ +function getExportedName(decl: ts.Declaration, originalId: ts.Identifier): string { + return ts.isImportSpecifier(decl) ? + (decl.propertyName !== undefined ? decl.propertyName : decl.name).text : + originalId.text; +} \ No newline at end of file diff --git a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts index df808e4bdbaa8..49e46d5b84d08 100644 --- a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts +++ b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts @@ -1737,6 +1737,41 @@ runInEachFileSystem(os => { 'i0.ɵɵNgModuleDefWithMeta'); }); + it('should extract the generic type if it is provided as qualified type name from another package', + () => { + env.write(`test.ts`, ` + import {NgModule} from '@angular/core'; + import {RouterModule} from 'router'; + + @NgModule({imports: [RouterModule.forRoot()]}) + export class TestModule {}`); + + env.write('node_modules/router/index.d.ts', ` + import {ModuleWithProviders} from '@angular/core'; + import * as router2 from 'router2'; + + declare export class RouterModule { + static forRoot(): ModuleWithProviders; + }`); + + env.write('node_modules/router2/index.d.ts', ` + import {ɵɵNgModuleDefWithMeta} from '@angular/core'; + export declare class Router2Module { + static ɵmod: ɵɵNgModuleDefWithMeta; + }`); + + env.driveMain(); + + const jsContents = env.getContents('test.js'); + expect(jsContents).toContain('imports: [[RouterModule.forRoot()]]'); + + const dtsContents = env.getContents('test.d.ts'); + expect(dtsContents).toContain(`import * as i1 from "router2";`); + expect(dtsContents) + .toContain( + 'i0.ɵɵNgModuleDefWithMeta'); + }); + it('should not reference a constant with a ModuleWithProviders value in module def imports', () => { env.write('dep.d.ts', `