From 97ef8ae9e73875ec33c8f5a96b838e89e6515035 Mon Sep 17 00:00:00 2001 From: JoostK Date: Fri, 9 Nov 2018 20:13:51 +0100 Subject: [PATCH] fix(ivy): let ngcc not consider deep imports as missing dependencies (#27031) This fixes an issue where packages would be skipped if they contained e.g. RxJS 5 style imports such as ``` import { observeOn } from 'rxjs/operators/observeOn'; ``` Given that no package.json file can be found at the imported path, the dependency would be reported missing, causing the package to be skipped. PR Close #27031 --- .../src/ngcc/src/packages/dependency_host.ts | 32 +++++--- .../ngcc/src/packages/dependency_resolver.ts | 10 ++- .../test/packages/dependency_host_spec.ts | 79 +++++++++++++------ 3 files changed, 83 insertions(+), 38 deletions(-) diff --git a/packages/compiler-cli/src/ngcc/src/packages/dependency_host.ts b/packages/compiler-cli/src/ngcc/src/packages/dependency_host.ts index f6d7bbd38b..9255fd5de0 100644 --- a/packages/compiler-cli/src/ngcc/src/packages/dependency_host.ts +++ b/packages/compiler-cli/src/ngcc/src/packages/dependency_host.ts @@ -17,15 +17,15 @@ export class DependencyHost { /** * Get a list of the resolved paths to all the dependencies of this entry point. * @param from An absolute path to the file whose dependencies we want to get. - * @param resolved A set that will have the resolved dependencies added to it. + * @param resolved A set that will have the absolute paths of resolved entry points added to it. * @param missing A set that will have the dependencies that could not be found added to it. + * @param deepImports A set that will have the import paths that exist but cannot be mapped to + * entry-points, i.e. deep-imports. * @param internal A set that is used to track internal dependencies to prevent getting stuck in a * circular dependency loop. - * @returns an object containing an array of absolute paths to `resolved` depenendencies and an - * array of import specifiers for dependencies that were `missing`. */ computeDependencies( - from: string, resolved: Set, missing: Set, + from: string, resolved: Set, missing: Set, deepImports: Set, internal: Set = new Set()): void { const fromContents = fs.readFileSync(from, 'utf8'); if (!this.hasImportOrReeportStatements(fromContents)) { @@ -48,14 +48,22 @@ export class DependencyHost { // Avoid circular dependencies if (!internal.has(internalDependency)) { internal.add(internalDependency); - this.computeDependencies(internalDependency, resolved, missing, internal); + this.computeDependencies( + internalDependency, resolved, missing, deepImports, internal); } } else { - const externalDependency = this.tryResolveExternal(from, importPath); - if (externalDependency !== null) { - resolved.add(externalDependency); + const resolvedEntryPoint = this.tryResolveEntryPoint(from, importPath); + if (resolvedEntryPoint !== null) { + resolved.add(resolvedEntryPoint); } else { - missing.add(importPath); + // If the import could not be resolved as entry point, it either does not exist + // at all or is a deep import. + const deeplyImportedFile = this.tryResolve(from, importPath); + if (deeplyImportedFile !== null) { + deepImports.add(importPath); + } else { + missing.add(importPath); + } } } }); @@ -91,9 +99,9 @@ export class DependencyHost { * @returns the resolved path to the entry point directory of the import or null * if it cannot be resolved. */ - tryResolveExternal(from: string, to: string): string|null { - const externalDependency = this.tryResolve(from, `${to}/package.json`); - return externalDependency && path.dirname(externalDependency); + tryResolveEntryPoint(from: string, to: string): string|null { + const entryPoint = this.tryResolve(from, `${to}/package.json`); + return entryPoint && path.dirname(entryPoint); } /** diff --git a/packages/compiler-cli/src/ngcc/src/packages/dependency_resolver.ts b/packages/compiler-cli/src/ngcc/src/packages/dependency_resolver.ts index 49f23f8c80..1448a933d8 100644 --- a/packages/compiler-cli/src/ngcc/src/packages/dependency_resolver.ts +++ b/packages/compiler-cli/src/ngcc/src/packages/dependency_resolver.ts @@ -87,7 +87,8 @@ export class DependencyResolver { const dependencies = new Set(); const missing = new Set(); - this.host.computeDependencies(entryPointPath, dependencies, missing); + const deepImports = new Set(); + this.host.computeDependencies(entryPointPath, dependencies, missing, deepImports); if (missing.size > 0) { // This entry point has dependencies that are missing @@ -109,6 +110,13 @@ export class DependencyResolver { } }); } + + if (deepImports.size) { + const imports = Array.from(deepImports).map(i => `'${i}'`).join(', '); + console.warn( + `Entry point '${entryPoint.name}' contains deep imports into ${imports}. ` + + `This is probably not a problem, but may cause the compilation of entry points to be out of order.`); + } }); // The map now only holds entry-points that ngcc cares about and whose dependencies diff --git a/packages/compiler-cli/src/ngcc/test/packages/dependency_host_spec.ts b/packages/compiler-cli/src/ngcc/test/packages/dependency_host_spec.ts index 4586aacc53..1542cfea9e 100644 --- a/packages/compiler-cli/src/ngcc/test/packages/dependency_host_spec.ts +++ b/packages/compiler-cli/src/ngcc/test/packages/dependency_host_spec.ts @@ -27,61 +27,88 @@ describe('DependencyHost', () => { it('should not generate a TS AST if the source does not contain any imports or re-exports', () => { spyOn(ts, 'createSourceFile'); - host.computeDependencies('/no/imports/or/re-exports.js', new Set(), new Set()); + host.computeDependencies('/no/imports/or/re-exports.js', new Set(), new Set(), new Set()); expect(ts.createSourceFile).not.toHaveBeenCalled(); }); it('should resolve all the external imports of the source file', () => { - spyOn(host, 'tryResolveExternal') + spyOn(host, 'tryResolveEntryPoint') .and.callFake((from: string, importPath: string) => `RESOLVED/${importPath}`); const resolved = new Set(); const missing = new Set(); - host.computeDependencies('/external/imports.js', resolved, missing); + const deepImports = new Set(); + host.computeDependencies('/external/imports.js', resolved, missing, deepImports); expect(resolved.size).toBe(2); - expect(resolved.has('RESOLVED/path/to/x')); - expect(resolved.has('RESOLVED/path/to/y')); + expect(resolved.has('RESOLVED/path/to/x')).toBe(true); + expect(resolved.has('RESOLVED/path/to/y')).toBe(true); }); it('should resolve all the external re-exports of the source file', () => { - spyOn(host, 'tryResolveExternal') + spyOn(host, 'tryResolveEntryPoint') .and.callFake((from: string, importPath: string) => `RESOLVED/${importPath}`); const resolved = new Set(); const missing = new Set(); - host.computeDependencies('/external/re-exports.js', resolved, missing); + const deepImports = new Set(); + host.computeDependencies('/external/re-exports.js', resolved, missing, deepImports); expect(resolved.size).toBe(2); - expect(resolved.has('RESOLVED/path/to/x')); - expect(resolved.has('RESOLVED/path/to/y')); + expect(resolved.has('RESOLVED/path/to/x')).toBe(true); + expect(resolved.has('RESOLVED/path/to/y')).toBe(true); }); it('should capture missing external imports', () => { - spyOn(host, 'tryResolveExternal') + spyOn(host, 'tryResolveEntryPoint') .and.callFake( (from: string, importPath: string) => importPath === 'missing' ? null : `RESOLVED/${importPath}`); + spyOn(host, 'tryResolve').and.callFake(() => null); const resolved = new Set(); const missing = new Set(); - host.computeDependencies('/external/imports-missing.js', resolved, missing); + const deepImports = new Set(); + host.computeDependencies('/external/imports-missing.js', resolved, missing, deepImports); expect(resolved.size).toBe(1); - expect(resolved.has('RESOLVED/path/to/x')); + expect(resolved.has('RESOLVED/path/to/x')).toBe(true); expect(missing.size).toBe(1); - expect(missing.has('missing')); + expect(missing.has('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. + const tryResolveEntryPoint = (from: string, importPath: string) => + importPath === 'deep/import' ? null : `RESOLVED/${importPath}`; + spyOn(host, 'tryResolveEntryPoint').and.callFake(tryResolveEntryPoint); + spyOn(host, 'tryResolve') + .and.callFake((from: string, importPath: string) => `RESOLVED/${importPath}`); + const resolved = new Set(); + const missing = new Set(); + const deepImports = new Set(); + host.computeDependencies('/external/deep-import.js', resolved, missing, deepImports); + expect(resolved.size).toBe(0); + expect(missing.size).toBe(0); + expect(deepImports.size).toBe(1); + expect(deepImports.has('deep/import')).toBe(true); }); it('should recurse into internal dependencies', () => { spyOn(host, 'resolveInternal') .and.callFake( (from: string, importPath: string) => path.join('/internal', importPath + '.js')); - spyOn(host, 'tryResolveExternal') + spyOn(host, 'tryResolveEntryPoint') .and.callFake((from: string, importPath: string) => `RESOLVED/${importPath}`); const getDependenciesSpy = spyOn(host, 'computeDependencies').and.callThrough(); const resolved = new Set(); const missing = new Set(); - host.computeDependencies('/internal/outer.js', resolved, missing); - expect(getDependenciesSpy).toHaveBeenCalledWith('/internal/outer.js', resolved, missing); + const deepImports = new Set(); + host.computeDependencies('/internal/outer.js', resolved, missing, deepImports); expect(getDependenciesSpy) - .toHaveBeenCalledWith('/internal/inner.js', resolved, missing, jasmine.any(Set)); + .toHaveBeenCalledWith('/internal/outer.js', resolved, missing, deepImports); + expect(getDependenciesSpy) + .toHaveBeenCalledWith( + '/internal/inner.js', resolved, missing, deepImports, jasmine.any(Set)); expect(resolved.size).toBe(1); - expect(resolved.has('RESOLVED/path/to/y')); + expect(resolved.has('RESOLVED/path/to/y')).toBe(true); }); @@ -89,14 +116,15 @@ describe('DependencyHost', () => { spyOn(host, 'resolveInternal') .and.callFake( (from: string, importPath: string) => path.join('/internal', importPath + '.js')); - spyOn(host, 'tryResolveExternal') + spyOn(host, 'tryResolveEntryPoint') .and.callFake((from: string, importPath: string) => `RESOLVED/${importPath}`); const resolved = new Set(); const missing = new Set(); - host.computeDependencies('/internal/circular-a.js', resolved, missing); + const deepImports = new Set(); + host.computeDependencies('/internal/circular-a.js', resolved, missing, deepImports); expect(resolved.size).toBe(2); - expect(resolved.has('RESOLVED/path/to/x')); - expect(resolved.has('RESOLVED/path/to/y')); + expect(resolved.has('RESOLVED/path/to/x')).toBe(true); + expect(resolved.has('RESOLVED/path/to/y')).toBe(true); }); function createMockFileSystem() { @@ -105,6 +133,7 @@ describe('DependencyHost', () => { '/external/imports.js': `import {X} from 'path/to/x';\nimport {Y} from 'path/to/y';`, '/external/re-exports.js': `export {X} from 'path/to/x';\nexport {Y} from 'path/to/y';`, '/external/imports-missing.js': `import {X} from 'path/to/x';\nimport {Y} from 'missing';`, + '/external/deep-import.js': `import {Y} from 'deep/import';`, '/internal/outer.js': `import {X} from './inner';`, '/internal/inner.js': `import {Y} from 'path/to/y';`, '/internal/circular-a.js': `import {B} from './circular-b'; import {X} from 'path/to/x';`, @@ -131,18 +160,18 @@ describe('DependencyHost', () => { describe('tryResolveExternal', () => { it('should call `tryResolve`, appending `package.json` to the target path', () => { const tryResolveSpy = spyOn(host, 'tryResolve').and.returnValue('PATH/TO/RESOLVED'); - host.tryResolveExternal('SOURCE_PATH', 'TARGET_PATH'); + host.tryResolveEntryPoint('SOURCE_PATH', 'TARGET_PATH'); expect(tryResolveSpy).toHaveBeenCalledWith('SOURCE_PATH', 'TARGET_PATH/package.json'); }); it('should return the directory containing the result from `tryResolve', () => { spyOn(host, 'tryResolve').and.returnValue('PATH/TO/RESOLVED'); - expect(host.tryResolveExternal('SOURCE_PATH', 'TARGET_PATH')).toEqual('PATH/TO'); + expect(host.tryResolveEntryPoint('SOURCE_PATH', 'TARGET_PATH')).toEqual('PATH/TO'); }); it('should return null if `tryResolve` returns null', () => { spyOn(host, 'tryResolve').and.returnValue(null); - expect(host.tryResolveExternal('SOURCE_PATH', 'TARGET_PATH')).toEqual(null); + expect(host.tryResolveEntryPoint('SOURCE_PATH', 'TARGET_PATH')).toEqual(null); }); });