From 20b0c80b0b37b005cd1964b4f2a5c832c77e1fa9 Mon Sep 17 00:00:00 2001 From: Pete Bacon Darwin Date: Tue, 25 Feb 2020 15:51:54 +0000 Subject: [PATCH] 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 --- .../src/dependencies/dependency_resolver.ts | 28 +++++-- packages/compiler-cli/ngcc/src/main.ts | 6 +- .../ngcc/src/packages/configuration.ts | 15 +++- .../dependencies/dependency_resolver_spec.ts | 78 +++++++++++++++++-- ...irectory_walker_entry_point_finder_spec.ts | 6 +- .../targeted_entry_point_finder_spec.ts | 10 +-- 6 files changed, 119 insertions(+), 24 deletions(-) diff --git a/packages/compiler-cli/ngcc/src/dependencies/dependency_resolver.ts b/packages/compiler-cli/ngcc/src/dependencies/dependency_resolver.ts index 8b13de349b..ef9d2c4e55 100644 --- a/packages/compiler-cli/ngcc/src/dependencies/dependency_resolver.ts +++ b/packages/compiler-cli/ngcc/src/dependencies/dependency_resolver.ts @@ -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>, 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[] { + 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 { diff --git a/packages/compiler-cli/ngcc/src/main.ts b/packages/compiler-cli/ngcc/src/main.ts index 05cdaf9f13..a335afb665 100644 --- a/packages/compiler-cli/ngcc/src/main.ts +++ b/packages/compiler-cli/ngcc/src/main.ts @@ -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, diff --git a/packages/compiler-cli/ngcc/src/packages/configuration.ts b/packages/compiler-cli/ngcc/src/packages/configuration.ts index 64c52b7c68..eef16381b2 100644 --- a/packages/compiler-cli/ngcc/src/packages/configuration.ts +++ b/packages/compiler-cli/ngcc/src/packages/configuration.ts @@ -13,7 +13,12 @@ import {PackageJsonFormatPropertiesMap} from './entry_point'; /** * The format of a project level configuration file. */ -export interface NgccProjectConfig { packages: {[packagePath: string]: T}; } +export interface NgccProjectConfig { + /** + * 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; diff --git a/packages/compiler-cli/ngcc/test/dependencies/dependency_resolver_spec.ts b/packages/compiler-cli/ngcc/test/dependencies/dependency_resolver_spec.ts index 237e1c13a1..65ac921e24 100644 --- a/packages/compiler-cli/ngcc/test/dependencies/dependency_resolver_spec.ts +++ b/packages/compiler-cli/ngcc/test/dependencies/dependency_resolver_spec.ts @@ -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}; }; } diff --git a/packages/compiler-cli/ngcc/test/entry_point_finder/directory_walker_entry_point_finder_spec.ts b/packages/compiler-cli/ngcc/test/entry_point_finder/directory_walker_entry_point_finder_spec.ts index e8c36e8888..5e81d42808 100644 --- a/packages/compiler-cli/ngcc/test/entry_point_finder/directory_walker_entry_point_finder_spec.ts +++ b/packages/compiler-cli/ngcc/test/entry_point_finder/directory_walker_entry_point_finder_spec.ts @@ -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(); diff --git a/packages/compiler-cli/ngcc/test/entry_point_finder/targeted_entry_point_finder_spec.ts b/packages/compiler-cli/ngcc/test/entry_point_finder/targeted_entry_point_finder_spec.ts index 454458cf30..63d317fd2d 100644 --- a/packages/compiler-cli/ngcc/test/entry_point_finder/targeted_entry_point_finder_spec.ts +++ b/packages/compiler-cli/ngcc/test/entry_point_finder/targeted_entry_point_finder_spec.ts @@ -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();