From 85b5c365fcc855b1216db33aa33034a3e03252dc Mon Sep 17 00:00:00 2001 From: Pete Bacon Darwin Date: Thu, 9 Jan 2020 14:23:50 +0000 Subject: [PATCH] fix(ngcc): do not add DTS deep imports to missing packages list (#34695) When searching the typings program for a package for imports a distinction is drawn between missing entry-points and deep imports. Previously in the `DtsDependencyHost` these deep imports may be marked as missing if there was no typings file at the deep import path. Instead there may be a javascript file instead. In practice this means the import is "deep" and not "missing". Now the `DtsDependencyHost` will also consider `.js` files when checking for deep-imports, and it will also look inside `@types/...` for a suitable deep-imported typings file. Fixes #34720 PR Close #34695 --- .../src/dependencies/dts_dependency_host.ts | 16 +- .../src/dependencies/esm_dependency_host.ts | 49 ++-- .../dependencies/dts_dependency_host_spec.ts | 253 ++++++++++++++++++ 3 files changed, 298 insertions(+), 20 deletions(-) create mode 100644 packages/compiler-cli/ngcc/test/dependencies/dts_dependency_host_spec.ts 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 73efa1b744..5d23c596a7 100644 --- a/packages/compiler-cli/ngcc/src/dependencies/dts_dependency_host.ts +++ b/packages/compiler-cli/ngcc/src/dependencies/dts_dependency_host.ts @@ -5,7 +5,7 @@ * 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 {FileSystem} from '../../../src/ngtsc/file_system'; +import {AbsoluteFsPath, FileSystem} from '../../../src/ngtsc/file_system'; import {PathMappings} from '../utils'; import {EsmDependencyHost} from './esm_dependency_host'; import {ModuleResolver} from './module_resolver'; @@ -15,6 +15,18 @@ 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'])); + super( + fs, new ModuleResolver(fs, pathMappings, ['', '.d.ts', '/index.d.ts', '.js', '/index.js'])); + } + + /** + * Attempts to process the `importPath` directly and also inside `@types/...`. + */ + protected processImport( + importPath: string, file: AbsoluteFsPath, dependencies: Set, + missing: Set, deepImports: Set, alreadySeen: Set): boolean { + return super.processImport(importPath, file, dependencies, missing, deepImports, alreadySeen) || + super.processImport( + `@types/${importPath}`, file, dependencies, missing, deepImports, alreadySeen); } } 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 6b2c1226d1..3c4d6e80ee 100644 --- a/packages/compiler-cli/ngcc/src/dependencies/esm_dependency_host.ts +++ b/packages/compiler-cli/ngcc/src/dependencies/esm_dependency_host.ts @@ -44,29 +44,42 @@ export class EsmDependencyHost extends DependencyHostBase { .filter(isStringImportOrReexport) // Grab the id of the module that is being imported .map(stmt => stmt.moduleSpecifier.text) - // Resolve this module id into an absolute path .forEach(importPath => { - const resolvedModule = this.moduleResolver.resolveModuleImport(importPath, file); - if (resolvedModule) { - if (resolvedModule instanceof ResolvedRelativeModule) { - const internalDependency = resolvedModule.modulePath; - if (!alreadySeen.has(internalDependency)) { - alreadySeen.add(internalDependency); - this.recursivelyCollectDependencies( - internalDependency, dependencies, missing, deepImports, alreadySeen); - } - } else { - if (resolvedModule instanceof ResolvedDeepImport) { - deepImports.add(resolvedModule.importPath); - } else { - dependencies.add(resolvedModule.entryPointPath); - } - } - } else { + const resolved = + this.processImport(importPath, file, dependencies, missing, deepImports, alreadySeen); + if (!resolved) { missing.add(importPath); } }); } + + /** + * Resolve the given `importPath` from `file` and add it to the appropriate set. + * + * @returns `true` if the import was resolved (to an entry-point, a local import, or a + * deep-import). + */ + protected processImport( + importPath: string, file: AbsoluteFsPath, dependencies: Set, + missing: Set, deepImports: Set, alreadySeen: Set): boolean { + const resolvedModule = this.moduleResolver.resolveModuleImport(importPath, file); + if (resolvedModule === null) { + return false; + } + if (resolvedModule instanceof ResolvedRelativeModule) { + const internalDependency = resolvedModule.modulePath; + if (!alreadySeen.has(internalDependency)) { + alreadySeen.add(internalDependency); + this.recursivelyCollectDependencies( + internalDependency, dependencies, missing, deepImports, alreadySeen); + } + } else if (resolvedModule instanceof ResolvedDeepImport) { + deepImports.add(resolvedModule.importPath); + } else { + dependencies.add(resolvedModule.entryPointPath); + } + return true; + } } /** 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 new file mode 100644 index 0000000000..7f5d360e83 --- /dev/null +++ b/packages/compiler-cli/ngcc/test/dependencies/dts_dependency_host_spec.ts @@ -0,0 +1,253 @@ +/** + * @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 {absoluteFrom, getFileSystem, relativeFrom} from '../../../src/ngtsc/file_system'; +import {runInEachFileSystem} from '../../../src/ngtsc/file_system/testing'; +import {loadTestFiles} from '../../../test/helpers'; +import {createDependencyInfo} from '../../src/dependencies/dependency_host'; +import {DtsDependencyHost} from '../../src/dependencies/dts_dependency_host'; + +runInEachFileSystem(() => { + + describe('DtsDependencyHost', () => { + let _: typeof absoluteFrom; + let host: DtsDependencyHost; + beforeEach(() => { + _ = absoluteFrom; + setupMockFileSystem(); + const fs = getFileSystem(); + host = new DtsDependencyHost(fs); + }); + + describe('getDependencies()', () => { + it('should not generate a TS AST if the source does not contain any imports or re-exports', + () => { + spyOn(ts, 'createSourceFile'); + host.collectDependencies( + _('/no/imports/or/re-exports/index.d.ts'), createDependencyInfo()); + expect(ts.createSourceFile).not.toHaveBeenCalled(); + }); + + it('should resolve all the external imports of the source file', () => { + const {dependencies, missing, deepImports} = createDependencyInfo(); + host.collectDependencies( + _('/external/imports/index.d.ts'), {dependencies, missing, deepImports}); + expect(dependencies.size).toBe(2); + expect(missing.size).toBe(0); + expect(deepImports.size).toBe(0); + expect(dependencies.has(_('/node_modules/lib-1'))).toBe(true); + expect(dependencies.has(_('/node_modules/lib-1/sub-1'))).toBe(true); + }); + + it('should resolve all the external re-exports of the source file', () => { + const {dependencies, missing, deepImports} = createDependencyInfo(); + host.collectDependencies( + _('/external/re-exports/index.d.ts'), {dependencies, missing, deepImports}); + expect(dependencies.size).toBe(2); + expect(missing.size).toBe(0); + expect(deepImports.size).toBe(0); + expect(dependencies.has(_('/node_modules/lib-1'))).toBe(true); + expect(dependencies.has(_('/node_modules/lib-1/sub-1'))).toBe(true); + }); + + it('should capture missing external imports', () => { + const {dependencies, missing, deepImports} = createDependencyInfo(); + host.collectDependencies( + _('/external/imports-missing/index.d.ts'), {dependencies, missing, deepImports}); + + expect(dependencies.size).toBe(1); + expect(dependencies.has(_('/node_modules/lib-1'))).toBe(true); + expect(missing.size).toBe(1); + expect(missing.has(relativeFrom('missing'))).toBe(true); + expect(deepImports.size).toBe(0); + }); + + it('should not register deep imports as missing', () => { + // This scenario verifies the behavior of the dependency analysis when an external import + // is found that does not map to an entry-point but still exists on disk, i.e. a deep + // import. Such deep imports are captured for diagnostics purposes. + // Note that in the DTS version, the deep import may not map to a .d.ts file, but instead + // a .js file. This test exercises this particular scenario. + const {dependencies, missing, deepImports} = createDependencyInfo(); + host.collectDependencies( + _('/external/deep-import/index.d.ts'), {dependencies, missing, deepImports}); + + expect(dependencies.size).toBe(0); + expect(missing.size).toBe(0); + expect(deepImports.size).toBe(1); + expect(deepImports.has(_('/node_modules/lib-1/deep/import'))).toBe(true); + }); + + it('should not register deep imports as missing, if available in `@types/...`', () => { + // This scenario verifies the behavior of the dependency analysis when an external import + // is found that does not map to an entry-point but still exists in an `@types/...` package, + // i.e. a type-only deep import. Such deep imports are captured for diagnostics purposes. + const {dependencies, missing, deepImports} = createDependencyInfo(); + host.collectDependencies( + _('/external/deep-import-2/index.d.ts'), {dependencies, missing, deepImports}); + + expect(dependencies.size).toBe(0); + expect(missing.size).toBe(0); + expect(deepImports.size).toBe(1); + expect(deepImports.has(_('/node_modules/@types/type-only/deep/import'))).toBe(true); + }); + + it('should recurse into internal dependencies', () => { + const {dependencies, missing, deepImports} = createDependencyInfo(); + host.collectDependencies( + _('/internal/outer/index.d.ts'), {dependencies, missing, deepImports}); + + expect(dependencies.size).toBe(1); + expect(dependencies.has(_('/node_modules/lib-1/sub-1'))).toBe(true); + expect(missing.size).toBe(0); + expect(deepImports.size).toBe(0); + }); + + it('should handle circular internal dependencies', () => { + const {dependencies, missing, deepImports} = createDependencyInfo(); + host.collectDependencies( + _('/internal/circular-a/index.d.ts'), {dependencies, missing, deepImports}); + expect(dependencies.size).toBe(2); + expect(dependencies.has(_('/node_modules/lib-1'))).toBe(true); + expect(dependencies.has(_('/node_modules/lib-1/sub-1'))).toBe(true); + expect(missing.size).toBe(0); + expect(deepImports.size).toBe(0); + }); + + it('should support `paths` alias mappings when resolving modules', () => { + const fs = getFileSystem(); + host = new DtsDependencyHost(fs, { + baseUrl: '/dist', + paths: { + '@app/*': ['*'], + '@lib/*/test': ['lib/*/test'], + } + }); + const {dependencies, missing, deepImports} = createDependencyInfo(); + host.collectDependencies(_('/path-alias/index.d.ts'), {dependencies, missing, deepImports}); + expect(dependencies.size).toBe(4); + expect(dependencies.has(_('/dist/components'))).toBe(true); + expect(dependencies.has(_('/dist/shared'))).toBe(true); + expect(dependencies.has(_('/dist/lib/shared/test'))).toBe(true); + expect(dependencies.has(_('/node_modules/lib-1'))).toBe(true); + expect(missing.size).toBe(0); + expect(deepImports.size).toBe(0); + }); + + it('should handle entry-point paths with no extension', () => { + const {dependencies, missing, deepImports} = createDependencyInfo(); + host.collectDependencies( + _('/external/imports/index'), {dependencies, missing, deepImports}); + expect(dependencies.size).toBe(2); + expect(missing.size).toBe(0); + expect(deepImports.size).toBe(0); + expect(dependencies.has(_('/node_modules/lib-1'))).toBe(true); + expect(dependencies.has(_('/node_modules/lib-1/sub-1'))).toBe(true); + }); + }); + + function setupMockFileSystem(): void { + loadTestFiles([ + { + name: _('/no/imports/or/re-exports/index.d.ts'), + contents: '// some text but no import-like statements' + }, + {name: _('/no/imports/or/re-exports/package.json'), contents: '{"esm2015": "./index.js"}'}, + {name: _('/no/imports/or/re-exports/index.metadata.json'), contents: 'MOCK METADATA'}, + { + name: _('/external/imports/index.d.ts'), + contents: `import {X} from 'lib-1';\nimport {Y} from 'lib-1/sub-1';` + }, + {name: _('/external/imports/package.json'), contents: '{"esm2015": "./index.js"}'}, + {name: _('/external/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';` + }, + {name: _('/external/re-exports/package.json'), contents: '{"esm2015": "./index.js"}'}, + {name: _('/external/re-exports/index.metadata.json'), contents: 'MOCK METADATA'}, + { + name: _('/external/imports-missing/index.d.ts'), + contents: `import {X} from 'lib-1';\nimport {Y} from 'missing';` + }, + {name: _('/external/imports-missing/package.json'), contents: '{"esm2015": "./index.js"}'}, + {name: _('/external/imports-missing/index.metadata.json'), contents: 'MOCK METADATA'}, + { + name: _('/external/deep-import/index.d.ts'), + contents: `import {Y} from 'lib-1/deep/import';` + }, + {name: _('/external/deep-import/package.json'), contents: '{"esm2015": "./index.js"}'}, + {name: _('/external/deep-import/index.metadata.json'), contents: 'MOCK METADATA'}, + { + name: _('/external/deep-import-2/index.d.ts'), + contents: `import {Y} from 'type-only/deep/import';` + }, + { + name: _('/node_modules/@types/type-only/deep/import/index.d.ts'), + contents: `export declare class Y {}` + }, + {name: _('/internal/outer/index.d.ts'), contents: `import {X} from '../inner';`}, + {name: _('/internal/outer/package.json'), contents: '{"esm2015": "./index.js"}'}, + {name: _('/internal/outer/index.metadata.json'), contents: 'MOCK METADATA'}, + { + name: _('/internal/inner/index.d.ts'), + contents: `import {Y} from 'lib-1/sub-1'; export declare class X {}` + }, + { + name: _('/internal/circular-a/index.d.ts'), + contents: + `import {B} from '../circular-b'; import {X} from '../circular-b'; export {Y} from 'lib-1/sub-1';` + }, + { + name: _('/internal/circular-b/index.d.ts'), + contents: + `import {A} from '../circular-a'; import {Y} from '../circular-a'; export {X} from 'lib-1';` + }, + {name: _('/internal/circular-a/package.json'), contents: '{"esm2015": "./index.js"}'}, + {name: _('/internal/circular-a/index.metadata.json'), contents: 'MOCK METADATA'}, + { + name: _('/path-alias/index.d.ts'), + contents: + `import {TestHelper} from '@app/components';\nimport {Service} from '@app/shared';\nimport {TestHelper} from '@lib/shared/test';\nimport {X} from 'lib-1';` + }, + {name: _('/path-alias/package.json'), contents: '{"esm2015": "./index.js"}'}, + {name: _('/path-alias/index.metadata.json'), contents: 'MOCK METADATA'}, + {name: _('/node_modules/lib-1/index.d.ts'), contents: 'export declare class X {}'}, + {name: _('/node_modules/lib-1/package.json'), contents: '{"esm2015": "./index.js"}'}, + {name: _('/node_modules/lib-1/index.metadata.json'), contents: 'MOCK METADATA'}, + { + name: _('/node_modules/lib-1/deep/import/index.js'), + contents: 'export class DeepImport {}' + }, + {name: _('/node_modules/lib-1/sub-1/index.d.ts'), contents: 'export declare class Y {}'}, + {name: _('/node_modules/lib-1/sub-1/package.json'), contents: '{"esm2015": "./index.js"}'}, + {name: _('/node_modules/lib-1/sub-1/index.metadata.json'), contents: 'MOCK METADATA'}, + {name: _('/node_modules/lib-1/sub-2.d.ts'), contents: `export * from './sub-2/sub-2';`}, + {name: _('/node_modules/lib-1/sub-2/sub-2.d.ts'), contents: `export declare class Z {}';`}, + {name: _('/node_modules/lib-1/sub-2/package.json'), contents: '{"esm2015": "./sub-2.js"}'}, + {name: _('/node_modules/lib-1/sub-2/sub-2.metadata.json'), contents: 'MOCK METADATA'}, + {name: _('/dist/components/index.d.ts'), contents: `class MyComponent {};`}, + {name: _('/dist/components/package.json'), contents: '{"esm2015": "./index.js"}'}, + {name: _('/dist/components/index.metadata.json'), contents: 'MOCK METADATA'}, + { + name: _('/dist/shared/index.d.ts'), + contents: `import {X} from 'lib-1';\nexport declare class Service {}` + }, + {name: _('/dist/shared/package.json'), contents: '{"esm2015": "./index.js"}'}, + {name: _('/dist/shared/index.metadata.json'), contents: 'MOCK METADATA'}, + { + name: _('/dist/lib/shared/test/index.d.ts'), + contents: `export declare class TestHelper {}` + }, + {name: _('/dist/lib/shared/test/package.json'), contents: '{"esm2015": "./index.js"}'}, + {name: _('/dist/lib/shared/test/index.metadata.json'), contents: 'MOCK METADATA'}, + ]); + } + }); +});