From ae364864f626bbe29a34c933a4a02b55f6337587 Mon Sep 17 00:00:00 2001 From: Pete Bacon Darwin Date: Tue, 9 Jun 2020 12:34:28 +0100 Subject: [PATCH] fix(ngcc): do not scan import expressions in d.ts files (#37503) It is quite common for the TS compiler to have to add synthetic types to function signatures, where the developer has not explicitly provided them. This results in `import(...)` expressions appearing in typings files. For example in `@ngrx/data` there is a class with a getter that has an implicit type: ```ts export declare class EntityCollectionServiceBase<...> { ... get store() { return this.dispatcher.store; } ... } ``` In the d.ts file for this we get: ```ts get store(): Store; ``` Given that this file is within the `@ngrx/data` package already, this caused ngcc to believe that there was a circular dependency, causing it to fail to process the package - and in fact crash! This commit resolves this problem by ignoring `import()` expressions when scanning typings programs for dependencies. This ability was only introduced very recently in a 10.0.0 RC release, and so it has limited benefit given that up till now ngcc has been able to process libraries effectively without it. Moreover, in the rare case that a package does have such a dependency, it should get picked up by the sync ngcc+CLI integration point. PR Close #37503 --- .../src/dependencies/dts_dependency_host.ts | 3 ++- .../src/dependencies/esm_dependency_host.ts | 9 ++++++-- .../dependencies/dts_dependency_host_spec.ts | 21 +++++++++++++++++++ 3 files changed, 30 insertions(+), 3 deletions(-) diff --git a/packages/compiler-cli/ngcc/src/dependencies/dts_dependency_host.ts b/packages/compiler-cli/ngcc/src/dependencies/dts_dependency_host.ts index 7c5545335a..c651c562b3 100644 --- a/packages/compiler-cli/ngcc/src/dependencies/dts_dependency_host.ts +++ b/packages/compiler-cli/ngcc/src/dependencies/dts_dependency_host.ts @@ -16,7 +16,8 @@ import {ModuleResolver} from './module_resolver'; export class DtsDependencyHost extends EsmDependencyHost { constructor(fs: FileSystem, pathMappings?: PathMappings) { super( - fs, new ModuleResolver(fs, pathMappings, ['', '.d.ts', '/index.d.ts', '.js', '/index.js'])); + fs, new ModuleResolver(fs, pathMappings, ['', '.d.ts', '/index.d.ts', '.js', '/index.js']), + false); } /** diff --git a/packages/compiler-cli/ngcc/src/dependencies/esm_dependency_host.ts b/packages/compiler-cli/ngcc/src/dependencies/esm_dependency_host.ts index 16e76af9e3..5ce08b8493 100644 --- a/packages/compiler-cli/ngcc/src/dependencies/esm_dependency_host.ts +++ b/packages/compiler-cli/ngcc/src/dependencies/esm_dependency_host.ts @@ -6,13 +6,18 @@ * found in the LICENSE file at https://angular.io/license */ import * as ts from 'typescript'; -import {AbsoluteFsPath} from '../../../src/ngtsc/file_system'; +import {AbsoluteFsPath, FileSystem} from '../../../src/ngtsc/file_system'; import {DependencyHostBase} from './dependency_host'; +import {ModuleResolver} from './module_resolver'; /** * Helper functions for computing dependencies. */ export class EsmDependencyHost extends DependencyHostBase { + constructor( + fs: FileSystem, moduleResolver: ModuleResolver, private scanImportExpressions = true) { + super(fs, moduleResolver); + } // By skipping trivia here we don't have to account for it in the processing below // It has no relevance to capturing imports. private scanner = ts.createScanner(ts.ScriptTarget.Latest, /* skipTrivia */ true); @@ -144,7 +149,7 @@ export class EsmDependencyHost extends DependencyHostBase { // Check for dynamic import expression if (kind === ts.SyntaxKind.OpenParenToken) { - return this.tryStringLiteral(); + return this.scanImportExpressions ? this.tryStringLiteral() : null; } // Check for defaultBinding diff --git a/packages/compiler-cli/ngcc/test/dependencies/dts_dependency_host_spec.ts b/packages/compiler-cli/ngcc/test/dependencies/dts_dependency_host_spec.ts index 18fb2e2895..291ae50e92 100644 --- a/packages/compiler-cli/ngcc/test/dependencies/dts_dependency_host_spec.ts +++ b/packages/compiler-cli/ngcc/test/dependencies/dts_dependency_host_spec.ts @@ -44,6 +44,15 @@ runInEachFileSystem(() => { expect(dependencies.has(_('/node_modules/lib-1/sub-1'))).toBe(true); }); + it('should ignore synthetic type imports', () => { + const {dependencies, missing, deepImports} = createDependencyInfo(); + host.collectDependencies( + _('/external/synthetic-type-imports/index.d.ts'), {dependencies, missing, deepImports}); + expect(dependencies.size).toBe(0); + expect(missing.size).toBe(0); + expect(deepImports.size).toBe(0); + }); + it('should resolve all the external re-exports of the source file', () => { const {dependencies, missing, deepImports} = createDependencyInfo(); host.collectDependencies( @@ -165,6 +174,18 @@ runInEachFileSystem(() => { }, {name: _('/external/imports/package.json'), contents: '{"esm2015": "./index.js"}'}, {name: _('/external/imports/index.metadata.json'), contents: 'MOCK METADATA'}, + { + name: _('/external/synthetic-type-imports/index.d.ts'), + contents: `const function foo(): Array;` + }, + { + name: _('/external/synthetic-type-imports/package.json'), + contents: '{"esm2015": "./index.js"}' + }, + { + name: _('/external/synthetic-type-imports/index.metadata.json'), + contents: 'MOCK METADATA' + }, { name: _('/external/re-exports/index.d.ts'), contents: `export {X} from 'lib-1';\nexport {Y} from 'lib-1/sub-1';`