From 1f557fd01736417483ab950ea260f6422b2f07a4 Mon Sep 17 00:00:00 2001 From: Younes Jaaidi Date: Sat, 17 Dec 2022 13:51:50 +0100 Subject: [PATCH 1/5] chore(linter): add empty tests --- .../src/rules/enforce-module-boundaries.spec.ts | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/packages/eslint-plugin-nx/src/rules/enforce-module-boundaries.spec.ts b/packages/eslint-plugin-nx/src/rules/enforce-module-boundaries.spec.ts index fde70a6d92626..cc97091ddf857 100644 --- a/packages/eslint-plugin-nx/src/rules/enforce-module-boundaries.spec.ts +++ b/packages/eslint-plugin-nx/src/rules/enforce-module-boundaries.spec.ts @@ -463,6 +463,22 @@ describe('Enforce Module Boundaries (eslint)', () => { expect(failures[1].message).toEqual(message); }); + it.todo( + 'should not error when importing npm packages matching allowed external imports' + ); + + it.todo( + 'should error when importing npm packages not matching allowed external imports' + ); + + it.todo( + 'should not error when importing npm packages matching allowed glob pattern' + ); + + it.todo( + 'should error when importing npm packages not matching allowed glob pattern' + ); + it('should error when importing transitive npm packages', () => { const failures = runRule( { From fbf238b63a89b64670de03acad95317609c27ad3 Mon Sep 17 00:00:00 2001 From: Younes Jaaidi Date: Sat, 17 Dec 2022 14:03:25 +0100 Subject: [PATCH 2/5] chore(linter): add wip tests for allowedExternalImports --- .../rules/enforce-module-boundaries.spec.ts | 94 ++++++++++++++++--- 1 file changed, 82 insertions(+), 12 deletions(-) diff --git a/packages/eslint-plugin-nx/src/rules/enforce-module-boundaries.spec.ts b/packages/eslint-plugin-nx/src/rules/enforce-module-boundaries.spec.ts index cc97091ddf857..2b92f06f6e823 100644 --- a/packages/eslint-plugin-nx/src/rules/enforce-module-boundaries.spec.ts +++ b/packages/eslint-plugin-nx/src/rules/enforce-module-boundaries.spec.ts @@ -463,21 +463,91 @@ describe('Enforce Module Boundaries (eslint)', () => { expect(failures[1].message).toEqual(message); }); - it.todo( - 'should not error when importing npm packages matching allowed external imports' - ); + xit('🚧 should not error when importing npm packages matching allowed external imports', () => { + const failures = runRule( + { + depConstraints: [ + { sourceTag: 'api', allowedExternalImports: ['@angular/core'] }, + ], + }, + `${process.cwd()}/proj/libs/api/src/index.ts`, + ` + import { Component } from '@angular/core'; + import '@angular/core'; + import('@angular/core'); + `, + graph + ); - it.todo( - 'should error when importing npm packages not matching allowed external imports' - ); + expect(failures.length).toEqual(0); + }); - it.todo( - 'should not error when importing npm packages matching allowed glob pattern' - ); + xit('🚧 should error when importing npm packages not matching allowed external imports', () => { + const failures = runRule( + { + depConstraints: [ + { sourceTag: 'api', allowedExternalImports: ['@angular/core'] }, + ], + }, + `${process.cwd()}/proj/libs/api/src/index.ts`, + ` + import { Injectable } from '@nestjs/core'; + import '@nestjs/core'; + import('@nestjs/core'); + `, + graph + ); - it.todo( - 'should error when importing npm packages not matching allowed glob pattern' - ); + const message = + 'A project tagged with "api" is not allowed to import the "@nestjs/core" package'; + expect(failures.length).toEqual(3); + expect(failures[0].message).toEqual(message); + expect(failures[1].message).toEqual(message); + expect(failures[2].message).toEqual(message); + }); + + xit('🚧 should not error when importing npm packages matching allowed glob pattern', () => { + const failures = runRule( + { + depConstraints: [ + { sourceTag: 'api', allowedExternalImports: ['@angular/*'] }, + ], + }, + `${process.cwd()}/proj/libs/api/src/index.ts`, + ` + import { Component } from '@angular/core'; + import '@angular/core'; + import('@angular/core'); + `, + graph + ); + + expect(failures.length).toEqual(0); + }); + + xit('🚧 should error when importing npm packages not matching allowed glob pattern', () => { + const failures = runRule( + { + depConstraints: [ + { sourceTag: 'api', allowedExternalImports: ['@angular/*'] }, + ], + }, + `${process.cwd()}/proj/libs/api/src/index.ts`, + ` + import { Injectable } from '@nestjs/core'; + import '@nestjs/core'; + import('@nestjs/core'); + `, + graph + ); + + const message = + 'A project tagged with "api" is not allowed to import the "@nestjs/core" package'; + expect(failures.length).toEqual(3); + expect(failures[0].message).toEqual(message); + expect(failures[1].message).toEqual(message); + expect(failures[2].message).toEqual(message); + }); it('should error when importing transitive npm packages', () => { const failures = runRule( From d50512d24edb7b092796a841614495939fc93e1c Mon Sep 17 00:00:00 2001 From: Younes Jaaidi Date: Sat, 17 Dec 2022 14:50:46 +0100 Subject: [PATCH 3/5] feat(linter): add allowedExternalImports option eslint's enforce-module-boundaries' allowedExternalImports option will only allow the project to import npm packages matching the string or wildcard Closes #12870 --- .../rules/enforce-module-boundaries.spec.ts | 66 +++++++++++-------- .../src/rules/enforce-module-boundaries.ts | 1 + .../src/utils/runtime-lint-utils.ts | 28 +++++--- 3 files changed, 61 insertions(+), 34 deletions(-) diff --git a/packages/eslint-plugin-nx/src/rules/enforce-module-boundaries.spec.ts b/packages/eslint-plugin-nx/src/rules/enforce-module-boundaries.spec.ts index 2b92f06f6e823..01611c9e2be46 100644 --- a/packages/eslint-plugin-nx/src/rules/enforce-module-boundaries.spec.ts +++ b/packages/eslint-plugin-nx/src/rules/enforce-module-boundaries.spec.ts @@ -463,18 +463,17 @@ describe('Enforce Module Boundaries (eslint)', () => { expect(failures[1].message).toEqual(message); }); - xit('🚧 should not error when importing npm packages matching allowed external imports', () => { + it('should not error when importing npm packages matching allowed external imports', () => { const failures = runRule( { depConstraints: [ - { sourceTag: 'api', allowedExternalImports: ['@angular/core'] }, + { sourceTag: 'api', allowedExternalImports: ['npm-package'] }, ], }, `${process.cwd()}/proj/libs/api/src/index.ts`, ` - import { Component } from '@angular/core'; - import '@angular/core'; - import('@angular/core'); + import 'npm-package'; + import('npm-package'); `, graph ); @@ -482,42 +481,39 @@ describe('Enforce Module Boundaries (eslint)', () => { expect(failures.length).toEqual(0); }); - xit('🚧 should error when importing npm packages not matching allowed external imports', () => { + it('should error when importing npm packages not matching allowed external imports', () => { const failures = runRule( { depConstraints: [ - { sourceTag: 'api', allowedExternalImports: ['@angular/core'] }, + { sourceTag: 'api', allowedExternalImports: ['npm-package'] }, ], }, `${process.cwd()}/proj/libs/api/src/index.ts`, ` - import { Injectable } from '@nestjs/core'; - import '@nestjs/core'; - import('@nestjs/core'); + import 'npm-awesome-package'; + import('npm-awesome-package'); `, graph ); const message = - 'A project tagged with "api" is not allowed to import the "@nestjs/core" package'; - expect(failures.length).toEqual(3); + 'A project tagged with "api" is not allowed to import the "npm-awesome-package" package'; + expect(failures.length).toEqual(2); expect(failures[0].message).toEqual(message); expect(failures[1].message).toEqual(message); - expect(failures[2].message).toEqual(message); }); - xit('🚧 should not error when importing npm packages matching allowed glob pattern', () => { + it('should not error when importing npm packages matching allowed glob pattern', () => { const failures = runRule( { depConstraints: [ - { sourceTag: 'api', allowedExternalImports: ['@angular/*'] }, + { sourceTag: 'api', allowedExternalImports: ['npm-awesome-*'] }, ], }, `${process.cwd()}/proj/libs/api/src/index.ts`, ` - import { Component } from '@angular/core'; - import '@angular/core'; - import('@angular/core'); + import 'npm-awesome-package'; + import('npm-awesome-package'); `, graph ); @@ -525,28 +521,46 @@ describe('Enforce Module Boundaries (eslint)', () => { expect(failures.length).toEqual(0); }); - xit('🚧 should error when importing npm packages not matching allowed glob pattern', () => { + it('should error when importing npm packages not matching allowed glob pattern', () => { const failures = runRule( { depConstraints: [ - { sourceTag: 'api', allowedExternalImports: ['@angular/*'] }, + { sourceTag: 'api', allowedExternalImports: ['npm-awesome-*'] }, ], }, `${process.cwd()}/proj/libs/api/src/index.ts`, ` - import { Injectable } from '@nestjs/core'; - import '@nestjs/core'; - import('@nestjs/core'); + import 'npm-package'; + import('npm-package'); `, graph ); const message = - 'A project tagged with "api" is not allowed to import the "@nestjs/core" package'; - expect(failures.length).toEqual(3); + 'A project tagged with "api" is not allowed to import the "npm-package" package'; + expect(failures.length).toEqual(2); + expect(failures[0].message).toEqual(message); + expect(failures[1].message).toEqual(message); + }); + + it('should error when importing any npm package if none is allowed', () => { + const failures = runRule( + { + depConstraints: [{ sourceTag: 'api', allowedExternalImports: [] }], + }, + `${process.cwd()}/proj/libs/api/src/index.ts`, + ` + import 'npm-package'; + import('npm-package'); + `, + graph + ); + + const message = + 'A project tagged with "api" is not allowed to import the "npm-package" package'; + expect(failures.length).toEqual(2); expect(failures[0].message).toEqual(message); expect(failures[1].message).toEqual(message); - expect(failures[2].message).toEqual(message); }); it('should error when importing transitive npm packages', () => { diff --git a/packages/eslint-plugin-nx/src/rules/enforce-module-boundaries.ts b/packages/eslint-plugin-nx/src/rules/enforce-module-boundaries.ts index 81273dc9b9c3e..4d2a09f1ece38 100644 --- a/packages/eslint-plugin-nx/src/rules/enforce-module-boundaries.ts +++ b/packages/eslint-plugin-nx/src/rules/enforce-module-boundaries.ts @@ -103,6 +103,7 @@ export default createESLintRule({ }, ], onlyDependOnLibsWithTags: [{ type: 'string' }], + allowedExternalImports: [{ type: 'string' }], bannedExternalImports: [{ type: 'string' }], notDependOnLibsWithTags: [{ type: 'string' }], }, diff --git a/packages/eslint-plugin-nx/src/utils/runtime-lint-utils.ts b/packages/eslint-plugin-nx/src/utils/runtime-lint-utils.ts index 02ca8a41137e2..b963b36ababc5 100644 --- a/packages/eslint-plugin-nx/src/utils/runtime-lint-utils.ts +++ b/packages/eslint-plugin-nx/src/utils/runtime-lint-utils.ts @@ -25,12 +25,14 @@ type SingleSourceTagConstraint = { sourceTag: string; onlyDependOnLibsWithTags?: string[]; notDependOnLibsWithTags?: string[]; + allowedExternalImports?: string[]; bannedExternalImports?: string[]; }; type ComboSourceTagConstraint = { allSourceTags: string[]; onlyDependOnLibsWithTags?: string[]; notDependOnLibsWithTags?: string[]; + allowedExternalImports?: string[]; bannedExternalImports?: string[]; }; export type DepConstraint = @@ -209,9 +211,23 @@ function isConstraintBanningProject( externalProject: ProjectGraphExternalNode, constraint: DepConstraint ): boolean { - return constraint.bannedExternalImports.some((importDefinition) => - parseImportWildcards(importDefinition).test( - externalProject.data.packageName + const { allowedExternalImports, bannedExternalImports } = constraint; + const { packageName } = externalProject.data; + + /* Check if import is banned... */ + if ( + bannedExternalImports?.some((importDefinition) => + parseImportWildcards(importDefinition).test(packageName) + ) + ) { + return true; + } + + /* ... then check if there is a whitelist and if there is a match in the whitelist. */ + return ( + allowedExternalImports != null && + !allowedExternalImports.some((importDefinition) => + parseImportWildcards(importDefinition).test(packageName) ) ); } @@ -230,11 +246,7 @@ export function hasBannedImport( tags = [c.sourceTag]; } - return ( - c.bannedExternalImports && - c.bannedExternalImports.length && - tags.every((t) => (source.data.tags || []).includes(t)) - ); + return tags.every((t) => (source.data.tags || []).includes(t)); }); return depConstraints.find((constraint) => isConstraintBanningProject(target, constraint) From fcfbb7031aac7814d2061ca66c4f4810096fd953 Mon Sep 17 00:00:00 2001 From: Younes Jaaidi Date: Sat, 17 Dec 2022 15:24:10 +0100 Subject: [PATCH 4/5] docs(linter): add allowedExternalImports option to the ban external imports recipe #12870 --- docs/shared/recipes/ban-external-imports.md | 40 +++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/docs/shared/recipes/ban-external-imports.md b/docs/shared/recipes/ban-external-imports.md index c3393107a41dc..7eb69ef1cf3af 100644 --- a/docs/shared/recipes/ban-external-imports.md +++ b/docs/shared/recipes/ban-external-imports.md @@ -59,3 +59,43 @@ Another common example is ensuring that util libraries stay framework-free by ba // ... more ESLint config here } ``` + +## Whitelisting external imports with `allowedExternalImports` + +If you need a more restrictive approach, you can use the `allowedExternalImports` option to ensure that a project only imports from a specific set of packages. +This is useful if you want to enforce separation of concerns _(e.g. keeping your domain logic clean from infrastructure concerns, or ui libraries clean from data access concerns)_ or keep some parts of your codebase framework-free or library-free. + +```jsonc {% fileName=".eslintrc.json" %} +{ + // ... more ESLint config here + + // nx-enforce-module-boundaries should already exist at the top-level of your config + "nx-enforce-module-boundaries": [ + "error", + { + "allow": [], + // update depConstraints based on your tags + "depConstraints": [ + // limiting the dependencies of util libraries to the bare minimum + // projects tagged with "type:util" can only import from "date-fns" + { + "sourceTag": "type:util", + "allowedExternalImports": ["date-fns"] + }, + // ui libraries clean from data access concerns + // projects tagged with "type:ui" can only import pacages matching "@angular/*" except "@angular/common/http" + { + "sourceTag": "type:ui", + "allowedExternalImports": ["@angular/*"], + "bannedExternalImports": ["@angular/common/http"] + }, + // keeping the domain logic clean from infrastructure concerns + // projects tagged with "type:core" can't import any external packages. + { + "sourceTag": "type:core", + "allowedExternalImports": [] + } + ] + } + ] +``` From 9569a87dd7f3b690c721b1f47f93b1752932c644 Mon Sep 17 00:00:00 2001 From: Younes Jaaidi Date: Mon, 19 Dec 2022 21:36:33 +0100 Subject: [PATCH 5/5] chore(linter): tidy up --- packages/eslint-plugin-nx/src/utils/runtime-lint-utils.ts | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/packages/eslint-plugin-nx/src/utils/runtime-lint-utils.ts b/packages/eslint-plugin-nx/src/utils/runtime-lint-utils.ts index b963b36ababc5..d14956e6df9c7 100644 --- a/packages/eslint-plugin-nx/src/utils/runtime-lint-utils.ts +++ b/packages/eslint-plugin-nx/src/utils/runtime-lint-utils.ts @@ -224,11 +224,9 @@ function isConstraintBanningProject( } /* ... then check if there is a whitelist and if there is a match in the whitelist. */ - return ( - allowedExternalImports != null && - !allowedExternalImports.some((importDefinition) => - parseImportWildcards(importDefinition).test(packageName) - ) + return allowedExternalImports?.every( + (importDefinition) => + !parseImportWildcards(importDefinition).test(packageName) ); }