fix(ngcc): allow deep-import warnings to be ignored (#35683)

This commit adds a new ngcc configuration, `ignorableDeepImportMatchers`
for packages. This is a list of regular expressions matching deep imports
that can be safely ignored from that package. Deep imports that are not
ignored cause a warning to be logged.

// FW-1892

Fixes #35615

PR Close #35683
This commit is contained in:
Pete Bacon Darwin 2020-02-25 15:51:54 +00:00 committed by atscott
parent 0b97d07ad6
commit 20b0c80b0b
6 changed files with 119 additions and 24 deletions

View File

@ -9,6 +9,7 @@
import {DepGraph} from 'dependency-graph';
import {AbsoluteFsPath, FileSystem, resolve} from '../../../src/ngtsc/file_system';
import {Logger} from '../logging/logger';
import {NgccConfiguration} from '../packages/configuration';
import {EntryPoint, EntryPointFormat, SUPPORTED_FORMAT_PROPERTIES, getEntryPointFormat} from '../packages/entry_point';
import {PartiallyOrderedList} from '../utils';
import {DependencyHost, DependencyInfo, createDependencyInfo} from './dependency_host';
@ -81,7 +82,7 @@ export interface SortedEntryPointsInfo extends DependencyDiagnostics {
*/
export class DependencyResolver {
constructor(
private fs: FileSystem, private logger: Logger,
private fs: FileSystem, private logger: Logger, private config: NgccConfiguration,
private hosts: Partial<Record<EntryPointFormat, DependencyHost>>,
private typingsHost: DependencyHost) {}
/**
@ -176,11 +177,14 @@ export class DependencyResolver {
});
}
if (deepImports.size) {
const imports = Array.from(deepImports).map(i => `'${i}'`).join(', ');
this.logger.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.`);
if (deepImports.size > 0) {
const notableDeepImports = this.filterIgnorableDeepImports(entryPoint, deepImports);
if (notableDeepImports.length > 0) {
const imports = notableDeepImports.map(i => `'${i}'`).join(', ');
this.logger.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.`);
}
}
});
@ -210,6 +214,18 @@ export class DependencyResolver {
throw new Error(
`There is no appropriate source code format in '${entryPoint.path}' entry-point.`);
}
/**
* Filter out the deepImports that can be ignored, according to this entryPoint's config.
*/
private filterIgnorableDeepImports(entryPoint: EntryPoint, deepImports: Set<AbsoluteFsPath>):
AbsoluteFsPath[] {
const version = (entryPoint.packageJson.version || null) as string | null;
const packageConfig = this.config.getConfig(entryPoint.package, version);
const matchers = packageConfig.ignorableDeepImportMatchers || [];
return Array.from(deepImports)
.filter(deepImport => !matchers.some(matcher => matcher.test(deepImport)));
}
}
interface DependencyGraph extends DependencyDiagnostics {

View File

@ -150,7 +150,7 @@ export function mainNgcc(
const fileSystem = getFileSystem();
const absBasePath = absoluteFrom(basePath);
const config = new NgccConfiguration(fileSystem, dirname(absBasePath));
const dependencyResolver = getDependencyResolver(fileSystem, logger, pathMappings);
const dependencyResolver = getDependencyResolver(fileSystem, logger, config, pathMappings);
// Bail out early if the work is already done.
const supportedPropertiesToConsider = ensureSupportedProperties(propertiesToConsider);
@ -355,7 +355,7 @@ function getExecutor(
}
function getDependencyResolver(
fileSystem: FileSystem, logger: Logger,
fileSystem: FileSystem, logger: Logger, config: NgccConfiguration,
pathMappings: PathMappings | undefined): DependencyResolver {
const moduleResolver = new ModuleResolver(fileSystem, pathMappings);
const esmDependencyHost = new EsmDependencyHost(fileSystem, moduleResolver);
@ -363,7 +363,7 @@ function getDependencyResolver(
const commonJsDependencyHost = new CommonJsDependencyHost(fileSystem, moduleResolver);
const dtsDependencyHost = new DtsDependencyHost(fileSystem, pathMappings);
return new DependencyResolver(
fileSystem, logger, {
fileSystem, logger, config, {
esm5: esmDependencyHost,
esm2015: esmDependencyHost,
umd: umdDependencyHost,

View File

@ -13,7 +13,12 @@ import {PackageJsonFormatPropertiesMap} from './entry_point';
/**
* The format of a project level configuration file.
*/
export interface NgccProjectConfig<T = NgccPackageConfig> { packages: {[packagePath: string]: T}; }
export interface NgccProjectConfig<T = NgccPackageConfig> {
/**
* The packages that are configured by this project config.
*/
packages: {[packagePath: string]: T};
}
/**
* The format of a package level configuration file.
@ -27,6 +32,11 @@ export interface NgccPackageConfig {
* will be absolute.
*/
entryPoints: {[entryPointPath: string]: NgccEntryPointConfig;};
/**
* A collection of regexes that match deep imports to ignore, for this package, rather than
* displaying a warning.
*/
ignorableDeepImportMatchers?: RegExp[];
}
/**
@ -201,7 +211,8 @@ export class NgccConfiguration {
const absPackagePath = resolve(baseDir, 'node_modules', packagePath);
const entryPoints = this.processEntryPoints(absPackagePath, packageConfig);
processedConfig.packages[absPackagePath] = processedConfig.packages[absPackagePath] || [];
processedConfig.packages[absPackagePath].push({versionRange, entryPoints});
processedConfig.packages[absPackagePath].push(
{...packageConfig, versionRange, entryPoints});
}
}
return processedConfig;

View File

@ -8,19 +8,20 @@
import {DepGraph} from 'dependency-graph';
import {FileSystem, absoluteFrom, getFileSystem, relativeFrom} from '../../../src/ngtsc/file_system';
import {AbsoluteFsPath, FileSystem, absoluteFrom, getFileSystem, relativeFrom} from '../../../src/ngtsc/file_system';
import {runInEachFileSystem} from '../../../src/ngtsc/file_system/testing';
import {DependencyInfo} from '../../src/dependencies/dependency_host';
import {DependencyResolver, SortedEntryPointsInfo} from '../../src/dependencies/dependency_resolver';
import {DtsDependencyHost} from '../../src/dependencies/dts_dependency_host';
import {EsmDependencyHost} from '../../src/dependencies/esm_dependency_host';
import {ModuleResolver} from '../../src/dependencies/module_resolver';
import {NgccConfiguration} from '../../src/packages/configuration';
import {EntryPoint} from '../../src/packages/entry_point';
import {MockLogger} from '../helpers/mock_logger';
interface DepMap {
[path: string]: {resolved: string[], missing: string[]};
[path: string]: {resolved: string[], missing: string[], deepImports?: AbsoluteFsPath[]};
}
runInEachFileSystem(() => {
@ -30,15 +31,19 @@ runInEachFileSystem(() => {
let dtsHost: EsmDependencyHost;
let resolver: DependencyResolver;
let fs: FileSystem;
let config: NgccConfiguration;
let logger: MockLogger;
let moduleResolver: ModuleResolver;
beforeEach(() => {
_ = absoluteFrom;
fs = getFileSystem();
config = new NgccConfiguration(fs, _('/'));
logger = new MockLogger();
moduleResolver = new ModuleResolver(fs);
host = new EsmDependencyHost(fs, moduleResolver);
dtsHost = new DtsDependencyHost(fs);
resolver = new DependencyResolver(fs, new MockLogger(), {esm5: host, esm2015: host}, dtsHost);
resolver = new DependencyResolver(fs, logger, config, {esm5: host, esm2015: host}, dtsHost);
});
describe('sortEntryPointsByDependency()', () => {
@ -53,7 +58,9 @@ runInEachFileSystem(() => {
beforeEach(() => {
first = {
name: 'first',
path: _('/first'),
package: _('/first'),
packageJson: {esm5: './index.js'},
compiledByAngular: true,
ignoreMissingDependencies: false,
@ -61,6 +68,7 @@ runInEachFileSystem(() => {
} as EntryPoint;
second = {
path: _('/second'),
package: _('/second'),
packageJson: {esm2015: './sub/index.js'},
compiledByAngular: true,
ignoreMissingDependencies: false,
@ -68,6 +76,7 @@ runInEachFileSystem(() => {
} as EntryPoint;
third = {
path: _('/third'),
package: _('/third'),
packageJson: {fesm5: './index.js'},
compiledByAngular: true,
ignoreMissingDependencies: false,
@ -75,6 +84,7 @@ runInEachFileSystem(() => {
} as EntryPoint;
fourth = {
path: _('/fourth'),
package: _('/fourth'),
packageJson: {fesm2015: './sub2/index.js'},
compiledByAngular: true,
ignoreMissingDependencies: false,
@ -82,6 +92,7 @@ runInEachFileSystem(() => {
} as EntryPoint;
fifth = {
path: _('/fifth'),
package: _('/fifth'),
packageJson: {module: './index.js'},
compiledByAngular: true,
ignoreMissingDependencies: false,
@ -90,6 +101,7 @@ runInEachFileSystem(() => {
sixthIgnoreMissing = {
path: _('/sixth'),
package: _('/sixth'),
packageJson: {module: './index.js'},
compiledByAngular: true,
ignoreMissingDependencies: true,
@ -232,6 +244,59 @@ runInEachFileSystem(() => {
]);
});
it('should log a warning for deep imports', () => {
spyOn(host, 'collectDependencies').and.callFake(createFakeComputeDependencies({
[_('/first/index.js')]: {resolved: [], missing: [], deepImports: [_('/deep/one')]},
}));
spyOn(dtsHost, 'collectDependencies').and.callFake(createFakeComputeDependencies({
[_('/first/index.d.ts')]: {resolved: [], missing: []},
}));
const result = resolver.sortEntryPointsByDependency([first]);
expect(result.entryPoints).toEqual([first]);
expect(logger.logs.warn).toEqual([[
`Entry point 'first' contains deep imports into '${_('/deep/one')}'. This is probably not a problem, but may cause the compilation of entry points to be out of order.`
]]);
});
it('should not log a warning for ignored deep imports', () => {
spyOn(host, 'collectDependencies').and.callFake(createFakeComputeDependencies({
[_('/project/node_modules/test-package/index.js')]: {
resolved: [],
missing: [],
deepImports: [
_('/project/node_modules/deep/one'), _('/project/node_modules/deep/two'),
_('/project/node_modules/deeper/one'), _('/project/node_modules/deeper/two')
]
},
}));
spyOn(dtsHost, 'collectDependencies').and.callFake(createFakeComputeDependencies({
[_('/project/node_modules/test-package/index.d.ts')]: {resolved: [], missing: []},
}));
// Setup the configuration to ignore deep imports that contain either "deep/" or "two".
fs.ensureDir(_('/project'));
fs.writeFile(
_('/project/ngcc.config.js'),
`module.exports = { packages: { 'test-package': { ignorableDeepImportMatchers: [/deep\\//, /two/] } } };`);
config = new NgccConfiguration(fs, _('/project'));
resolver = new DependencyResolver(fs, logger, config, {esm5: host, esm2015: host}, dtsHost);
const testEntryPoint = {
name: 'test-package',
path: _('/project/node_modules/test-package'),
package: _('/project/node_modules/test-package'),
packageJson: {esm5: './index.js'},
compiledByAngular: true,
ignoreMissingDependencies: false,
typings: _('/project/node_modules/test-package/index.d.ts'),
} as EntryPoint;
const result = resolver.sortEntryPointsByDependency([testEntryPoint]);
expect(result.entryPoints).toEqual([testEntryPoint]);
expect(logger.logs.warn).toEqual([[
`Entry point 'test-package' contains deep imports into '${_('/project/node_modules/deeper/one')}'. This is probably not a problem, but may cause the compilation of entry points to be out of order.`
]]);
});
it('should error if the entry point does not have a suitable format', () => {
expect(() => resolver.sortEntryPointsByDependency([
{ path: '/first', packageJson: {}, compiledByAngular: true } as EntryPoint
@ -239,7 +304,7 @@ runInEachFileSystem(() => {
});
it('should error if there is no appropriate DependencyHost for the given formats', () => {
resolver = new DependencyResolver(fs, new MockLogger(), {esm2015: host}, host);
resolver = new DependencyResolver(fs, new MockLogger(), config, {esm2015: host}, host);
expect(() => resolver.sortEntryPointsByDependency([first]))
.toThrowError(
`Could not find a suitable format for computing dependencies of entry-point: '${first.path}'.`);
@ -326,7 +391,7 @@ runInEachFileSystem(() => {
const esm2015Host = new EsmDependencyHost(fs, moduleResolver);
const dtsHost = new DtsDependencyHost(fs);
resolver = new DependencyResolver(
fs, new MockLogger(), {esm5: esm5Host, esm2015: esm2015Host}, dtsHost);
fs, new MockLogger(), config, {esm5: esm5Host, esm2015: esm2015Host}, dtsHost);
spyOn(esm5Host, 'collectDependencies')
.and.callFake(createFakeComputeDependencies(dependencies));
spyOn(esm2015Host, 'collectDependencies')
@ -396,6 +461,9 @@ runInEachFileSystem(() => {
deps[entryPointPath].resolved.forEach(dep => dependencies.add(absoluteFrom(dep)));
deps[entryPointPath].missing.forEach(
dep => missing.add(fs.isRooted(dep) ? absoluteFrom(dep) : relativeFrom(dep)));
if (deps[entryPointPath].deepImports) {
deps[entryPointPath].deepImports !.forEach(dep => deepImports.add(dep));
}
return {dependencies, missing, deepImports};
};
}

View File

@ -32,8 +32,8 @@ runInEachFileSystem(() => {
logger = new MockLogger();
const srcHost = new EsmDependencyHost(fs, new ModuleResolver(fs));
const dtsHost = new DtsDependencyHost(fs);
resolver = new DependencyResolver(fs, logger, {esm2015: srcHost}, dtsHost);
config = new NgccConfiguration(fs, _Abs('/'));
resolver = new DependencyResolver(fs, logger, config, {esm2015: srcHost}, dtsHost);
});
describe('findEntryPoints()', () => {
@ -156,7 +156,7 @@ runInEachFileSystem(() => {
]);
const srcHost = new EsmDependencyHost(fs, new ModuleResolver(fs, pathMappings));
const dtsHost = new DtsDependencyHost(fs, pathMappings);
resolver = new DependencyResolver(fs, logger, {esm2015: srcHost}, dtsHost);
resolver = new DependencyResolver(fs, logger, config, {esm2015: srcHost}, dtsHost);
const finder = new DirectoryWalkerEntryPointFinder(
fs, config, logger, resolver, basePath, pathMappings);
const {entryPoints} = finder.findEntryPoints();
@ -184,7 +184,7 @@ runInEachFileSystem(() => {
]);
const srcHost = new EsmDependencyHost(fs, new ModuleResolver(fs, pathMappings));
const dtsHost = new DtsDependencyHost(fs, pathMappings);
resolver = new DependencyResolver(fs, logger, {esm2015: srcHost}, dtsHost);
resolver = new DependencyResolver(fs, logger, config, {esm2015: srcHost}, dtsHost);
const finder = new DirectoryWalkerEntryPointFinder(
fs, config, logger, resolver, basePath, pathMappings);
const {entryPoints} = finder.findEntryPoints();

View File

@ -33,8 +33,8 @@ runInEachFileSystem(() => {
logger = new MockLogger();
const srcHost = new EsmDependencyHost(fs, new ModuleResolver(fs));
const dtsHost = new DtsDependencyHost(fs);
resolver = new DependencyResolver(fs, logger, {esm2015: srcHost}, dtsHost);
config = new NgccConfiguration(fs, _Abs('/'));
resolver = new DependencyResolver(fs, logger, config, {esm2015: srcHost}, dtsHost);
});
describe('findEntryPoints()', () => {
@ -189,7 +189,7 @@ runInEachFileSystem(() => {
]);
const srcHost = new EsmDependencyHost(fs, new ModuleResolver(fs, pathMappings));
const dtsHost = new DtsDependencyHost(fs, pathMappings);
resolver = new DependencyResolver(fs, logger, {esm2015: srcHost}, dtsHost);
resolver = new DependencyResolver(fs, logger, config, {esm2015: srcHost}, dtsHost);
const finder = new TargetedEntryPointFinder(
fs, config, logger, resolver, basePath, targetPath, pathMappings);
const {entryPoints} = finder.findEntryPoints();
@ -217,7 +217,7 @@ runInEachFileSystem(() => {
]);
const srcHost = new EsmDependencyHost(fs, new ModuleResolver(fs, pathMappings));
const dtsHost = new DtsDependencyHost(fs, pathMappings);
resolver = new DependencyResolver(fs, logger, {esm2015: srcHost}, dtsHost);
resolver = new DependencyResolver(fs, logger, config, {esm2015: srcHost}, dtsHost);
const finder = new TargetedEntryPointFinder(
fs, config, logger, resolver, basePath, targetPath, pathMappings);
const {entryPoints} = finder.findEntryPoints();
@ -250,7 +250,7 @@ runInEachFileSystem(() => {
]);
const srcHost = new EsmDependencyHost(fs, new ModuleResolver(fs, pathMappings));
const dtsHost = new DtsDependencyHost(fs, pathMappings);
resolver = new DependencyResolver(fs, logger, {esm2015: srcHost}, dtsHost);
resolver = new DependencyResolver(fs, logger, config, {esm2015: srcHost}, dtsHost);
const finder = new TargetedEntryPointFinder(
fs, config, logger, resolver, basePath, targetPath, pathMappings);
const {entryPoints} = finder.findEntryPoints();
@ -277,7 +277,7 @@ runInEachFileSystem(() => {
]);
const srcHost = new EsmDependencyHost(fs, new ModuleResolver(fs, pathMappings));
const dtsHost = new DtsDependencyHost(fs, pathMappings);
resolver = new DependencyResolver(fs, logger, {esm2015: srcHost}, dtsHost);
resolver = new DependencyResolver(fs, logger, config, {esm2015: srcHost}, dtsHost);
const finder = new TargetedEntryPointFinder(
fs, config, logger, resolver, basePath, targetPath, pathMappings);
const {entryPoints} = finder.findEntryPoints();