honeymoose/angular-cn
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(ivy): let ngcc not consider deep imports as missing dependencies (#27031)

Browse Source 
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
This commit is contained in:
JoostK 2018-11-09 20:13:51 +01:00 committed by Andrew Kushnir
parent 99c5db1fb1
commit 97ef8ae9e7
3 changed files with 83 additions and 38 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

30
packages/compiler-cli/src/ngcc/src/packages/dependency_host.ts
View File

@ -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<string>, missing: Set<string>,
from: string, resolved: Set<string>, missing: Set<string>, deepImports: Set<string>,
internal: Set<string> = new Set()): void {
const fromContents = fs.readFileSync(from, 'utf8');
if (!this.hasImportOrReeportStatements(fromContents)) {
@ -48,16 +48,24 @@ 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 {
// 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);
}
/**

10
packages/compiler-cli/src/ngcc/src/packages/dependency_resolver.ts
View File

@ -87,7 +87,8 @@ export class DependencyResolver {
const dependencies = new Set<string>();
const missing = new Set<string>();
this.host.computeDependencies(entryPointPath, dependencies, missing);
const deepImports = new Set<string>();
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

79
packages/compiler-cli/src/ngcc/test/packages/dependency_host_spec.ts
View File

@ -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);
});
});