From 3c7da767d8faa2dd231f57cf75708e9f942bb9fe Mon Sep 17 00:00:00 2001 From: JoostK Date: Thu, 12 Sep 2019 21:08:21 +0200 Subject: [PATCH] fix(ngcc): resolve imports in `.d.ts` files for UMD/CommonJS bundles (#32619) In ngcc's reflection host for UMD and CommonJS bundles, custom logic is present to resolve import details of an identifier. However, this custom logic is unable to resolve an import for an identifier inside of declaration files, as such files use the regular ESM import syntax. As a consequence of this limitation, ngtsc is unable to resolve `ModuleWithProviders` imports that are declared in an external library. In that situation, ngtsc determines the type of the actual `NgModule` that is imported, by looking in the library's declaration files for the generic type argument on `ModuleWithProviders`. In this process, ngtsc resolves the import for the `ModuleWithProviders` identifier to verify that it is indeed the `ModuleWithProviders` type from `@angular/core`. So, when the UMD reflection host was in use this resolution would fail, therefore no `NgModule` type could be detected. This commit fixes the bug by using the regular import resolution logic in addition to the custom resolution logic that is required for UMD and CommonJS bundles. Fixes #31791 PR Close #32619 --- .../ngcc/src/host/commonjs_host.ts | 5 ++++ .../compiler-cli/ngcc/src/host/umd_host.ts | 5 ++++ .../ngcc/test/host/commonjs_host_spec.ts | 26 ++++++++++++++++++- .../ngcc/test/host/umd_host_spec.ts | 25 +++++++++++++++++- 4 files changed, 59 insertions(+), 2 deletions(-) diff --git a/packages/compiler-cli/ngcc/src/host/commonjs_host.ts b/packages/compiler-cli/ngcc/src/host/commonjs_host.ts index f752412df3..a5ac0a77b5 100644 --- a/packages/compiler-cli/ngcc/src/host/commonjs_host.ts +++ b/packages/compiler-cli/ngcc/src/host/commonjs_host.ts @@ -26,6 +26,11 @@ 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; diff --git a/packages/compiler-cli/ngcc/src/host/umd_host.ts b/packages/compiler-cli/ngcc/src/host/umd_host.ts index 89b401106d..5bf2060580 100644 --- a/packages/compiler-cli/ngcc/src/host/umd_host.ts +++ b/packages/compiler-cli/ngcc/src/host/umd_host.ts @@ -25,6 +25,11 @@ export class UmdReflectionHost extends Esm5ReflectionHost { } getImportOfIdentifier(id: ts.Identifier): Import|null { + const superImport = super.getImportOfIdentifier(id); + if (superImport !== null) { + return superImport; + } + const importParameter = this.findUmdImportParameter(id); const from = importParameter && this.getUmdImportPath(importParameter); return from !== null ? {from, name: id.text} : null; 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 a688fcf630..67e6c93102 100644 --- a/packages/compiler-cli/ngcc/test/host/commonjs_host_spec.ts +++ b/packages/compiler-cli/ngcc/test/host/commonjs_host_spec.ts @@ -9,7 +9,7 @@ import * as ts from 'typescript'; import {absoluteFrom, getFileSystem, getSourceFileOrError} from '../../../src/ngtsc/file_system'; import {TestFile, runInEachFileSystem} from '../../../src/ngtsc/file_system/testing'; -import {ClassMemberKind, CtorParameter, Import, InlineDeclaration, isNamedClassDeclaration, isNamedFunctionDeclaration, isNamedVariableDeclaration} from '../../../src/ngtsc/reflection'; +import {ClassMemberKind, CtorParameter, InlineDeclaration, isNamedClassDeclaration, isNamedFunctionDeclaration, isNamedVariableDeclaration} from '../../../src/ngtsc/reflection'; import {getDeclaration} from '../../../src/ngtsc/testing'; import {loadFakeCore, loadTestFiles} from '../../../test/helpers'; import {CommonJsReflectionHost} from '../../src/host/commonjs_host'; @@ -1558,6 +1558,30 @@ 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 {program, host: compilerHost} = makeTestBundleProgram(_('/index.d.ts')); + const host = new CommonJsReflectionHost(new MockLogger(), false, program, compilerHost); + const variableNode = + getDeclaration(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 {program, host: compilerHost} = makeTestBundleProgram(_('/index.js')); 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 74566a1bee..52a5ec58b9 100644 --- a/packages/compiler-cli/ngcc/test/host/umd_host_spec.ts +++ b/packages/compiler-cli/ngcc/test/host/umd_host_spec.ts @@ -13,7 +13,6 @@ import {TestFile, runInEachFileSystem} from '../../../src/ngtsc/file_system/test import {ClassMemberKind, CtorParameter, Import, isNamedClassDeclaration, isNamedFunctionDeclaration, isNamedVariableDeclaration} from '../../../src/ngtsc/reflection'; import {getDeclaration} from '../../../src/ngtsc/testing'; import {loadFakeCore, loadTestFiles} from '../../../test/helpers'; -import {Esm2015ReflectionHost} from '../../src/host/esm2015_host'; import {getIifeBody} from '../../src/host/esm5_host'; import {UmdReflectionHost} from '../../src/host/umd_host'; import {MockLogger} from '../helpers/mock_logger'; @@ -1658,6 +1657,30 @@ 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 {program, host: compilerHost} = makeTestBundleProgram(_('/index.d.ts')); + const host = new UmdReflectionHost(new MockLogger(), false, program, compilerHost); + const variableNode = + getDeclaration(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 {program, host: compilerHost} = makeTestBundleProgram(_('/index.js'));