diff --git a/astro-docs/src/content/docs/kb/enforce-module-boundaries.mdoc b/astro-docs/src/content/docs/kb/enforce-module-boundaries.mdoc index 365b514a114..6dac20b21ae 100644 --- a/astro-docs/src/content/docs/kb/enforce-module-boundaries.mdoc +++ b/astro-docs/src/content/docs/kb/enforce-module-boundaries.mdoc @@ -73,16 +73,16 @@ export default [ ## Options -| Property | Type | Default | Description | -| ---------------------------------- | ------------------------- | ------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| allow | _Array_ | _[]_ | List of imports that should be allowed without any checks | -| allowCircularSelfDependency | _boolean_ | _false_ | Disable check for self circular dependency when project imports from itself via alias path | -| banTransitiveDependencies | _boolean_ | _false_ | Ban import of dependencies that were not specified in the root or project's `package.json` | -| ignoredCircularDependencies | _Array<[string, string]>_ | _[]_ | List of project pairs that should be skipped from `Circular dependencies` checks, including the self-circular dependency check. E.g. `['feature-project-a', 'myapp']`. Project name can be replaced by catch all `*` for more generic matches. | -| checkDynamicDependenciesExceptions | _Array_ | _[]_ | List of imports that should be skipped for `Imports of lazy-loaded libraries forbidden` checks. E.g. `['@myorg/lazy-project/component/*', '@myorg/other-project']` | -| checkNestedExternalImports | _boolean_ | _false_ | Enable to enforce the check for banned external imports in the nested packages. Check [Dependency constraints](#dependency-constraints) for more information | -| enforceBuildableLibDependency | _boolean_ | _false_ | Enable to restrict the buildable libs from importing non-buildable libraries | -| depConstraints | _Array_ | _[]_ | List of dependency constraints between projects | +| Property | Type | Default | Description | +| ---------------------------------- | ------------------------- | ------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| allow | _Array_ | _[]_ | List of imports that should be allowed without any checks | +| allowCircularSelfDependency | _boolean_ | _false_ | Disable check for self circular dependency when project imports from itself via alias path | +| banTransitiveDependencies | _boolean_ | _false_ | Ban import of dependencies that were not specified in the root or project's `package.json` | +| ignoredCircularDependencies | _Array<[string, string]>_ | _[]_ | List of project pairs that should be skipped from `Circular dependencies` checks, including the self-circular dependency check. E.g. `['feature-project-a', 'myapp']`. Project name can be replaced by catch all `*` for more generic matches. | +| checkDynamicDependenciesExceptions | _Array_ | _[]_ | List of imports that should be skipped for `Imports of lazy-loaded libraries forbidden` checks. E.g. `['@myorg/lazy-project/component/*', '@myorg/other-project']` | +| checkNestedExternalImports | _boolean_ | _false_ | Enable to also apply the `bannedExternalImports` and `allowedExternalImports` constraints to the external packages that imported projects depend on, transitively. The nested check compares whole package names. In `bannedExternalImports`, a subpath entry such as `lodash/fp` only applies to direct imports. In `allowedExternalImports`, a subpath entry does not allow the package for the nested check, so the rule reports the nested dependency. Check [Dependency constraints](#dependency-constraints) for more information | +| enforceBuildableLibDependency | _boolean_ | _false_ | Enable to restrict the buildable libs from importing non-buildable libraries | +| depConstraints | _Array_ | _[]_ | List of dependency constraints between projects | ### Dependency constraints diff --git a/packages/eslint-plugin/src/rules/enforce-module-boundaries.spec.ts b/packages/eslint-plugin/src/rules/enforce-module-boundaries.spec.ts index 82a0ff6afe1..8b912132de9 100644 --- a/packages/eslint-plugin/src/rules/enforce-module-boundaries.spec.ts +++ b/packages/eslint-plugin/src/rules/enforce-module-boundaries.spec.ts @@ -664,6 +664,289 @@ describe('Enforce Module Boundaries (eslint)', () => { expect(failures[1].message).toEqual(message); }); + it('should error when importing a project that transitively depends on a banned external package', () => { + const failures = runRule( + { + depConstraints: [ + { sourceTag: 'api', bannedExternalImports: ['npm-package'] }, + ], + checkNestedExternalImports: true, + }, + `${process.cwd()}/proj/libs/api/src/index.ts`, + ` + import '@mycompany/impl'; + import('@mycompany/impl'); + `, + { + ...graph, + dependencies: { + ...graph.dependencies, + implName: [ + { + source: 'implName', + target: 'npm:npm-package', + type: DependencyType.static, + }, + ], + }, + }, + fileMap + ); + + const message = + 'A project tagged with "api" is not allowed to import "@mycompany/impl". Nested import of "npm-package" found at implName'; + expect(failures.length).toEqual(2); + expect(failures[0].message).toEqual(message); + expect(failures[1].message).toEqual(message); + }); + + it('should not error when importing a project whose transitive external dependencies are not banned', () => { + const failures = runRule( + { + depConstraints: [ + { sourceTag: 'api', bannedExternalImports: ['npm-package'] }, + ], + checkNestedExternalImports: true, + }, + `${process.cwd()}/proj/libs/api/src/index.ts`, + ` + import '@mycompany/impl'; + import('@mycompany/impl'); + `, + { + ...graph, + dependencies: { + ...graph.dependencies, + implName: [ + { + source: 'implName', + target: 'npm:npm-awesome-package', + type: DependencyType.static, + }, + ], + }, + }, + fileMap + ); + + expect(failures.length).toEqual(0); + }); + + describe.each([ + { + name: 'a mixed banned and allowed constraint', + constraint: { + sourceTag: 'api', + bannedExternalImports: ['npm-package'], + allowedExternalImports: ['npm-package2'], + }, + }, + { + name: 'an allowed-only constraint', + constraint: { + sourceTag: 'api', + allowedExternalImports: ['npm-package2'], + }, + }, + { + name: 'an empty allowed constraint', + constraint: { sourceTag: 'api', allowedExternalImports: [] }, + }, + ])('nested external imports with $name', ({ constraint }) => { + it('should error when importing a project that transitively depends on an external package outside the allowed list', () => { + const failures = runRule( + { depConstraints: [constraint], checkNestedExternalImports: true }, + `${process.cwd()}/proj/libs/api/src/index.ts`, + ` + import '@mycompany/impl'; + import('@mycompany/impl'); + `, + { + ...graph, + dependencies: { + ...graph.dependencies, + implName: [ + { + source: 'implName', + target: 'npm:npm-awesome-package', + type: DependencyType.static, + }, + ], + }, + }, + fileMap + ); + + const message = + 'A project tagged with "api" is not allowed to import "@mycompany/impl". Nested import of "npm-awesome-package" found at implName'; + expect(failures.length).toEqual(2); + expect(failures[0].message).toEqual(message); + expect(failures[1].message).toEqual(message); + }); + }); + + it('should not error when importing a project whose transitive external dependencies are in the allowed list', () => { + const failures = runRule( + { + depConstraints: [ + { sourceTag: 'api', allowedExternalImports: ['npm-awesome-*'] }, + ], + checkNestedExternalImports: true, + }, + `${process.cwd()}/proj/libs/api/src/index.ts`, + ` + import '@mycompany/impl'; + import('@mycompany/impl'); + `, + { + ...graph, + dependencies: { + ...graph.dependencies, + implName: [ + { + source: 'implName', + target: 'npm:npm-awesome-package', + type: DependencyType.static, + }, + ], + }, + }, + fileMap + ); + + expect(failures.length).toEqual(0); + }); + + it('should report the project owning the nested import when the banned package is more than one hop away', () => { + const failures = runRule( + { + depConstraints: [ + { sourceTag: 'api', bannedExternalImports: ['npm-package'] }, + ], + checkNestedExternalImports: true, + }, + `${process.cwd()}/proj/libs/api/src/index.ts`, + ` + import '@mycompany/impl'; + `, + { + ...graph, + dependencies: { + ...graph.dependencies, + implName: [ + { + source: 'implName', + target: 'impl2Name', + type: DependencyType.static, + }, + ], + impl2Name: [ + { + source: 'impl2Name', + target: 'npm:npm-package', + type: DependencyType.static, + }, + ], + }, + }, + fileMap + ); + + expect(failures.length).toEqual(1); + expect(failures[0].message).toEqual( + 'A project tagged with "api" is not allowed to import "@mycompany/impl". Nested import of "npm-package" found at impl2Name' + ); + }); + + it('should report a single violation when multiple dependency edges reach the same banned package', () => { + const failures = runRule( + { + depConstraints: [ + { sourceTag: 'api', bannedExternalImports: ['npm-package'] }, + ], + checkNestedExternalImports: true, + }, + `${process.cwd()}/proj/libs/api/src/index.ts`, + ` + import '@mycompany/impl'; + `, + { + ...graph, + dependencies: { + ...graph.dependencies, + implName: [ + { + source: 'implName', + target: 'npm:npm-package', + type: DependencyType.static, + }, + { + source: 'implName', + target: 'npm:npm-package', + type: DependencyType.dynamic, + }, + ], + }, + }, + fileMap + ); + + expect(failures.length).toEqual(1); + expect(failures[0].message).toEqual( + 'A project tagged with "api" is not allowed to import "@mycompany/impl". Nested import of "npm-package" found at implName' + ); + }); + + it('should report a single violation when multiple versions of the same banned package are reached', () => { + const failures = runRule( + { + depConstraints: [ + { sourceTag: 'api', bannedExternalImports: ['npm-package'] }, + ], + checkNestedExternalImports: true, + }, + `${process.cwd()}/proj/libs/api/src/index.ts`, + ` + import '@mycompany/impl'; + `, + { + ...graph, + externalNodes: { + ...graph.externalNodes, + 'npm:npm-package@2.0.0': { + name: 'npm:npm-package@2.0.0', + type: 'npm', + data: { + packageName: 'npm-package', + version: '2.0.0', + }, + }, + }, + dependencies: { + ...graph.dependencies, + implName: [ + { + source: 'implName', + target: 'npm:npm-package', + type: DependencyType.static, + }, + { + source: 'implName', + target: 'npm:npm-package@2.0.0', + type: DependencyType.static, + }, + ], + }, + }, + fileMap + ); + + expect(failures.length).toEqual(1); + expect(failures[0].message).toEqual( + 'A project tagged with "api" is not allowed to import "@mycompany/impl". Nested import of "npm-package" found at implName' + ); + }); + it('should error when importing transitive npm packages', () => { const failures = runRule( { diff --git a/packages/eslint-plugin/src/rules/enforce-module-boundaries.ts b/packages/eslint-plugin/src/rules/enforce-module-boundaries.ts index 45c64aa67a7..5ed36db3f26 100644 --- a/packages/eslint-plugin/src/rules/enforce-module-boundaries.ts +++ b/packages/eslint-plugin/src/rules/enforce-module-boundaries.ts @@ -189,7 +189,7 @@ export default ESLintUtils.RuleCreator( noImportsOfLazyLoadedLibraries: `Static imports of lazy-loaded libraries are forbidden.\n\nLibrary "{{targetProjectName}}" is lazy-loaded in these files:\n{{filePaths}}`, projectWithoutTagsCannotHaveDependencies: `A project without tags matching at least one constraint cannot depend on any libraries`, bannedExternalImportsViolation: `A project tagged with "{{sourceTag}}" is not allowed to import "{{imp}}"`, - nestedBannedExternalImportsViolation: `A project tagged with "{{sourceTag}}" is not allowed to import "{{imp}}". Nested import found at {{childProjectName}}`, + nestedBannedExternalImportsViolation: `A project tagged with "{{sourceTag}}" is not allowed to import "{{imp}}". Nested import of "{{packageName}}" found at {{childProjectName}}`, noTransitiveDependencies: `Only packages defined in the "package.json" can be imported. Transitive or unresolvable dependencies are not allowed.`, onlyTagsConstraintViolation: `A project tagged with "{{sourceTag}}" can only depend on libs tagged with {{tags}}`, emptyOnlyTagsConstraintViolation: @@ -754,14 +754,13 @@ export default ESLintUtils.RuleCreator( } if ( checkNestedExternalImports && - constraint.bannedExternalImports && - constraint.bannedExternalImports.length + (constraint.bannedExternalImports?.length || + constraint.allowedExternalImports) ) { const matches = hasBannedDependencies( transitiveExternalDeps, projectGraph, - constraint, - imp + constraint ); if (matches.length > 0) { matches.forEach(([target, violatingSource, constraint]) => { @@ -772,6 +771,7 @@ export default ESLintUtils.RuleCreator( sourceTag: isComboDepConstraint(constraint) ? constraint.allSourceTags.join('" and "') : constraint.sourceTag, + packageName: target.data.packageName, childProjectName: violatingSource.name, imp, }, diff --git a/packages/eslint-plugin/src/utils/runtime-lint-utils.spec.ts b/packages/eslint-plugin/src/utils/runtime-lint-utils.spec.ts index 3b6a07fdc6f..b7a83378b5b 100644 --- a/packages/eslint-plugin/src/utils/runtime-lint-utils.spec.ts +++ b/packages/eslint-plugin/src/utils/runtime-lint-utils.spec.ts @@ -312,15 +312,10 @@ describe('dependentsHaveBannedImport + findTransitiveExternalDependencies', () = it("should return empty array if any dependents don't have banned import", () => { expect( - hasBannedDependencies( - externalDependencies.slice(1), - graph, - { - sourceTag: 'a', - bannedExternalImports: ['angular'], - }, - 'react-native' - ) + hasBannedDependencies(externalDependencies.slice(1), graph, { + sourceTag: 'a', + bannedExternalImports: ['angular'], + }) ).toStrictEqual([]); }); @@ -331,12 +326,7 @@ describe('dependentsHaveBannedImport + findTransitiveExternalDependencies', () = }; expect( - hasBannedDependencies( - externalDependencies.slice(1), - graph, - constraint, - 'react-native' - ) + hasBannedDependencies(externalDependencies.slice(1), graph, constraint) ).toStrictEqual([[bannedTarget, d, constraint]]); }); @@ -347,41 +337,12 @@ describe('dependentsHaveBannedImport + findTransitiveExternalDependencies', () = }; expect( - hasBannedDependencies( - externalDependencies.slice(1), - graph, - constraint, - 'react' - ) + hasBannedDependencies(externalDependencies.slice(1), graph, constraint) ).toStrictEqual([ [nonBannedTarget, target, constraint], [nonBannedTarget, c, constraint], ]); }); - - it('should return undefined if no baneed external imports found', () => { - const constraint: DepConstraint = { - sourceTag: 'a', - bannedExternalImports: ['angular'], - }; - - expect( - hasBannedDependencies( - externalDependencies.slice(1), - graph, - constraint, - 'react-native' - ).length - ).toBe(0); - expect( - hasBannedDependencies( - externalDependencies.slice(1), - graph, - constraint, - 'react' - ).length - ).toBe(0); - }); }); describe('is terminal run', () => { diff --git a/packages/eslint-plugin/src/utils/runtime-lint-utils.ts b/packages/eslint-plugin/src/utils/runtime-lint-utils.ts index 5cba263bb9b..a441ec0539c 100644 --- a/packages/eslint-plugin/src/utils/runtime-lint-utils.ts +++ b/packages/eslint-plugin/src/utils/runtime-lint-utils.ts @@ -267,19 +267,22 @@ export function getSourceFilePath(sourceFileName: string, projectPath: string) { function isConstraintBanningProject( externalProject: ProjectGraphExternalNode, constraint: DepConstraint, - imp: string + importSpecifier: string ): boolean { const { allowedExternalImports, bannedExternalImports } = constraint; const { packageName } = externalProject.data; - if (imp !== packageName && !imp.startsWith(`${packageName}/`)) { + if ( + importSpecifier !== packageName && + !importSpecifier.startsWith(`${packageName}/`) + ) { return false; } /* Check if import is banned... */ if ( bannedExternalImports?.some((importDefinition) => - mapGlobToRegExp(importDefinition).test(imp) + mapGlobToRegExp(importDefinition).test(importSpecifier) ) ) { return true; @@ -288,8 +291,8 @@ function isConstraintBanningProject( /* ... then check if there is a whitelist and if there is a match in the whitelist. */ return allowedExternalImports?.every( (importDefinition) => - !imp.startsWith(packageName) || - !mapGlobToRegExp(importDefinition).test(imp) + !importSpecifier.startsWith(packageName) || + !mapGlobToRegExp(importDefinition).test(importSpecifier) ); } @@ -338,13 +341,18 @@ export function findTransitiveExternalDependencies( } const externalDependencies = []; + const seen = new Set(); for (let i = 0; i < allReachableProjects.length; i++) { const dependencies = graph.dependencies[allReachableProjects[i]]; if (dependencies) { for (let d = 0; d < dependencies.length; d++) { const dependency = dependencies[d]; if (graph.externalNodes[dependency.target]) { - externalDependencies.push(dependency); + const key = `${dependency.source}|${graph.externalNodes[dependency.target].data.packageName}`; + if (!seen.has(key)) { + seen.add(key); + externalDependencies.push(dependency); + } } } } @@ -363,8 +371,7 @@ export function findTransitiveExternalDependencies( export function hasBannedDependencies( externalDependencies: ProjectGraphDependency[], graph: ProjectGraph, - depConstraint: DepConstraint, - imp: string + depConstraint: DepConstraint ): | Array<[ProjectGraphExternalNode, ProjectGraphProjectNode, DepConstraint]> | undefined { @@ -373,7 +380,7 @@ export function hasBannedDependencies( isConstraintBanningProject( graph.externalNodes[dependency.target], depConstraint, - imp + graph.externalNodes[dependency.target].data.packageName ) ) .map((dep) => [