From b3f5ce51d700e3930ec1fd90cf817a04b5fba98b Mon Sep 17 00:00:00 2001 From: Jay Tavares Date: Tue, 14 Sep 2021 14:10:11 -0400 Subject: [PATCH 1/9] feat(core): implement notDependOnLibsWithTags lib dependency constraint ISSUES CLOSED: #984 --- .../rules/enforce-module-boundaries.spec.ts | 41 +++++++++++++++++++ .../src/rules/enforce-module-boundaries.ts | 29 +++++++++++-- .../workspace/src/utils/runtime-lint-utils.ts | 9 +++- 3 files changed, 74 insertions(+), 5 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 0411310a5503c..c0ec7952f029b 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 @@ -249,6 +249,28 @@ describe('Enforce Module Boundaries (eslint)', () => { files: [createFile(`libs/impl/src/index.ts`)], }, }, + publicName: { + name: 'publicName', + type: ProjectType.lib, + data: { + root: 'libs/public', + tags: ['public'], + implicitDependencies: [], + architect: {}, + files: [createFile(`libs/public/src/index.ts`)], + }, + }, + privateName: { + name: 'privateName', + type: ProjectType.lib, + data: { + root: 'libs/private', + tags: ['private'], + implicitDependencies: [], + architect: {}, + files: [createFile(`libs/private/src/index.ts`)], + }, + }, untaggedName: { name: 'untaggedName', type: ProjectType.lib, @@ -304,6 +326,7 @@ describe('Enforce Module Boundaries (eslint)', () => { { sourceTag: 'impl', onlyDependOnLibsWithTags: ['api', 'impl'] }, { sourceTag: 'domain1', onlyDependOnLibsWithTags: ['domain1'] }, { sourceTag: 'domain2', onlyDependOnLibsWithTags: ['domain2'] }, + { sourceTag: 'public', notDependOnLibsWithTags: ['private'] }, ], }; @@ -447,6 +470,24 @@ describe('Enforce Module Boundaries (eslint)', () => { expect(failures[1].message).toEqual(message); }); + it('should error when the target library has a disallowed tag', () => { + const failures = runRule( + depConstraints, + `${process.cwd()}/proj/libs/public/src/index.ts`, + ` + import '@mycompany/private'; + import('@mycompany/private'); + `, + graph + ); + + const message = + 'A project tagged with "public" can not depend on libs tagged with "private"'; + expect(failures.length).toEqual(2); + expect(failures[0].message).toEqual(message); + expect(failures[1].message).toEqual(message); + }); + it('should error when the source library is untagged', () => { const failures = runRule( depConstraints, 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 2ab49c2af3423..813b8a1392abe 100644 --- a/packages/eslint-plugin-nx/src/rules/enforce-module-boundaries.ts +++ b/packages/eslint-plugin-nx/src/rules/enforce-module-boundaries.ts @@ -7,6 +7,7 @@ import { getSourceFilePath, hasBuildExecutor, hasNoneOfTheseTags, + hasAnyOfTheseTags, isAbsoluteImportIntoAnotherProject, isRelativeImportIntoAnotherProject, mapProjectGraphFiles, @@ -55,9 +56,10 @@ export type MessageIds = | 'noImportOfNonBuildableLibraries' | 'noImportsOfLazyLoadedLibraries' | 'projectWithoutTagsCannotHaveDependencies' - | 'tagConstraintViolation' | 'bannedExternalImportsViolation' - | 'noTransitiveDependencies'; + | 'noTransitiveDependencies' + | 'onlyTagsConstraintViolation' + | 'notTagsConstraintViolation'; export const RULE_NAME = 'enforce-module-boundaries'; export default createESLintRule({ @@ -84,6 +86,7 @@ export default createESLintRule({ sourceTag: { type: 'string' }, onlyDependOnLibsWithTags: [{ type: 'string' }], bannedExternalImports: [{ type: 'string' }], + notDependOnLibsWithTags: [{ type: 'string' }], }, additionalProperties: false, }, @@ -102,9 +105,10 @@ export default createESLintRule({ 'Buildable libraries cannot import or export from non-buildable libraries', noImportsOfLazyLoadedLibraries: `Imports of lazy-loaded libraries are forbidden`, projectWithoutTagsCannotHaveDependencies: `A project without tags matching at least one constraint cannot depend on any libraries`, - tagConstraintViolation: `A project tagged with "{{sourceTag}}" can only depend on libs tagged with {{allowedTags}}`, bannedExternalImportsViolation: `A project tagged with "{{sourceTag}}" is not allowed to import the "{{package}}" package`, noTransitiveDependencies: `Transitive dependencies are not allowed. Only packages defined in the "package.json" can be imported`, + onlyTagsConstraintViolation: `A project tagged with "{{sourceTag}}" can only depend on libs tagged with {{allowedTags}}`, + notTagsConstraintViolation: `A project tagged with "{{sourceTag}}" can not depend on libs tagged with {{disallowedTags}}`, }, }, defaultOptions: [ @@ -410,7 +414,7 @@ export default createESLintRule({ .join(', '); context.report({ node, - messageId: 'tagConstraintViolation', + messageId: 'onlyTagsConstraintViolation', data: { sourceTag: constraint.sourceTag, allowedTags, @@ -418,6 +422,23 @@ export default createESLintRule({ }); return; } + if ( + constraint.notDependOnLibsWithTags && + hasAnyOfTheseTags(targetProject, constraint.notDependOnLibsWithTags) + ) { + const disallowedTags = constraint.notDependOnLibsWithTags + .map((s) => `"${s}"`) + .join(', '); + context.report({ + node, + messageId: 'notTagsConstraintViolation', + data: { + sourceTag: constraint.sourceTag, + disallowedTags: disallowedTags, + }, + }); + return; + } } } } diff --git a/packages/workspace/src/utils/runtime-lint-utils.ts b/packages/workspace/src/utils/runtime-lint-utils.ts index 2c5c66f719b8d..2617a76349fe2 100644 --- a/packages/workspace/src/utils/runtime-lint-utils.ts +++ b/packages/workspace/src/utils/runtime-lint-utils.ts @@ -26,16 +26,23 @@ export type Deps = { [projectName: string]: ProjectGraphDependency[] }; export type DepConstraint = { sourceTag: string; onlyDependOnLibsWithTags: string[]; + notDependOnLibsWithTags: string[]; bannedExternalImports?: string[]; }; export function hasNoneOfTheseTags( - proj: ProjectGraphProjectNode, + proj: ProjectGraphProjectNode, tags: string[] ) { return tags.filter((tag) => hasTag(proj, tag)).length === 0; } +export function hasAnyOfTheseTags(proj: ProjectGraphProjectNode, tags: string[]) { + return ( + tags.filter((disallowedTag) => hasTag(proj, disallowedTag)).length !== 0 + ); +} + function hasTag(proj: ProjectGraphProjectNode, tag: string) { return tag === '*' || (proj.data.tags || []).indexOf(tag) > -1; } From c61df8c4543acde1e6f414776c5ed22db72a197a Mon Sep 17 00:00:00 2001 From: Jay Tavares Date: Thu, 20 Jan 2022 17:42:41 -0500 Subject: [PATCH 2/9] feat(core): enhance notDependOnLibsWithTags lib dependency constraint to search for disallowed tags in project graph --- .../rules/enforce-module-boundaries.spec.ts | 64 +++++++++++++++++++ .../src/rules/enforce-module-boundaries.ts | 6 +- .../workspace/src/utils/runtime-lint-utils.ts | 26 ++++++-- 3 files changed, 91 insertions(+), 5 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 c0ec7952f029b..c427378721f69 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 @@ -488,6 +488,70 @@ describe('Enforce Module Boundaries (eslint)', () => { expect(failures[1].message).toEqual(message); }); + it('should error when there is a disallowed tag in the dependency tree', () => { + const failures = runRule( + depConstraints, + `${process.cwd()}/proj/libs/public/src/index.ts`, + ` + import '@mycompany/dependsOnPrivate'; + import('@mycompany/dependsOnPrivate'); + `, + { + nodes: { + publicName: { + name: 'publicName', + type: ProjectType.lib, + data: { + root: 'libs/public', + tags: ['public'], + implicitDependencies: [], + architect: {}, + files: [createFile(`libs/public/src/index.ts`)], + }, + }, + privateName: { + name: 'privateName', + type: ProjectType.lib, + data: { + root: 'libs/private', + tags: ['private'], + implicitDependencies: [], + architect: {}, + files: [createFile(`libs/private/src/index.ts`)], + }, + }, + dependsOnPrivateName: { + name: 'dependsOnPrivateName', + type: ProjectType.lib, + data: { + root: 'libs/dependsOnPrivate', + tags: [], + implicitDependencies: [], + architect: {}, + files: [createFile(`libs/dependsOnPrivate/src/index.ts`)], + }, + }, + }, + dependencies: { + dependsOnPrivateName: [ + { + source: 'dependsOnPrivateName', + target: 'privateName', + type: DependencyType.static, + }, + ], + }, + } + ); + + expect(failures.length).toEqual(2); + // TODO: Add project dependency path to message + const message = + 'A project tagged with "public" can not depend on libs tagged with "private"'; + expect(failures[0].message).toEqual(message); + expect(failures[1].message).toEqual(message); + }); + it('should error when the source library is untagged', () => { const failures = runRule( depConstraints, 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 813b8a1392abe..585ae86898fa5 100644 --- a/packages/eslint-plugin-nx/src/rules/enforce-module-boundaries.ts +++ b/packages/eslint-plugin-nx/src/rules/enforce-module-boundaries.ts @@ -424,7 +424,11 @@ export default createESLintRule({ } if ( constraint.notDependOnLibsWithTags && - hasAnyOfTheseTags(targetProject, constraint.notDependOnLibsWithTags) + hasAnyOfTheseTags( + projectGraph, + targetProject.name, + constraint.notDependOnLibsWithTags + ) ) { const disallowedTags = constraint.notDependOnLibsWithTags .map((s) => `"${s}"`) diff --git a/packages/workspace/src/utils/runtime-lint-utils.ts b/packages/workspace/src/utils/runtime-lint-utils.ts index 2617a76349fe2..5d37df0c2b773 100644 --- a/packages/workspace/src/utils/runtime-lint-utils.ts +++ b/packages/workspace/src/utils/runtime-lint-utils.ts @@ -37,10 +37,28 @@ export function hasNoneOfTheseTags( return tags.filter((tag) => hasTag(proj, tag)).length === 0; } -export function hasAnyOfTheseTags(proj: ProjectGraphProjectNode, tags: string[]) { - return ( - tags.filter((disallowedTag) => hasTag(proj, disallowedTag)).length !== 0 - ); +export function hasAnyOfTheseTags( + graph: ProjectGraph, + projectName: string, + tags: string[], + visited?: string[] +) { + visited = visited ?? []; + + let found = + tags.filter((disallowedTag) => + hasTag(graph.nodes[projectName], disallowedTag) + ).length !== 0; + + if (found) return true; + + for (let d of graph.dependencies[projectName] || []) { + if (visited.indexOf(d.target) > -1) continue; + visited.push(d.target); + if (hasAnyOfTheseTags(graph, d.target, tags, visited)) return true; + } + + return false; } function hasTag(proj: ProjectGraphProjectNode, tag: string) { From d116e013032ed84537f53cb536c4959f9cfe4e6b Mon Sep 17 00:00:00 2001 From: Jay Tavares Date: Sun, 23 Jan 2022 22:28:03 -0500 Subject: [PATCH 3/9] fix(testing): fix eslint-plugin-nx failing tests --- .../src/rules/enforce-module-boundaries.spec.ts | 3 +++ 1 file changed, 3 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 c427378721f69..1fe59b0d7b05f 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 @@ -34,6 +34,9 @@ const tsconfig = { '@mycompany/myapp-e2e': ['apps/myapp-e2e/src/index.ts'], '@mycompany/mylib': ['libs/mylib/src/index.ts'], '@mycompany/mylibName': ['libs/mylibName/src/index.ts'], + '@mycompany/public': ['libs/public/src/index.ts'], + '@mycompany/dependsOnPrivate': ['libs/dependsOnPrivate/src/index.ts'], + '@mycompany/private': ['libs/private/src/index.ts'], '@mycompany/anotherlibName': ['libs/anotherlibName/src/index.ts'], '@mycompany/badcirclelib': ['libs/badcirclelib/src/index.ts'], '@mycompany/domain1': ['libs/domain1/src/index.ts'], From df9af3ef165ece781d2111f5980d9703a1bfe0d3 Mon Sep 17 00:00:00 2001 From: Jay Tavares Date: Mon, 24 Jan 2022 15:02:20 -0500 Subject: [PATCH 4/9] cleanup(testing): add missing references to enforce-module-boundaries.spec.ts test files --- .../rules/enforce-module-boundaries.spec.ts | 79 ++++++++----------- 1 file changed, 31 insertions(+), 48 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 1fe59b0d7b05f..2f31316487b9d 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 @@ -81,6 +81,9 @@ const fileSys = { './libs/domain2/src/index.ts': '', './libs/buildableLib/src/main.ts': '', './libs/nonBuildableLib/src/main.ts': '', + './libs/public/src/index.ts': '', + './libs/dependsOnPrivate/src/index.ts': '', + './libs/private/src/index.ts': '', './tsconfig.base.json': JSON.stringify(tsconfig), './package.json': JSON.stringify(packageJson), }; @@ -263,6 +266,19 @@ describe('Enforce Module Boundaries (eslint)', () => { files: [createFile(`libs/public/src/index.ts`)], }, }, + dependsOnPrivateName: { + name: 'dependsOnPrivateName', + type: ProjectType.lib, + data: { + root: 'libs/dependsOnPrivate', + tags: [], + implicitDependencies: [], + architect: {}, + files: [ + createFile(`libs/dependsOnPrivate/src/index.ts`, ['privateName']), + ], + }, + }, privateName: { name: 'privateName', type: ProjectType.lib, @@ -271,7 +287,11 @@ describe('Enforce Module Boundaries (eslint)', () => { tags: ['private'], implicitDependencies: [], architect: {}, - files: [createFile(`libs/private/src/index.ts`)], + files: [ + createFile( + `libs/private/src/index.tslibs/private/src/index.tslibs/private/src/index.ts` + ), + ], }, }, untaggedName: { @@ -320,7 +340,15 @@ describe('Enforce Module Boundaries (eslint)', () => { }, }, }, - dependencies: {}, + dependencies: { + dependsOnPrivateName: [ + { + source: 'dependsOnPrivateName', + target: 'privateName', + type: DependencyType.static, + }, + ], + }, }; const depConstraints = { @@ -499,52 +527,7 @@ describe('Enforce Module Boundaries (eslint)', () => { import '@mycompany/dependsOnPrivate'; import('@mycompany/dependsOnPrivate'); `, - { - nodes: { - publicName: { - name: 'publicName', - type: ProjectType.lib, - data: { - root: 'libs/public', - tags: ['public'], - implicitDependencies: [], - architect: {}, - files: [createFile(`libs/public/src/index.ts`)], - }, - }, - privateName: { - name: 'privateName', - type: ProjectType.lib, - data: { - root: 'libs/private', - tags: ['private'], - implicitDependencies: [], - architect: {}, - files: [createFile(`libs/private/src/index.ts`)], - }, - }, - dependsOnPrivateName: { - name: 'dependsOnPrivateName', - type: ProjectType.lib, - data: { - root: 'libs/dependsOnPrivate', - tags: [], - implicitDependencies: [], - architect: {}, - files: [createFile(`libs/dependsOnPrivate/src/index.ts`)], - }, - }, - }, - dependencies: { - dependsOnPrivateName: [ - { - source: 'dependsOnPrivateName', - target: 'privateName', - type: DependencyType.static, - }, - ], - }, - } + graph ); expect(failures.length).toEqual(2); From 3b8802b81d497a0d898552e001807965dd430f2f Mon Sep 17 00:00:00 2001 From: Jay Tavares Date: Fri, 4 Feb 2022 15:21:49 -0500 Subject: [PATCH 5/9] cleanup(core): simplify tags variable naming rename allowedTags & disallowedTags variables for consistency and simplicity --- .../src/rules/enforce-module-boundaries.ts | 12 ++++++------ .../src/tslint/nxEnforceModuleBoundariesRule.ts | 4 ++-- packages/workspace/src/utils/runtime-lint-utils.ts | 4 +--- 3 files changed, 9 insertions(+), 11 deletions(-) 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 585ae86898fa5..2fe4cb5296bcd 100644 --- a/packages/eslint-plugin-nx/src/rules/enforce-module-boundaries.ts +++ b/packages/eslint-plugin-nx/src/rules/enforce-module-boundaries.ts @@ -107,8 +107,8 @@ export default createESLintRule({ 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 the "{{package}}" package`, noTransitiveDependencies: `Transitive dependencies are not allowed. Only packages defined in the "package.json" can be imported`, - onlyTagsConstraintViolation: `A project tagged with "{{sourceTag}}" can only depend on libs tagged with {{allowedTags}}`, - notTagsConstraintViolation: `A project tagged with "{{sourceTag}}" can not depend on libs tagged with {{disallowedTags}}`, + onlyTagsConstraintViolation: `A project tagged with "{{sourceTag}}" can only depend on libs tagged with {{tags}}`, + notTagsConstraintViolation: `A project tagged with "{{sourceTag}}" can not depend on libs tagged with {{tags}}`, }, }, defaultOptions: [ @@ -409,7 +409,7 @@ export default createESLintRule({ constraint.onlyDependOnLibsWithTags ) ) { - const allowedTags = constraint.onlyDependOnLibsWithTags + const tags = constraint.onlyDependOnLibsWithTags .map((s) => `"${s}"`) .join(', '); context.report({ @@ -417,7 +417,7 @@ export default createESLintRule({ messageId: 'onlyTagsConstraintViolation', data: { sourceTag: constraint.sourceTag, - allowedTags, + tags, }, }); return; @@ -430,7 +430,7 @@ export default createESLintRule({ constraint.notDependOnLibsWithTags ) ) { - const disallowedTags = constraint.notDependOnLibsWithTags + const tags = constraint.notDependOnLibsWithTags .map((s) => `"${s}"`) .join(', '); context.report({ @@ -438,7 +438,7 @@ export default createESLintRule({ messageId: 'notTagsConstraintViolation', data: { sourceTag: constraint.sourceTag, - disallowedTags: disallowedTags, + tags, }, }); return; diff --git a/packages/workspace/src/tslint/nxEnforceModuleBoundariesRule.ts b/packages/workspace/src/tslint/nxEnforceModuleBoundariesRule.ts index 5d6ca05c27061..967d7964fa9fd 100644 --- a/packages/workspace/src/tslint/nxEnforceModuleBoundariesRule.ts +++ b/packages/workspace/src/tslint/nxEnforceModuleBoundariesRule.ts @@ -277,10 +277,10 @@ class EnforceModuleBoundariesWalker extends Lint.RuleWalker { constraint.onlyDependOnLibsWithTags || [] ) ) { - const allowedTags = constraint.onlyDependOnLibsWithTags + const tags = constraint.onlyDependOnLibsWithTags .map((s) => `"${s}"`) .join(', '); - const error = `A project tagged with "${constraint.sourceTag}" can only depend on libs tagged with ${allowedTags}`; + const error = `A project tagged with "${constraint.sourceTag}" can only depend on libs tagged with ${tags}`; this.addFailureAt(node.getStart(), node.getWidth(), error); return; } diff --git a/packages/workspace/src/utils/runtime-lint-utils.ts b/packages/workspace/src/utils/runtime-lint-utils.ts index 5d37df0c2b773..876ef2a42b61e 100644 --- a/packages/workspace/src/utils/runtime-lint-utils.ts +++ b/packages/workspace/src/utils/runtime-lint-utils.ts @@ -46,9 +46,7 @@ export function hasAnyOfTheseTags( visited = visited ?? []; let found = - tags.filter((disallowedTag) => - hasTag(graph.nodes[projectName], disallowedTag) - ).length !== 0; + tags.filter((tag) => hasTag(graph.nodes[projectName], tag)).length !== 0; if (found) return true; From c08b25b4d964990cddc9838c39107c8a1d10c326 Mon Sep 17 00:00:00 2001 From: Miroslav Jonas Date: Fri, 4 Mar 2022 16:02:02 +0100 Subject: [PATCH 6/9] chore(linter): minor cleanup --- .../src/rules/enforce-module-boundaries.ts | 13 ++----- .../workspace/src/utils/runtime-lint-utils.ts | 38 +++++++++++++------ 2 files changed, 30 insertions(+), 21 deletions(-) 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 2fe4cb5296bcd..0c80bc96a3636 100644 --- a/packages/eslint-plugin-nx/src/rules/enforce-module-boundaries.ts +++ b/packages/eslint-plugin-nx/src/rules/enforce-module-boundaries.ts @@ -17,6 +17,7 @@ import { hasBannedImport, isDirectDependency, isTerminalRun, + stringifyTags, } from '@nrwl/workspace/src/utils/runtime-lint-utils'; import { AST_NODE_TYPES, @@ -403,42 +404,34 @@ export default createESLintRule({ for (let constraint of constraints) { if ( - constraint.onlyDependOnLibsWithTags && hasNoneOfTheseTags( targetProject, constraint.onlyDependOnLibsWithTags ) ) { - const tags = constraint.onlyDependOnLibsWithTags - .map((s) => `"${s}"`) - .join(', '); context.report({ node, messageId: 'onlyTagsConstraintViolation', data: { sourceTag: constraint.sourceTag, - tags, + tags: stringifyTags(constraint.onlyDependOnLibsWithTags), }, }); return; } if ( - constraint.notDependOnLibsWithTags && hasAnyOfTheseTags( projectGraph, targetProject.name, constraint.notDependOnLibsWithTags ) ) { - const tags = constraint.notDependOnLibsWithTags - .map((s) => `"${s}"`) - .join(', '); context.report({ node, messageId: 'notTagsConstraintViolation', data: { sourceTag: constraint.sourceTag, - tags, + tags: stringifyTags(constraint.notDependOnLibsWithTags), }, }); return; diff --git a/packages/workspace/src/utils/runtime-lint-utils.ts b/packages/workspace/src/utils/runtime-lint-utils.ts index 876ef2a42b61e..ed601130071c4 100644 --- a/packages/workspace/src/utils/runtime-lint-utils.ts +++ b/packages/workspace/src/utils/runtime-lint-utils.ts @@ -30,30 +30,46 @@ export type DepConstraint = { bannedExternalImports?: string[]; }; +export function stringifyTags(tags: string[]): string { + return tags.map((t) => `"${t}"`).join(', '); +} + export function hasNoneOfTheseTags( - proj: ProjectGraphProjectNode, - tags: string[] -) { + proj: ProjectGraphProjectNode, + tags?: string[] +): boolean { + if (!tags) { + return false; + } return tags.filter((tag) => hasTag(proj, tag)).length === 0; } export function hasAnyOfTheseTags( graph: ProjectGraph, projectName: string, - tags: string[], + tags?: string[], visited?: string[] -) { - visited = visited ?? []; - - let found = +): boolean { + if (!tags) { + return false; + } + const found = tags.filter((tag) => hasTag(graph.nodes[projectName], tag)).length !== 0; - if (found) return true; + if (found) { + return true; + } + + visited = visited ?? []; for (let d of graph.dependencies[projectName] || []) { - if (visited.indexOf(d.target) > -1) continue; + if (visited.indexOf(d.target) > -1) { + continue; + } visited.push(d.target); - if (hasAnyOfTheseTags(graph, d.target, tags, visited)) return true; + if (hasAnyOfTheseTags(graph, d.target, tags, visited)) { + return true; + } } return false; From 8c93ae9952f6679d9006c76188a80b33afa041f5 Mon Sep 17 00:00:00 2001 From: Miroslav Jonas Date: Mon, 7 Mar 2022 15:34:23 +0100 Subject: [PATCH 7/9] feat(linter): add project chain mapping for dependency tree --- .../rules/enforce-module-boundaries.spec.ts | 12 ++-- .../src/rules/enforce-module-boundaries.ts | 47 +++++++++------- .../tslint/nxEnforceModuleBoundariesRule.ts | 40 ++++++++++--- .../workspace/src/utils/runtime-lint-utils.ts | 56 +++++++++++-------- 4 files changed, 99 insertions(+), 56 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 2f31316487b9d..2eed692ca377c 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 @@ -512,8 +512,10 @@ describe('Enforce Module Boundaries (eslint)', () => { graph ); - const message = - 'A project tagged with "public" can not depend on libs tagged with "private"'; + const message = `A project tagged with "public" can not depend on libs tagged with "private" + +Violation detected in: +- privateName`; expect(failures.length).toEqual(2); expect(failures[0].message).toEqual(message); expect(failures[1].message).toEqual(message); @@ -532,8 +534,10 @@ describe('Enforce Module Boundaries (eslint)', () => { expect(failures.length).toEqual(2); // TODO: Add project dependency path to message - const message = - 'A project tagged with "public" can not depend on libs tagged with "private"'; + const message = `A project tagged with "public" can not depend on libs tagged with "private" + +Violation detected in: +- dependsOnPrivateName -> privateName`; expect(failures[0].message).toEqual(message); expect(failures[1].message).toEqual(message); }); 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 0c80bc96a3636..b7e6eac91ae24 100644 --- a/packages/eslint-plugin-nx/src/rules/enforce-module-boundaries.ts +++ b/packages/eslint-plugin-nx/src/rules/enforce-module-boundaries.ts @@ -6,8 +6,7 @@ import { findSourceProject, getSourceFilePath, hasBuildExecutor, - hasNoneOfTheseTags, - hasAnyOfTheseTags, + findDependenciesWithTags, isAbsoluteImportIntoAnotherProject, isRelativeImportIntoAnotherProject, mapProjectGraphFiles, @@ -18,6 +17,7 @@ import { isDirectDependency, isTerminalRun, stringifyTags, + hasNoneOfTheseTags, } from '@nrwl/workspace/src/utils/runtime-lint-utils'; import { AST_NODE_TYPES, @@ -109,7 +109,7 @@ export default createESLintRule({ bannedExternalImportsViolation: `A project tagged with "{{sourceTag}}" is not allowed to import the "{{package}}" package`, noTransitiveDependencies: `Transitive dependencies are not allowed. Only packages defined in the "package.json" can be imported`, onlyTagsConstraintViolation: `A project tagged with "{{sourceTag}}" can only depend on libs tagged with {{tags}}`, - notTagsConstraintViolation: `A project tagged with "{{sourceTag}}" can not depend on libs tagged with {{tags}}`, + notTagsConstraintViolation: `A project tagged with "{{sourceTag}}" can not depend on libs tagged with {{tags}}\n\nViolation detected in:\n{{projects}}`, }, }, defaultOptions: [ @@ -309,7 +309,7 @@ export default createESLintRule({ // spacer text used for indirect dependencies when printing one line per file. // without this, we can end up with a very long line that does not display well in the terminal. - const spacer = '\n '; + const spacer = ' '; context.report({ node, @@ -324,7 +324,9 @@ export default createESLintRule({ filePaths: circularFilePath .map((files) => files.length > 1 - ? `[${spacer}${files.join(`,${spacer}`)}\n ]` + ? `[${files + .map((f) => `\n${spacer}${spacer}${f}`) + .join(',')}\n${spacer}]` : files[0] ) .reduce( @@ -404,6 +406,8 @@ export default createESLintRule({ for (let constraint of constraints) { if ( + constraint.onlyDependOnLibsWithTags && + constraint.onlyDependOnLibsWithTags.length && hasNoneOfTheseTags( targetProject, constraint.onlyDependOnLibsWithTags @@ -420,21 +424,26 @@ export default createESLintRule({ return; } if ( - hasAnyOfTheseTags( - projectGraph, - targetProject.name, - constraint.notDependOnLibsWithTags - ) + constraint.notDependOnLibsWithTags && + constraint.notDependOnLibsWithTags.length ) { - context.report({ - node, - messageId: 'notTagsConstraintViolation', - data: { - sourceTag: constraint.sourceTag, - tags: stringifyTags(constraint.notDependOnLibsWithTags), - }, - }); - return; + const projects = findDependenciesWithTags( + targetProject, + constraint.notDependOnLibsWithTags, + projectGraph + ); + if (projects.length > 0) { + context.report({ + node, + messageId: 'notTagsConstraintViolation', + data: { + sourceTag: constraint.sourceTag, + tags: stringifyTags(constraint.notDependOnLibsWithTags), + projects: `- ${projects.map((p) => p.name).join(' -> ')}`, + }, + }); + return; + } } } } diff --git a/packages/workspace/src/tslint/nxEnforceModuleBoundariesRule.ts b/packages/workspace/src/tslint/nxEnforceModuleBoundariesRule.ts index 967d7964fa9fd..c91f28aa7da0e 100644 --- a/packages/workspace/src/tslint/nxEnforceModuleBoundariesRule.ts +++ b/packages/workspace/src/tslint/nxEnforceModuleBoundariesRule.ts @@ -11,13 +11,15 @@ import { findSourceProject, getSourceFilePath, hasBuildExecutor, - hasNoneOfTheseTags, + findDependenciesWithTags, isAbsoluteImportIntoAnotherProject, isRelativeImportIntoAnotherProject, MappedProjectGraph, mapProjectGraphFiles, matchImportWithWildcard, onlyLoadChildren, + stringifyTags, + hasNoneOfTheseTags, } from '../utils/runtime-lint-utils'; import { normalize } from 'path'; import { readNxJson } from '../core/file-utils'; @@ -272,18 +274,38 @@ class EnforceModuleBoundariesWalker extends Lint.RuleWalker { for (let constraint of constraints) { if ( - hasNoneOfTheseTags( - targetProject, - constraint.onlyDependOnLibsWithTags || [] - ) + constraint.onlyDependOnLibsWithTags && + hasNoneOfTheseTags(targetProject, constraint.onlyDependOnLibsWithTags) ) { - const tags = constraint.onlyDependOnLibsWithTags - .map((s) => `"${s}"`) - .join(', '); - const error = `A project tagged with "${constraint.sourceTag}" can only depend on libs tagged with ${tags}`; + const error = `A project tagged with "${ + constraint.sourceTag + }" can only depend on libs tagged with ${stringifyTags( + constraint.onlyDependOnLibsWithTags + )}`; this.addFailureAt(node.getStart(), node.getWidth(), error); return; } + if ( + constraint.notDependOnLibsWithTags && + constraint.notDependOnLibsWithTags.length + ) { + const projects = findDependenciesWithTags( + targetProject, + constraint.notDependOnLibsWithTags, + this.projectGraph + ); + if (projects.length > 0) { + const error = `A project tagged with "${ + constraint.sourceTag + }" can not depend on libs tagged with ${stringifyTags( + constraint.notDependOnLibsWithTags + )}\n\nViolation detected in:\n${projects + .map((p) => p.name) + .join(' -> ')}`; + this.addFailureAt(node.getStart(), node.getWidth(), error); + return; + } + } } } diff --git a/packages/workspace/src/utils/runtime-lint-utils.ts b/packages/workspace/src/utils/runtime-lint-utils.ts index ed601130071c4..ddd70e34caf02 100644 --- a/packages/workspace/src/utils/runtime-lint-utils.ts +++ b/packages/workspace/src/utils/runtime-lint-utils.ts @@ -35,44 +35,52 @@ export function stringifyTags(tags: string[]): string { } export function hasNoneOfTheseTags( - proj: ProjectGraphProjectNode, - tags?: string[] + proj: ProjectGraphProjectNode, + tags: string[] ): boolean { - if (!tags) { - return false; - } return tags.filter((tag) => hasTag(proj, tag)).length === 0; } -export function hasAnyOfTheseTags( +/** + * Check if any of the given tags is included in the project + * @param proj ProjectGraphProjectNode + * @param tags + * @returns + */ +export function findDependenciesWithTags( + proj: ProjectGraphProjectNode, + tags: string[], graph: ProjectGraph, - projectName: string, - tags?: string[], - visited?: string[] -): boolean { - if (!tags) { - return false; + foundPath: Array = [], + visited: string[] = [] +): Array { + if (!hasNoneOfTheseTags(proj, tags)) { + return [...foundPath, proj]; } - const found = - tags.filter((tag) => hasTag(graph.nodes[projectName], tag)).length !== 0; - - if (found) { - return true; + if (!graph.dependencies[proj.name]) { + return foundPath; } - visited = visited ?? []; - - for (let d of graph.dependencies[projectName] || []) { + for (let d of graph.dependencies[proj.name]) { if (visited.indexOf(d.target) > -1) { continue; } visited.push(d.target); - if (hasAnyOfTheseTags(graph, d.target, tags, visited)) { - return true; + if (graph.nodes[d.target]) { + const tempPath = [...foundPath, proj]; + const newPath = findDependenciesWithTags( + graph.nodes[d.target], + tags, + graph, + tempPath, + visited + ); + if (newPath !== tempPath) { + return newPath; + } } } - - return false; + return foundPath; } function hasTag(proj: ProjectGraphProjectNode, tag: string) { From 92ae03c188da1e36ccb58b7129094dfab914763a Mon Sep 17 00:00:00 2001 From: Miroslav Jonas Date: Mon, 7 Mar 2022 23:14:45 +0100 Subject: [PATCH 8/9] feat(linter): use graph utils to calculate dependencies with tags --- .../rules/enforce-module-boundaries.spec.ts | 78 +++++++++++++++++-- .../src/rules/enforce-module-boundaries.ts | 11 ++- .../tslint/nxEnforceModuleBoundariesRule.ts | 13 ++-- packages/workspace/src/utils/graph-utils.ts | 31 ++++++-- .../workspace/src/utils/runtime-lint-utils.ts | 48 ++++-------- 5 files changed, 127 insertions(+), 54 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 2eed692ca377c..4624fb50e59fb 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 @@ -36,6 +36,7 @@ const tsconfig = { '@mycompany/mylibName': ['libs/mylibName/src/index.ts'], '@mycompany/public': ['libs/public/src/index.ts'], '@mycompany/dependsOnPrivate': ['libs/dependsOnPrivate/src/index.ts'], + '@mycompany/dependsOnPrivate2': ['libs/dependsOnPrivate2/src/index.ts'], '@mycompany/private': ['libs/private/src/index.ts'], '@mycompany/anotherlibName': ['libs/anotherlibName/src/index.ts'], '@mycompany/badcirclelib': ['libs/badcirclelib/src/index.ts'], @@ -83,6 +84,7 @@ const fileSys = { './libs/nonBuildableLib/src/main.ts': '', './libs/public/src/index.ts': '', './libs/dependsOnPrivate/src/index.ts': '', + './libs/dependsOnPrivate2/src/index.ts': '', './libs/private/src/index.ts': '', './tsconfig.base.json': JSON.stringify(tsconfig), './package.json': JSON.stringify(packageJson), @@ -279,6 +281,21 @@ describe('Enforce Module Boundaries (eslint)', () => { ], }, }, + dependsOnPrivateName2: { + name: 'dependsOnPrivateName2', + type: ProjectType.lib, + data: { + root: 'libs/dependsOnPrivate2', + tags: [], + implicitDependencies: [], + architect: {}, + files: [ + createFile(`libs/dependsOnPrivate2/src/index.ts`, [ + 'privateName', + ]), + ], + }, + }, privateName: { name: 'privateName', type: ProjectType.lib, @@ -348,6 +365,13 @@ describe('Enforce Module Boundaries (eslint)', () => { type: DependencyType.static, }, ], + dependsOnPrivateName2: [ + { + source: 'dependsOnPrivateName2', + target: 'privateName', + type: DependencyType.static, + }, + ], }, }; @@ -509,7 +533,19 @@ describe('Enforce Module Boundaries (eslint)', () => { import '@mycompany/private'; import('@mycompany/private'); `, - graph + { + ...graph, + dependencies: { + ...graph.dependencies, + publicName: [ + { + source: 'publicName', + target: 'privateName', + type: DependencyType.static, + }, + ], + }, + } ); const message = `A project tagged with "public" can not depend on libs tagged with "private" @@ -527,19 +563,45 @@ Violation detected in: `${process.cwd()}/proj/libs/public/src/index.ts`, ` import '@mycompany/dependsOnPrivate'; - import('@mycompany/dependsOnPrivate'); + import '@mycompany/dependsOnPrivate2'; + import '@mycompany/private'; `, - graph + { + ...graph, + dependencies: { + ...graph.dependencies, + publicName: [ + { + source: 'publicName', + target: 'dependsOnPrivateName', + type: DependencyType.static, + }, + { + source: 'publicName', + target: 'dependsOnPrivateName2', + type: DependencyType.static, + }, + { + source: 'publicName', + target: 'privateName', + type: DependencyType.static, + }, + ], + }, + } ); - expect(failures.length).toEqual(2); + expect(failures.length).toEqual(3); // TODO: Add project dependency path to message - const message = `A project tagged with "public" can not depend on libs tagged with "private" + const message = ( + prefix + ) => `A project tagged with "public" can not depend on libs tagged with "private" Violation detected in: -- dependsOnPrivateName -> privateName`; - expect(failures[0].message).toEqual(message); - expect(failures[1].message).toEqual(message); +- ${prefix}privateName`; + expect(failures[0].message).toEqual(message('dependsOnPrivateName -> ')); + expect(failures[1].message).toEqual(message('dependsOnPrivateName2 -> ')); + expect(failures[2].message).toEqual(message('')); }); it('should error when the source library is untagged', () => { 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 b7e6eac91ae24..7cd40c65e8525 100644 --- a/packages/eslint-plugin-nx/src/rules/enforce-module-boundaries.ts +++ b/packages/eslint-plugin-nx/src/rules/enforce-module-boundaries.ts @@ -427,19 +427,24 @@ export default createESLintRule({ constraint.notDependOnLibsWithTags && constraint.notDependOnLibsWithTags.length ) { - const projects = findDependenciesWithTags( + const projectPaths = findDependenciesWithTags( targetProject, constraint.notDependOnLibsWithTags, projectGraph ); - if (projects.length > 0) { + if (projectPaths.length > 0) { context.report({ node, messageId: 'notTagsConstraintViolation', data: { sourceTag: constraint.sourceTag, tags: stringifyTags(constraint.notDependOnLibsWithTags), - projects: `- ${projects.map((p) => p.name).join(' -> ')}`, + projects: projectPaths + .map( + (projectPath) => + `- ${projectPath.map((p) => p.name).join(' -> ')}` + ) + .join('\n'), }, }); return; diff --git a/packages/workspace/src/tslint/nxEnforceModuleBoundariesRule.ts b/packages/workspace/src/tslint/nxEnforceModuleBoundariesRule.ts index c91f28aa7da0e..97e9805267897 100644 --- a/packages/workspace/src/tslint/nxEnforceModuleBoundariesRule.ts +++ b/packages/workspace/src/tslint/nxEnforceModuleBoundariesRule.ts @@ -289,19 +289,22 @@ class EnforceModuleBoundariesWalker extends Lint.RuleWalker { constraint.notDependOnLibsWithTags && constraint.notDependOnLibsWithTags.length ) { - const projects = findDependenciesWithTags( + const projectPaths = findDependenciesWithTags( targetProject, constraint.notDependOnLibsWithTags, this.projectGraph ); - if (projects.length > 0) { + if (projectPaths.length > 0) { const error = `A project tagged with "${ constraint.sourceTag }" can not depend on libs tagged with ${stringifyTags( constraint.notDependOnLibsWithTags - )}\n\nViolation detected in:\n${projects - .map((p) => p.name) - .join(' -> ')}`; + )}\n\nViolation detected in:\n${projectPaths + .map( + (projectPath) => + `- ${projectPath.map((p) => p.name).join(' -> ')}` + ) + .join('\n')}`; this.addFailureAt(node.getStart(), node.getWidth(), error); return; } diff --git a/packages/workspace/src/utils/graph-utils.ts b/packages/workspace/src/utils/graph-utils.ts index dafc137b31906..92aa4b191dc33 100644 --- a/packages/workspace/src/utils/graph-utils.ts +++ b/packages/workspace/src/utils/graph-utils.ts @@ -29,9 +29,9 @@ function buildMatrix(graph: ProjectGraph) { }; }, {}); - nodes.forEach((v, i) => { - adjList[nodes[i]] = []; - matrix[nodes[i]] = { ...initMatrixValues }; + nodes.forEach((v) => { + adjList[v] = []; + matrix[v] = { ...initMatrixValues }; }); for (let proj in dependencies) { @@ -42,7 +42,7 @@ function buildMatrix(graph: ProjectGraph) { } } - const traverse = (s, v) => { + const traverse = (s: string, v: string) => { matrix[s][v] = true; for (let adj of adjList[v]) { @@ -52,8 +52,8 @@ function buildMatrix(graph: ProjectGraph) { } }; - nodes.forEach((v, i) => { - traverse(nodes[i], nodes[i]); + nodes.forEach((v) => { + traverse(v, v); }); return { @@ -106,11 +106,28 @@ export function getPath( } } +export function pathExists( + graph: ProjectGraph, + sourceProjectName: string, + targetProjectName: string +): boolean { + if (sourceProjectName === targetProjectName) return true; + + if (reach.graph !== graph) { + const { matrix, adjList } = buildMatrix(graph); + reach.graph = graph; + reach.matrix = matrix; + reach.adjList = adjList; + } + + return reach.matrix[sourceProjectName][targetProjectName]; +} + export function checkCircularPath( graph: ProjectGraph, sourceProject: ProjectGraphProjectNode, targetProject: ProjectGraphProjectNode -): Array { +): ProjectGraphProjectNode[] { if (!graph.nodes[targetProject.name]) return []; return getPath(graph, targetProject.name, sourceProject.name); diff --git a/packages/workspace/src/utils/runtime-lint-utils.ts b/packages/workspace/src/utils/runtime-lint-utils.ts index ddd70e34caf02..f529afb4f9274 100644 --- a/packages/workspace/src/utils/runtime-lint-utils.ts +++ b/packages/workspace/src/utils/runtime-lint-utils.ts @@ -12,6 +12,7 @@ import { import { TargetProjectLocator } from '../core/target-project-locator'; import { join } from 'path'; import { appRootPath } from './app-root'; +import { getPath, pathExists } from './graph-utils'; export type MappedProjectGraphNode = ProjectGraphProjectNode & { data: { @@ -48,39 +49,24 @@ export function hasNoneOfTheseTags( * @returns */ export function findDependenciesWithTags( - proj: ProjectGraphProjectNode, + targetProject: ProjectGraphProjectNode, tags: string[], - graph: ProjectGraph, - foundPath: Array = [], - visited: string[] = [] -): Array { - if (!hasNoneOfTheseTags(proj, tags)) { - return [...foundPath, proj]; - } - if (!graph.dependencies[proj.name]) { - return foundPath; - } + graph: ProjectGraph +): ProjectGraphProjectNode[][] { + // find all reachable projects that have one of the tags and + // are reacheable from the targetProject (including self) + const allReachableProjects = Object.keys(graph.nodes).filter( + (projectName) => + pathExists(graph, targetProject.name, projectName) && + tags.some((tag) => hasTag(graph.nodes[projectName], tag)) + ); - for (let d of graph.dependencies[proj.name]) { - if (visited.indexOf(d.target) > -1) { - continue; - } - visited.push(d.target); - if (graph.nodes[d.target]) { - const tempPath = [...foundPath, proj]; - const newPath = findDependenciesWithTags( - graph.nodes[d.target], - tags, - graph, - tempPath, - visited - ); - if (newPath !== tempPath) { - return newPath; - } - } - } - return foundPath; + // return path from targetProject to reachable project + return allReachableProjects.map((project) => + targetProject.name === project + ? [targetProject] + : getPath(graph, targetProject.name, project) + ); } function hasTag(proj: ProjectGraphProjectNode, tag: string) { From 9fa9ec56f1c0e96d36a92ea5eb19be4c397b32ad Mon Sep 17 00:00:00 2001 From: Miroslav Jonas Date: Tue, 8 Mar 2022 11:24:00 +0100 Subject: [PATCH 9/9] docs(linter): add info on notDependOnLibsWithTags --- docs/shared/monorepo-tags.md | 43 ++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/docs/shared/monorepo-tags.md b/docs/shared/monorepo-tags.md index beb0e3e84acd3..5c563f2b709b9 100644 --- a/docs/shared/monorepo-tags.md +++ b/docs/shared/monorepo-tags.md @@ -155,6 +155,49 @@ If you try to violate the constrains, you will get an error: A project tagged with "scope:admin" can only depend on projects tagged with "scoped:shared" or "scope:admin". ``` +### Explicitly banning tags + +Specifying which tags is project allowed to depend on can sometimes lead to a long list of possible options: + +```jsonc +{ + "sourceTag": "scope:client", + // we actually want to say it cannot depend on `scope:admin` + "onlyDependOnLibsWithTags": [ + "scope:shared", + "scope:utils", + "scope:core", + "scope:client" + ] +} +``` + +The property `notDependOnLibsWithTags` is used to invert this condition by explicitly specifying which tag(s) it cannot depend on: + +```jsonc +{ + "sourceTag": "scope:client", + // we accept any tag except for `scope:admin` + "notDependOnLibsWithTags": ["scope:admin"] +} +``` + +> In contrast to `onlyDependOnLibsWithTags`, the `notDependOnLibsWithTags` will also follow down the _entire dependency tree_ to make sure there are no sub-dependencies that violate this rule. You can also use a combination of these two rules to restrict certain types of projects to be imported: + +```jsonc +{ + "sourceTag": "type:react", + "onlyDependOnLibsWithTags": [ + "type:react", + "type:utils", + "type:animation", + "type:model" + ], + // make sure no `angular` code ends up being referenced by react projects + "notDependOnLibsWithTags": ["type:angular"] +} +``` + ### Exceptions The `"allow": []` are the list of imports that won't fail linting.