Skip to content
Permalink
Browse files

fix(ngcc): capture entry-point dependencies from typings as well as s…

…ource (#34494)

ngcc computes a dependency graph of entry-points to ensure that
entry-points are processed in the correct order. Previously only the imports
in source files were analysed to determine the dependencies for each
entry-point.

This is not sufficient when an entry-point has a "type-only" dependency
 - for example only importing an interface from another entry-point.
In this case the "type-only" import does not appear in the
source code. It only appears in the typings files. This can cause a
dependency to be missed on the entry-point.

This commit fixes this by additionally processing the imports in the
typings program, as well as the source program.

Note that these missing dependencies could cause unexpected flakes when
running ngcc in async mode on multiple processes due to the way that
ngcc caches files when they are first read from disk.

Fixes #34411

// FW-1781

PR Close #34494
  • Loading branch information
petebacondarwin authored and alxhub committed Dec 19, 2019
1 parent fa43184 commit c692757029664ec618eb8cfe7c2adbd5b9d52ec8
@@ -82,7 +82,8 @@ export interface SortedEntryPointsInfo extends DependencyDiagnostics {
export class DependencyResolver {
constructor(
private fs: FileSystem, private logger: Logger,
private hosts: Partial<Record<EntryPointFormat, DependencyHost>>) {}
private hosts: Partial<Record<EntryPointFormat, DependencyHost>>,
private typingsHost: DependencyHost) {}
/**
* Sort the array of entry points so that the dependant entry points always come later than
* their dependencies in the array.
@@ -125,6 +126,7 @@ export class DependencyResolver {
}
const depInfo = createDependencyInfo();
host.collectDependencies(formatInfo.path, depInfo);
this.typingsHost.collectDependencies(entryPoint.typings, depInfo);
return depInfo;
}

@@ -35,7 +35,7 @@ import {NgccConfiguration} from './packages/configuration';
import {EntryPoint, EntryPointJsonProperty, EntryPointPackageJson, SUPPORTED_FORMAT_PROPERTIES, getEntryPointFormat} from './packages/entry_point';
import {makeEntryPointBundle} from './packages/entry_point_bundle';
import {Transformer} from './packages/transformer';
import {PathMappings} from './utils';
import {PathMappings, createDtsDependencyHost} from './utils';
import {FileWriter} from './writing/file_writer';
import {InPlaceFileWriter} from './writing/in_place_file_writer';
import {NewEntryPointFileWriter} from './writing/new_entry_point_file_writer';
@@ -164,12 +164,15 @@ export function mainNgcc(
const esmDependencyHost = new EsmDependencyHost(fileSystem, moduleResolver);
const umdDependencyHost = new UmdDependencyHost(fileSystem, moduleResolver);
const commonJsDependencyHost = new CommonJsDependencyHost(fileSystem, moduleResolver);
const dependencyResolver = new DependencyResolver(fileSystem, logger, {
esm5: esmDependencyHost,
esm2015: esmDependencyHost,
umd: umdDependencyHost,
commonjs: commonJsDependencyHost
});
const dtsDependencyHost = createDtsDependencyHost(fileSystem, pathMappings);
const dependencyResolver = new DependencyResolver(
fileSystem, logger, {
esm5: esmDependencyHost,
esm2015: esmDependencyHost,
umd: umdDependencyHost,
commonjs: commonJsDependencyHost
},
dtsDependencyHost);

const absBasePath = absoluteFrom(basePath);
const config = new NgccConfiguration(fileSystem, dirname(absBasePath));
@@ -7,6 +7,8 @@
*/
import * as ts from 'typescript';
import {AbsoluteFsPath, FileSystem, absoluteFrom} from '../../src/ngtsc/file_system';
import {EsmDependencyHost} from './dependencies/esm_dependency_host';
import {ModuleResolver} from './dependencies/module_resolver';

/**
* A list (`Array`) of partially ordered `T` items.
@@ -119,3 +121,8 @@ export function stripDollarSuffix(value: string): string {
export function stripExtension(fileName: string): string {
return fileName.replace(/\..+$/, '');
}

export function createDtsDependencyHost(fileSystem: FileSystem, pathMappings?: PathMappings) {
return new EsmDependencyHost(
fileSystem, new ModuleResolver(fileSystem, pathMappings, ['', '.d.ts', '/index.d.ts']));
}
@@ -15,6 +15,7 @@ import {DependencyResolver, SortedEntryPointsInfo} from '../../src/dependencies/
import {EsmDependencyHost} from '../../src/dependencies/esm_dependency_host';
import {ModuleResolver} from '../../src/dependencies/module_resolver';
import {EntryPoint} from '../../src/packages/entry_point';
import {createDtsDependencyHost} from '../../src/utils';
import {MockLogger} from '../helpers/mock_logger';


@@ -26,6 +27,7 @@ runInEachFileSystem(() => {
describe('DependencyResolver', () => {
let _: typeof absoluteFrom;
let host: EsmDependencyHost;
let dtsHost: EsmDependencyHost;
let resolver: DependencyResolver;
let fs: FileSystem;
let moduleResolver: ModuleResolver;
@@ -35,7 +37,8 @@ runInEachFileSystem(() => {
fs = getFileSystem();
moduleResolver = new ModuleResolver(fs);
host = new EsmDependencyHost(fs, moduleResolver);
resolver = new DependencyResolver(fs, new MockLogger(), {esm5: host, esm2015: host});
dtsHost = createDtsDependencyHost(fs);
resolver = new DependencyResolver(fs, new MockLogger(), {esm5: host, esm2015: host}, dtsHost);
});

describe('sortEntryPointsByDependency()', () => {
@@ -46,44 +49,51 @@ runInEachFileSystem(() => {
let fifth: EntryPoint;
let sixthIgnoreMissing: EntryPoint;
let dependencies: DepMap;
let dtsDependencies: DepMap;

beforeEach(() => {
first = {
path: _('/first'),
packageJson: {esm5: './index.js'},
compiledByAngular: true,
ignoreMissingDependencies: false,
typings: _('/first/index.d.ts'),
} as EntryPoint;
second = {
path: _('/second'),
packageJson: {esm2015: './sub/index.js'},
compiledByAngular: true,
ignoreMissingDependencies: false,
typings: _('/second/sub/index.d.ts'),
} as EntryPoint;
third = {
path: _('/third'),
packageJson: {fesm5: './index.js'},
compiledByAngular: true,
ignoreMissingDependencies: false,
typings: _('/third/index.d.ts'),
} as EntryPoint;
fourth = {
path: _('/fourth'),
packageJson: {fesm2015: './sub2/index.js'},
compiledByAngular: true,
ignoreMissingDependencies: false,
typings: _('/fourth/sub2/index.d.ts'),
} as EntryPoint;
fifth = {
path: _('/fifth'),
packageJson: {module: './index.js'},
compiledByAngular: true,
ignoreMissingDependencies: false,
typings: _('/fifth/index.d.ts'),
} as EntryPoint;

sixthIgnoreMissing = {
path: _('/sixth'),
packageJson: {module: './index.js'},
compiledByAngular: true,
ignoreMissingDependencies: true,
typings: _('/sixth/index.d.ts'),
} as EntryPoint;

dependencies = {
@@ -94,11 +104,21 @@ runInEachFileSystem(() => {
[_('/fourth/sub2/index.js')]: {resolved: [fifth.path], missing: []},
[_('/fifth/index.js')]: {resolved: [], missing: []},
};
dtsDependencies = {
[_('/first/index.d.ts')]:
{resolved: [second.path, third.path, _('/ignored-1')], missing: []},
[_('/second/sub/index.d.ts')]: {resolved: [third.path, fifth.path], missing: []},
[_('/third/index.d.ts')]: {resolved: [fourth.path, _('/ignored-2')], missing: []},
[_('/fourth/sub2/index.d.ts')]: {resolved: [fifth.path], missing: []},
[_('/fifth/index.d.ts')]: {resolved: [], missing: []},
};
});

it('should order the entry points by their dependency on each other', () => {
spyOn(host, 'collectDependencies')
.and.callFake(createFakeComputeDependencies(dependencies));
spyOn(dtsHost, 'collectDependencies')
.and.callFake(createFakeComputeDependencies(dtsDependencies));
const result = resolver.sortEntryPointsByDependency([fifth, first, fourth, second, third]);
expect(result.entryPoints).toEqual([fifth, fourth, third, second, first]);
});
@@ -108,6 +128,10 @@ runInEachFileSystem(() => {
[_('/first/index.js')]: {resolved: [], missing: [_('/missing')]},
[_('/second/sub/index.js')]: {resolved: [], missing: []},
}));
spyOn(dtsHost, 'collectDependencies').and.callFake(createFakeComputeDependencies({
[_('/first/index.d.ts')]: {resolved: [], missing: [_('/missing')]},
[_('/second/sub/index.d.ts')]: {resolved: [], missing: []},
}));
const result = resolver.sortEntryPointsByDependency([first, second]);
expect(result.entryPoints).toEqual([second]);
expect(result.invalidEntryPoints).toEqual([
@@ -121,6 +145,11 @@ runInEachFileSystem(() => {
[_('/second/sub/index.js')]: {resolved: [], missing: [_('/missing')]},
[_('/third/index.js')]: {resolved: [], missing: []},
}));
spyOn(dtsHost, 'collectDependencies').and.callFake(createFakeComputeDependencies({
[_('/first/index.d.ts')]: {resolved: [second.path, third.path], missing: []},
[_('/second/sub/index.d.ts')]: {resolved: [], missing: [_('/missing')]},
[_('/third/index.d.ts')]: {resolved: [], missing: []},
}));
// Note that we will process `first` before `second`, which has the missing dependency.
const result = resolver.sortEntryPointsByDependency([first, second, third]);
expect(result.entryPoints).toEqual([third]);
@@ -136,6 +165,11 @@ runInEachFileSystem(() => {
[_('/second/sub/index.js')]: {resolved: [], missing: [_('/missing')]},
[_('/third/index.js')]: {resolved: [], missing: []},
}));
spyOn(dtsHost, 'collectDependencies').and.callFake(createFakeComputeDependencies({
[_('/first/index.d.ts')]: {resolved: [second.path, third.path], missing: []},
[_('/second/sub/index.d.ts')]: {resolved: [], missing: [_('/missing')]},
[_('/third/index.d.ts')]: {resolved: [], missing: []},
}));
// Note that we will process `first` after `second`, which has the missing dependency.
const result = resolver.sortEntryPointsByDependency([second, first, third]);
expect(result.entryPoints).toEqual([third]);
@@ -151,6 +185,10 @@ runInEachFileSystem(() => {
[_('/first/index.js')]: {resolved: [sixthIgnoreMissing.path], missing: []},
[_('/sixth/index.js')]: {resolved: [], missing: [_('/missing')]},
}));
spyOn(dtsHost, 'collectDependencies').and.callFake(createFakeComputeDependencies({
[_('/first/index.d.ts')]: {resolved: [sixthIgnoreMissing.path], missing: []},
[_('/sixth/index.d.ts')]: {resolved: [], missing: [_('/missing')]},
}));
// Note that we will process `first` after `second`, which has the missing dependency.
const result = resolver.sortEntryPointsByDependency([sixthIgnoreMissing, first]);
expect(result.entryPoints).toEqual([sixthIgnoreMissing, first]);
@@ -163,6 +201,11 @@ runInEachFileSystem(() => {
[_('/second/sub/index.js')]: {resolved: [first.path], missing: []},
[_('/sixth/index.js')]: {resolved: [second.path], missing: []},
}));
spyOn(dtsHost, 'collectDependencies').and.callFake(createFakeComputeDependencies({
[_('/first/index.d.ts')]: {resolved: [], missing: [_('/missing')]},
[_('/second/sub/index.d.ts')]: {resolved: [first.path], missing: []},
[_('/sixth/index.d.ts')]: {resolved: [second.path], missing: []},
}));
const result = resolver.sortEntryPointsByDependency([first, second, sixthIgnoreMissing]);
// sixth has no missing dependencies, but it has _invalid_ dependencies, so it's not
// compiled.
@@ -175,6 +218,11 @@ runInEachFileSystem(() => {
[_('/second/sub/index.js')]: {resolved: [], missing: [_('/missing2')]},
[_('/third/index.js')]: {resolved: [first.path, second.path], missing: []},
}));
spyOn(dtsHost, 'collectDependencies').and.callFake(createFakeComputeDependencies({
[_('/first/index.d.ts')]: {resolved: [], missing: [_('/missing1')]},
[_('/second/sub/index.d.ts')]: {resolved: [], missing: [_('/missing2')]},
[_('/third/index.d.ts')]: {resolved: [first.path, second.path], missing: []},
}));
const result = resolver.sortEntryPointsByDependency([first, second, third]);
expect(result.entryPoints).toEqual([]);
expect(result.invalidEntryPoints).toEqual([
@@ -191,7 +239,7 @@ runInEachFileSystem(() => {
});

it('should error if there is no appropriate DependencyHost for the given formats', () => {
resolver = new DependencyResolver(fs, new MockLogger(), {esm2015: host});
resolver = new DependencyResolver(fs, new MockLogger(), {esm2015: host}, host);
expect(() => resolver.sortEntryPointsByDependency([first]))
.toThrowError(
`Could not find a suitable format for computing dependencies of entry-point: '${first.path}'.`);
@@ -200,6 +248,8 @@ runInEachFileSystem(() => {
it('should capture any dependencies that were ignored', () => {
spyOn(host, 'collectDependencies')
.and.callFake(createFakeComputeDependencies(dependencies));
spyOn(dtsHost, 'collectDependencies')
.and.callFake(createFakeComputeDependencies(dtsDependencies));
const result = resolver.sortEntryPointsByDependency([fifth, first, fourth, second, third]);
expect(result.ignoredDependencies).toEqual([
{entryPoint: first, dependencyPath: _('/ignored-1')},
@@ -210,6 +260,8 @@ runInEachFileSystem(() => {
it('should return the computed dependency graph', () => {
spyOn(host, 'collectDependencies')
.and.callFake(createFakeComputeDependencies(dependencies));
spyOn(dtsHost, 'collectDependencies')
.and.callFake(createFakeComputeDependencies(dtsDependencies));
const result = resolver.sortEntryPointsByDependency([fifth, first, fourth, second, third]);

expect(result.graph).toEqual(jasmine.any(DepGraph));
@@ -220,6 +272,8 @@ runInEachFileSystem(() => {
it('should only return dependencies of the target, if provided', () => {
spyOn(host, 'collectDependencies')
.and.callFake(createFakeComputeDependencies(dependencies));
spyOn(dtsHost, 'collectDependencies')
.and.callFake(createFakeComputeDependencies(dtsDependencies));
const entryPoints = [fifth, first, fourth, second, third];
let sorted: SortedEntryPointsInfo;

@@ -239,6 +293,9 @@ runInEachFileSystem(() => {
spyOn(host, 'collectDependencies').and.callFake(createFakeComputeDependencies({
[_('/first/index.js')]: {resolved: [], missing: [_('/missing')]},
}));
spyOn(dtsHost, 'collectDependencies').and.callFake(createFakeComputeDependencies({
[_('/first/index.d.ts')]: {resolved: [], missing: [_('/missing')]},
}));
const entryPoints = [first];
let sorted: SortedEntryPointsInfo;

@@ -252,6 +309,9 @@ runInEachFileSystem(() => {
spyOn(host, 'collectDependencies').and.callFake(createFakeComputeDependencies({
[_('/first/index.js')]: {resolved: [], missing: ['fs']},
}));
spyOn(dtsHost, 'collectDependencies').and.callFake(createFakeComputeDependencies({
[_('/first/index.d.ts')]: {resolved: [], missing: ['fs']},
}));
const entryPoints = [first];
let sorted: SortedEntryPointsInfo;

@@ -264,12 +324,15 @@ runInEachFileSystem(() => {
it('should use the appropriate DependencyHost for each entry-point', () => {
const esm5Host = new EsmDependencyHost(fs, moduleResolver);
const esm2015Host = new EsmDependencyHost(fs, moduleResolver);
resolver =
new DependencyResolver(fs, new MockLogger(), {esm5: esm5Host, esm2015: esm2015Host});
const dtsHost = createDtsDependencyHost(fs);
resolver = new DependencyResolver(
fs, new MockLogger(), {esm5: esm5Host, esm2015: esm2015Host}, dtsHost);
spyOn(esm5Host, 'collectDependencies')
.and.callFake(createFakeComputeDependencies(dependencies));
spyOn(esm2015Host, 'collectDependencies')
.and.callFake(createFakeComputeDependencies(dependencies));
spyOn(dtsHost, 'collectDependencies')
.and.callFake(createFakeComputeDependencies(dtsDependencies));
const result = resolver.sortEntryPointsByDependency([fifth, first, fourth, second, third]);
expect(result.entryPoints).toEqual([fifth, fourth, third, second, first]);

@@ -295,6 +358,37 @@ runInEachFileSystem(() => {
.toHaveBeenCalledWith(fs.resolve(fourth.path, 'sub2/index.js'), jasmine.any(Object));
expect(esm2015Host.collectDependencies)
.not.toHaveBeenCalledWith(fs.resolve(fifth.path, 'index.js'), jasmine.any(Object));

expect(dtsHost.collectDependencies)
.toHaveBeenCalledWith(fs.resolve(first.path, 'index.d.ts'), jasmine.any(Object));
expect(dtsHost.collectDependencies)
.toHaveBeenCalledWith(fs.resolve(second.path, 'sub/index.d.ts'), jasmine.any(Object));
expect(dtsHost.collectDependencies)
.toHaveBeenCalledWith(fs.resolve(third.path, 'index.d.ts'), jasmine.any(Object));
expect(dtsHost.collectDependencies)
.toHaveBeenCalledWith(fs.resolve(fourth.path, 'sub2/index.d.ts'), jasmine.any(Object));
expect(dtsHost.collectDependencies)
.toHaveBeenCalledWith(fs.resolve(fifth.path, 'index.d.ts'), jasmine.any(Object));
});

it('should merge "typings-only" dependencies with source dependencies', () => {
spyOn(host, 'collectDependencies').and.callFake(createFakeComputeDependencies({
[_('/first/index.js')]: {resolved: [], missing: []},
[_('/second/sub/index.js')]: {resolved: [], missing: [_('/missing1')]},
[_('/third/index.js')]: {resolved: [first.path], missing: []},
}));
spyOn(dtsHost, 'collectDependencies').and.callFake(createFakeComputeDependencies({
[_('/first/index.d.ts')]: {resolved: [], missing: []},
[_('/second/sub/index.d.ts')]: {resolved: [], missing: [_('/missing2')]},
[_('/third/index.d.ts')]: {resolved: [second.path], missing: []},
}));
const entryPoints = [first, second, third];
const sorted = resolver.sortEntryPointsByDependency(entryPoints);
expect(sorted.entryPoints).toEqual([first]);
expect(sorted.invalidEntryPoints).toEqual([
{entryPoint: second, missingDependencies: [_('/missing1'), _('/missing2')]},
{entryPoint: third, missingDependencies: [_('/second')]},
]);
});

function createFakeComputeDependencies(deps: DepMap) {
@@ -13,6 +13,7 @@ import {loadTestFiles} from '../../../test/helpers';
import {createDependencyInfo} from '../../src/dependencies/dependency_host';
import {EsmDependencyHost, hasImportOrReexportStatements, isStringImportOrReexport} from '../../src/dependencies/esm_dependency_host';
import {ModuleResolver} from '../../src/dependencies/module_resolver';
import {createDtsDependencyHost} from '../../src/utils';

runInEachFileSystem(() => {

@@ -159,8 +160,7 @@ runInEachFileSystem(() => {
expect(jsDeps.missing.has(relativeFrom('./internal-typings'))).toBeTruthy();

// Typings mode will pick up `internal-typings.d.ts` dependency
const dtsHost = new EsmDependencyHost(
fs, new ModuleResolver(fs, undefined, ['', '.d.ts', 'index.d.ts']));
const dtsHost = createDtsDependencyHost(fs);
const dtsDeps = createDependencyInfo();
dtsHost.collectDependencies(_('/external/index.d.ts'), dtsDeps);
expect(dtsDeps.dependencies.size).toEqual(2);

0 comments on commit c692757

Please sign in to comment.
You can’t perform that action at this time.