From 3a4f132ddb0a81270928f84f5ecac56360ff2794 Mon Sep 17 00:00:00 2001 From: Rainer Hahnekamp Date: Sat, 19 Oct 2024 14:24:50 +0200 Subject: [PATCH] feat(eslint): supersede `deep-import` rules with `encapsulation` rule The renaming is necessary since the concept of a deep import does not exist in barrel-less modules. The configs for ESLint have been changed. To use the old `deep-import` rule, you need to switch to the config `barrelModulesOnly`. --- packages/core/src/index.ts | 2 +- ...rts.ts => has-encapsulation-violations.ts} | 17 ++- .../deep-import-with-exclude-root.spec.ts | 82 ---------- ...c.ts => encapsulation-barrel-less.spec.ts} | 20 +-- .../encapsulation-with-exclude-root.spec.ts | 97 ++++++++++++ ...s => has-encapsulation-violations.spec.ts} | 8 +- packages/core/src/lib/cli/verify.ts | 11 +- .../core/src/lib/eslint/deep-import.spec.ts | 77 ---------- packages/core/src/lib/eslint/deep-import.ts | 50 ------ .../src/lib/eslint/{ => tests}/eslint.spec.ts | 18 +-- .../violates-dependency-rule.spec.ts | 14 +- .../tests/violates-encapsulation-rule.spec.ts | 143 ++++++++++++++++++ .../lib/eslint/violates-encapsulation-rule.ts | 68 +++++++++ packages/eslint-plugin/src/index.ts | 6 +- packages/eslint-plugin/src/lib/configs/all.ts | 18 ++- .../eslint-plugin/src/lib/configs/legacy.ts | 12 +- .../src/lib/rules/deep-import.ts | 5 +- .../src/lib/rules/encapsulation.ts | 22 +++ packages/eslint-plugin/src/lib/rules/index.ts | 2 + .../src/lib/rules/tests/deep-import.spec.ts | 4 +- .../src/lib/rules/tests/encapsulation.spec.ts | 85 +++++++++++ test-projects/angular-i/integration-test.sh | 10 +- .../tests/expected/encapsulation-lint.json} | 4 +- test-projects/angular-iv/integration-test.sh | 10 +- .../tests/expected/encapsulation-lint.json} | 4 +- 25 files changed, 515 insertions(+), 274 deletions(-) rename packages/core/src/lib/checks/{check-for-deep-imports.ts => has-encapsulation-violations.ts} (80%) delete mode 100644 packages/core/src/lib/checks/tests/deep-import-with-exclude-root.spec.ts rename packages/core/src/lib/checks/tests/{check-for-deep-imports.barrel-less.spec.ts => encapsulation-barrel-less.spec.ts} (89%) create mode 100644 packages/core/src/lib/checks/tests/encapsulation-with-exclude-root.spec.ts rename packages/core/src/lib/checks/tests/{check-for-deep-imports.spec.ts => has-encapsulation-violations.spec.ts} (84%) delete mode 100644 packages/core/src/lib/eslint/deep-import.spec.ts delete mode 100644 packages/core/src/lib/eslint/deep-import.ts rename packages/core/src/lib/eslint/{ => tests}/eslint.spec.ts (69%) rename packages/core/src/lib/eslint/{ => tests}/violates-dependency-rule.spec.ts (89%) create mode 100644 packages/core/src/lib/eslint/tests/violates-encapsulation-rule.spec.ts create mode 100644 packages/core/src/lib/eslint/violates-encapsulation-rule.ts create mode 100644 packages/eslint-plugin/src/lib/rules/encapsulation.ts create mode 100644 packages/eslint-plugin/src/lib/rules/tests/encapsulation.spec.ts rename test-projects/{angular-iv/tests/expected/deep-import-lint.json => angular-i/tests/expected/encapsulation-lint.json} (90%) rename test-projects/{angular-i/tests/expected/deep-import-lint.json => angular-iv/tests/expected/encapsulation-lint.json} (90%) diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 52f60fb..532c556 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -1,4 +1,4 @@ -export { hasDeepImport } from './lib/eslint/deep-import'; +export { violatesEncapsulationRule } from './lib/eslint/violates-encapsulation-rule'; export { violatesDependencyRule } from './lib/eslint/violates-dependency-rule'; export { anyTag } from './lib/checks/any-tag'; export { sameTag } from './lib/checks/same-tag'; diff --git a/packages/core/src/lib/checks/check-for-deep-imports.ts b/packages/core/src/lib/checks/has-encapsulation-violations.ts similarity index 80% rename from packages/core/src/lib/checks/check-for-deep-imports.ts rename to packages/core/src/lib/checks/has-encapsulation-violations.ts index a7b69e6..1a0d834 100644 --- a/packages/core/src/lib/checks/check-for-deep-imports.ts +++ b/packages/core/src/lib/checks/has-encapsulation-violations.ts @@ -4,16 +4,18 @@ import { ProjectInfo } from '../main/init'; import { FileInfo } from '../modules/file.info'; /** - * verifies if an existing file has deep imports which are forbidden. + * verifies if an existing file has imports which break + * the other module's encapsulation. + * * Unresolvable imports are skipped. * * It is up to the caller to decide. */ -export function checkForDeepImports( +export function hasEncapsulationViolations( fsPath: FsPath, { rootDir, config, getFileInfo }: ProjectInfo, -): string[] { - const deepImports: string[] = []; +): Record { + const encapsulationViolations: Record = {}; const assignedFileInfo = getFileInfo(fsPath); for (const importedFileInfo of assignedFileInfo.imports) { @@ -25,13 +27,12 @@ export function checkForDeepImports( ) { // 👍 all good } else { - deepImports.push( - assignedFileInfo.getRawImportForImportedFileInfo(importedFileInfo.path), - ); + const rawImport = assignedFileInfo.getRawImportForImportedFileInfo(importedFileInfo.path); + encapsulationViolations[rawImport] = importedFileInfo; } } - return deepImports; + return encapsulationViolations; } function accessesExposedFileForBarrelLessModules(fileInfo: FileInfo, enableBarrelLess: boolean) { diff --git a/packages/core/src/lib/checks/tests/deep-import-with-exclude-root.spec.ts b/packages/core/src/lib/checks/tests/deep-import-with-exclude-root.spec.ts deleted file mode 100644 index 1328207..0000000 --- a/packages/core/src/lib/checks/tests/deep-import-with-exclude-root.spec.ts +++ /dev/null @@ -1,82 +0,0 @@ -import { describe, expect, it } from 'vitest'; -import { anyTag, hasDeepImport } from "@softarc/sheriff-core"; -import getFs from '../../fs/getFs'; -import { toFsPath } from '../../file-info/fs-path'; -import { testInit } from "../../test/test-init"; -import { sheriffConfig } from "../../test/project-configurator"; -import { tsConfig } from "../../test/fixtures/ts-config"; - -describe('deep imports and excludeRoot config property', () => { - for (const { excludeRoot, enableBarrelLess, hasDeepImport } of [ - { excludeRoot: true, enableBarrelLess: false, hasDeepImport: false }, - { excludeRoot: false, enableBarrelLess: false, hasDeepImport: true }, - { excludeRoot: true, enableBarrelLess: true, hasDeepImport: false }, - { excludeRoot: false, enableBarrelLess: true, hasDeepImport: false }, - ]) { - it(`should be ${ - hasDeepImport ? 'valid' : 'invalid' - } for rootExcluded: ${excludeRoot} and barrel-less: ${enableBarrelLess}`, () => { - testInit('src/main.ts', { - 'tsconfig.json': tsConfig(), - 'sheriff.config.ts': sheriffConfig({ - modules: { 'src/shared': 'shared' }, - depRules: { '*': anyTag }, - excludeRoot, - enableBarrelLess, - }), - src: { - 'main.ts': '', - 'router.ts': ['./shared/dialog'], // always deep import - 'config.ts': ['./shared/index'], - shared: { - 'get.ts': ['../config', '../holidays/holidays-component'], // depends on `excludeRoot` - 'dialog.ts': '', - 'index.ts': '', - }, - holidays: { - 'holidays-component.ts': ['../config'], // always valid}, - }, - }, - }); - - assertDeepImport('/project/src/router.ts', './shared/dialog'); - assertDeepImport( - '/project/src/holidays/holidays-component.ts', - '../config', - false, - ); - - assertDeepImport( - '/project/src/shared/get.ts', - '../config', - hasDeepImport, - ); - - assertDeepImport( - '/project/src/shared/get.ts', - '../holidays/holidays-component', - hasDeepImport, - ); - }); - } -}); - -function assertDeepImport( - filename: string, - importCommand: string, - isDeepImport = true, -) { - expect( - hasDeepImport( - filename, - importCommand, - true, - getFs().readFile(toFsPath(filename)), - ), - `deep import in ${filename} from ${importCommand} should be ${isDeepImport}`, - ).toBe( - isDeepImport - ? "Deep import is not allowed. Use the module's index.ts or path." - : '', - ); -} diff --git a/packages/core/src/lib/checks/tests/check-for-deep-imports.barrel-less.spec.ts b/packages/core/src/lib/checks/tests/encapsulation-barrel-less.spec.ts similarity index 89% rename from packages/core/src/lib/checks/tests/check-for-deep-imports.barrel-less.spec.ts rename to packages/core/src/lib/checks/tests/encapsulation-barrel-less.spec.ts index eb22a32..e1d0115 100644 --- a/packages/core/src/lib/checks/tests/check-for-deep-imports.barrel-less.spec.ts +++ b/packages/core/src/lib/checks/tests/encapsulation-barrel-less.spec.ts @@ -2,7 +2,7 @@ import { describe, it, expect } from 'vitest'; import { testInit } from '../../test/test-init'; import { tsConfig } from '../../test/fixtures/ts-config'; import { FileTree, sheriffConfig } from '../../test/project-configurator'; -import { checkForDeepImports } from '../check-for-deep-imports'; +import { hasEncapsulationViolations } from '../has-encapsulation-violations'; import { UserSheriffConfig } from '../../config/user-sheriff-config'; import { traverseFileInfo } from '../../modules/traverse-file-info'; @@ -25,7 +25,7 @@ describe('barrel-less', () => { internal: { 'hidden.service.ts': [] }, }, }) - .hasDeepImports({ + .hasEncapsulationViolations({ 'feature/customers.component.ts': [ '../data/internal/hidden.service.ts', ], @@ -52,7 +52,7 @@ describe('barrel-less', () => { internal: { services: { 'hidden.service.ts': [] } }, }, }) - .hasDeepImports({ + .hasEncapsulationViolations({ 'feature/customers.component.ts': [ '../data/internal/services/hidden.service.ts', ], @@ -82,7 +82,7 @@ describe('barrel-less', () => { internal: { 'hidden.service.ts': [] }, }, }) - .hasDeepImports({ + .hasEncapsulationViolations({ 'feature/components/customers-sub.component.ts': [ '../../data/internal/hidden.service.ts', ], @@ -103,7 +103,7 @@ describe('barrel-less', () => { internal: { 'customer.service.ts': ['../private/hidden.service.ts'] }, }, }) - .hasDeepImports({ + .hasEncapsulationViolations({ 'feature/customers.component.ts': ['../data/private/hidden.service.ts'], }); }); @@ -121,7 +121,7 @@ describe('barrel-less', () => { internal: { 'hidden.service.ts': [] }, }, }) - .hasDeepImports({ + .hasEncapsulationViolations({ 'feature/customers.component.ts': ['../data/open.service.ts'], }); }); @@ -131,7 +131,7 @@ function assertProject(config: Partial = {}) { return { withCustomerRoute(customerFileTree: FileTree) { return { - hasDeepImports(deepImports: Record = {}) { + hasEncapsulationViolations(encapsulationViolations: Record = {}) { const projectInfo = testInit('src/main.ts', { 'tsconfig.json': tsConfig(), 'sheriff.config.ts': sheriffConfig({ @@ -167,10 +167,12 @@ function assertProject(config: Partial = {}) { '', ); - const expectedDeepImports = deepImports[pathToLookup] || []; + const expectedDeepImports = encapsulationViolations[pathToLookup] || []; + const violations = hasEncapsulationViolations(fileInfo.path, projectInfo); + const violatedImports = Object.keys(violations); expect .soft( - checkForDeepImports(fileInfo.path, projectInfo), + violatedImports, `deep imports check failed for ${fileInfo.path}`, ) .toEqual(expectedDeepImports); diff --git a/packages/core/src/lib/checks/tests/encapsulation-with-exclude-root.spec.ts b/packages/core/src/lib/checks/tests/encapsulation-with-exclude-root.spec.ts new file mode 100644 index 0000000..2dbc5ad --- /dev/null +++ b/packages/core/src/lib/checks/tests/encapsulation-with-exclude-root.spec.ts @@ -0,0 +1,97 @@ +import { describe, expect, it } from 'vitest'; +import { anyTag, violatesEncapsulationRule } from '@softarc/sheriff-core'; +import getFs from '../../fs/getFs'; +import { toFsPath } from '../../file-info/fs-path'; +import { testInit } from '../../test/test-init'; +import { sheriffConfig } from '../../test/project-configurator'; +import { tsConfig } from '../../test/fixtures/ts-config'; + +describe('encapsulation and excludeRoot config property', () => { + for (const { excludeRoot, enableBarrelLess, hasViolation } of [ + { excludeRoot: true, enableBarrelLess: false, hasViolation: false }, + { excludeRoot: false, enableBarrelLess: false, hasViolation: true }, + { excludeRoot: true, enableBarrelLess: true, hasViolation: false }, + { excludeRoot: false, enableBarrelLess: true, hasViolation: false }, + ]) { + it(`should be ${ + hasViolation ? 'valid' : 'invalid' + } for rootExcluded: ${excludeRoot} and barrel-less: ${enableBarrelLess}`, () => { + testInit('src/main.ts', { + 'tsconfig.json': tsConfig(), + 'sheriff.config.ts': sheriffConfig({ + modules: { 'src/shared': 'shared' }, + depRules: { '*': anyTag }, + excludeRoot, + enableBarrelLess, + }), + src: { + 'main.ts': '', + 'router.ts': ['./shared/dialog'], // always violation + 'config.ts': ['./shared/index'], + shared: { + 'get.ts': ['../config', '../holidays/holidays-component'], // depends on `excludeRoot` + 'dialog.ts': '', + 'index.ts': '', + }, + holidays: { + 'holidays-component.ts': ['../config'], // always valid}, + }, + }, + }); + + assertViolation('/project/src/router.ts', './shared/dialog', false, true); + + assertViolation( + '/project/src/holidays/holidays-component.ts', + '../config', + true, + false, + ); + + assertViolation( + '/project/src/shared/get.ts', + '../config', + true, + hasViolation, + ); + + assertViolation( + '/project/src/shared/get.ts', + '../holidays/holidays-component', + true, + hasViolation, + ); + }); + } +}); + +function assertViolation( + filename: string, + importCommand: string, + isImportToBarrelLess: boolean, + hasViolation: boolean, +) { + expect( + violatesEncapsulationRule( + filename, + importCommand, + true, + getFs().readFile(toFsPath(filename)), + false, + ), + `import in ${filename} to ${importCommand} should violate encapsulation: ${hasViolation}`, + ).toBe(getMessage(hasViolation, isImportToBarrelLess, importCommand)); +} + +function getMessage( + hasViolation: boolean, + isImportToBarrelLess: boolean, + importCommand: string, +) { + if (!hasViolation) { + return ''; + } + return isImportToBarrelLess + ? `'${importCommand}' cannot be imported. It is encapsulated.` + : `'${importCommand}' is a deep import from a barrel module. Use the module's barrel file (index.ts) instead.`; +} diff --git a/packages/core/src/lib/checks/tests/check-for-deep-imports.spec.ts b/packages/core/src/lib/checks/tests/has-encapsulation-violations.spec.ts similarity index 84% rename from packages/core/src/lib/checks/tests/check-for-deep-imports.spec.ts rename to packages/core/src/lib/checks/tests/has-encapsulation-violations.spec.ts index a11a77c..32d453c 100644 --- a/packages/core/src/lib/checks/tests/check-for-deep-imports.spec.ts +++ b/packages/core/src/lib/checks/tests/has-encapsulation-violations.spec.ts @@ -1,10 +1,10 @@ import { describe, expect, it } from 'vitest'; -import { checkForDeepImports } from '../check-for-deep-imports'; +import { hasEncapsulationViolations } from '../has-encapsulation-violations'; import { toFsPath } from '../../file-info/fs-path'; import { testInit } from '../../test/test-init'; import { tsConfig } from '../../test/fixtures/ts-config'; -describe('check deep imports', () => { +describe('check encapsulation', () => { it('should check for deep imports', () => { const projectInfo = testInit('src/main.ts', { 'tsconfig.json': tsConfig(), @@ -31,7 +31,7 @@ describe('check deep imports', () => { ['src/app/customers/index.ts', []], ]) { expect( - checkForDeepImports(toFsPath(`/project/${filename}`), projectInfo), + Object.keys(hasEncapsulationViolations(toFsPath(`/project/${filename}`), projectInfo)), `failed deep import test for ${filename}`, ).toEqual(deepImports); } @@ -53,7 +53,7 @@ describe('check deep imports', () => { ['src/app/app.component.ts', []], ]) { expect( - checkForDeepImports(toFsPath(`/project/${filename}`), projectInfo), + Object.keys(hasEncapsulationViolations(toFsPath(`/project/${filename}`), projectInfo)), ).toEqual(deepImports); } }); diff --git a/packages/core/src/lib/cli/verify.ts b/packages/core/src/lib/cli/verify.ts index b08115f..a076a9d 100644 --- a/packages/core/src/lib/cli/verify.ts +++ b/packages/core/src/lib/cli/verify.ts @@ -1,4 +1,4 @@ -import { checkForDeepImports } from '../checks/check-for-deep-imports'; +import { hasEncapsulationViolations } from '../checks/has-encapsulation-violations'; import { traverseFileInfo } from '../modules/traverse-file-info'; import { checkForDependencyRuleViolation } from '../checks/check-for-dependency-rule-violation'; import getFs from '../fs/getFs'; @@ -21,16 +21,17 @@ export function verify(args: string[]) { const projectInfo = getEntryFromCliOrConfig(args[0]); for (const { fileInfo } of traverseFileInfo(projectInfo.fileInfo)) { - const deepImports = checkForDeepImports(fileInfo.path, projectInfo); + const violations = Object.keys(hasEncapsulationViolations(fileInfo.path, projectInfo)); + const dependencyRuleViolations = checkForDependencyRuleViolation( fileInfo.path, projectInfo, ); - if (deepImports.length > 0 || dependencyRuleViolations.length > 0) { + if (violations.length > 0 || dependencyRuleViolations.length > 0) { hasError = true; filesCount++; - deepImportsCount += deepImports.length; + deepImportsCount += violations.length; dependencyRulesCount += dependencyRuleViolations.length; const dependencyRules = dependencyRuleViolations.map( @@ -39,7 +40,7 @@ export function verify(args: string[]) { ); validationsMap[fs.relativeTo(fs.cwd(), fileInfo.path)] = { - deepImports, + deepImports: violations, dependencyRules, }; } diff --git a/packages/core/src/lib/eslint/deep-import.spec.ts b/packages/core/src/lib/eslint/deep-import.spec.ts deleted file mode 100644 index e407282..0000000 --- a/packages/core/src/lib/eslint/deep-import.spec.ts +++ /dev/null @@ -1,77 +0,0 @@ -import { beforeAll, beforeEach, describe, expect, it } from 'vitest'; -import getFs, { useVirtualFs } from '../fs/getFs'; -import { hasDeepImport } from './deep-import'; -import { toFsPath } from '../file-info/fs-path'; -import { testInit } from '../test/test-init'; -import { tsConfig } from '../test/fixtures/ts-config'; - -describe('deep import', () => { - const assertDeepImport = ( - filename: string, - importCommand: string, - isDeepImport = true, - ) => { - expect( - hasDeepImport( - filename, - importCommand, - true, - getFs().readFile(toFsPath(filename)), - ), - `deep import in ${filename} from ${importCommand} should be ${isDeepImport}`, - ).toBe( - isDeepImport - ? "Deep import is not allowed. Use the module's index.ts or path." - : '', - ); - }; - - beforeAll(() => { - useVirtualFs(); - }); - - beforeEach(() => { - getFs().reset(); - }); - - it('should find a deep import', () => { - testInit('src/main.ts', { - 'tsconfig.json': tsConfig(), - src: { - 'main.ts': ['./app/app.component', './app/app.routes'], - app: { - 'app.routes.ts': ['./home.component', './customers/index'], - 'app.component.ts': ['./customers/customer.component'], - customers: { - 'customer.component.ts': [], - 'customer.routes.ts': ['./customer.component'], - 'index.ts': ['./customer.routes'], - }, - }, - }, - }); - - assertDeepImport( - '/project/src/app/app.component.ts', - './customers/customer.component', - ); - }); - - it('should mark an unresolvable import', () => { - testInit('src/main.ts', { - 'tsconfig.json': tsConfig(), - src: { - 'main.ts': ['./app/app.component'], - }, - }); - - expect( - hasDeepImport( - '/project/src/main.ts', - './app/app.component', - true, - getFs().readFile(toFsPath('/project/src/main.ts')), - ), - ).toBe('import ./app/app.component cannot be resolved'); - }); -}); diff --git a/packages/core/src/lib/eslint/deep-import.ts b/packages/core/src/lib/eslint/deep-import.ts deleted file mode 100644 index 38c1c79..0000000 --- a/packages/core/src/lib/eslint/deep-import.ts +++ /dev/null @@ -1,50 +0,0 @@ -import { toFsPath } from '../file-info/fs-path'; -import { init } from '../main/init'; -import { checkForDeepImports } from '../checks/check-for-deep-imports'; -import { FileInfo } from '../modules/file.info'; - -/** - * This is the adapter for the ESLint plugin - * This file needs to store the deep imports in a - * cache because ESLint requests for every import - * separately. - * - * We need both variables in order to distinguish - * if we have an existing cache. - * In case `cache` is empty we can't say if that - * is because we never or run or because there no - * deep imports. - */ -let cache: string[] = []; -let cachedFileInfo: FileInfo | undefined; - -export const hasDeepImport = ( - filename: string, - importCommand: string, - isFirstRun: boolean, - fileContent: string, -): string => { - if (isFirstRun) { - cache = []; - cachedFileInfo = undefined; - } - - if (!cachedFileInfo) { - const fsPath = toFsPath(filename); - const projectInfo = init(fsPath, { - traverse: false, - entryFileContent: fileContent, - }); - - cachedFileInfo = projectInfo.fileInfo; - cache = checkForDeepImports(fsPath, projectInfo); - } - - if (cachedFileInfo.isUnresolvableImport(importCommand)) { - return `import ${importCommand} cannot be resolved`; - } - - return cache.includes(importCommand) - ? "Deep import is not allowed. Use the module's index.ts or path." - : ''; -}; diff --git a/packages/core/src/lib/eslint/eslint.spec.ts b/packages/core/src/lib/eslint/tests/eslint.spec.ts similarity index 69% rename from packages/core/src/lib/eslint/eslint.spec.ts rename to packages/core/src/lib/eslint/tests/eslint.spec.ts index 9009733..a202cd4 100644 --- a/packages/core/src/lib/eslint/eslint.spec.ts +++ b/packages/core/src/lib/eslint/tests/eslint.spec.ts @@ -1,12 +1,12 @@ import { describe, expect, it, vitest } from 'vitest'; -import getFs from '../fs/getFs'; -import { sheriffConfig } from '../test/project-configurator'; -import { anyTag } from '../checks/any-tag'; -import { createProject } from '../test/project-creator'; -import { hasDeepImport } from './deep-import'; -import { toFsPath } from '../file-info/fs-path'; -import { violatesDependencyRule } from './violates-dependency-rule'; -import { tsConfig } from '../test/fixtures/ts-config'; +import getFs from '../../fs/getFs'; +import { sheriffConfig } from '../../test/project-configurator'; +import { anyTag } from '../../checks/any-tag'; +import { createProject } from '../../test/project-creator'; +import { toFsPath } from '../../file-info/fs-path'; +import { violatesDependencyRule } from '../violates-dependency-rule'; +import { tsConfig } from '../../test/fixtures/ts-config'; +import { violatesEncapsulationRule } from "../violates-encapsulation-rule"; describe('ESLint features', () => { it('should never read from linted file', () => { @@ -38,7 +38,7 @@ describe('ESLint features', () => { const fsPath = toFsPath('/project/src/shared/get.ts'); const fileContent = getFs().readFile(fsPath); const fileReadSpy = vitest.spyOn(fs, 'readFile'); - hasDeepImport(fsPath, '../config', true, fileContent); + violatesEncapsulationRule(fsPath, '../config', true, fileContent, false); violatesDependencyRule(fsPath, '../config', true, fileContent); const readsOnLintedFile = fileReadSpy.mock.calls.filter((params) => { diff --git a/packages/core/src/lib/eslint/violates-dependency-rule.spec.ts b/packages/core/src/lib/eslint/tests/violates-dependency-rule.spec.ts similarity index 89% rename from packages/core/src/lib/eslint/violates-dependency-rule.spec.ts rename to packages/core/src/lib/eslint/tests/violates-dependency-rule.spec.ts index 89abd25..dae81cf 100644 --- a/packages/core/src/lib/eslint/violates-dependency-rule.spec.ts +++ b/packages/core/src/lib/eslint/tests/violates-dependency-rule.spec.ts @@ -1,11 +1,11 @@ import { describe, expect, it, vitest } from 'vitest'; -import * as fileInfoGenerator from '../file-info/generate-unassigned-file-info'; -import { sheriffConfig } from '../test/project-configurator'; -import { createProject } from '../test/project-creator'; -import getFs from '../fs/getFs'; -import { toFsPath } from '../file-info/fs-path'; -import { violatesDependencyRule } from './violates-dependency-rule'; -import { tsConfig } from '../test/fixtures/ts-config'; +import * as fileInfoGenerator from '../../file-info/generate-unassigned-file-info'; +import { sheriffConfig } from '../../test/project-configurator'; +import { createProject } from '../../test/project-creator'; +import getFs from '../../fs/getFs'; +import { toFsPath } from '../../file-info/fs-path'; +import { violatesDependencyRule } from '../violates-dependency-rule'; +import { tsConfig } from '../../test/fixtures/ts-config'; import { noDependencies, sameTag } from '@softarc/sheriff-core'; describe('violates dependency rules', () => { diff --git a/packages/core/src/lib/eslint/tests/violates-encapsulation-rule.spec.ts b/packages/core/src/lib/eslint/tests/violates-encapsulation-rule.spec.ts new file mode 100644 index 0000000..2ef6ca8 --- /dev/null +++ b/packages/core/src/lib/eslint/tests/violates-encapsulation-rule.spec.ts @@ -0,0 +1,143 @@ +import { beforeAll, beforeEach, describe, expect, it } from 'vitest'; +import getFs, { useVirtualFs } from '../../fs/getFs'; +import { violatesEncapsulationRule } from '../violates-encapsulation-rule'; +import { toFsPath } from '../../file-info/fs-path'; +import { testInit } from '../../test/test-init'; +import { tsConfig } from '../../test/fixtures/ts-config'; +import { sheriffConfig } from '../../test/project-configurator'; + +describe('encapsulation', () => { + const assertViolation = ( + filename: string, + importCommand: string, + isDeepImport = true, + ) => { + expect( + violatesEncapsulationRule( + filename, + importCommand, + true, + getFs().readFile(toFsPath(filename)), + true, + ), + `deep import in ${filename} from ${importCommand} should be ${isDeepImport}`, + ).toBe( + isDeepImport + ? "Deep import is not allowed. Use the module's index.ts or path." + : '', + ); + }; + + beforeAll(() => { + useVirtualFs(); + }); + + beforeEach(() => { + getFs().reset(); + }); + + it('should find a deep import', () => { + testInit('src/main.ts', { + 'tsconfig.json': tsConfig(), + src: { + 'main.ts': ['./app/app.component', './app/app.routes'], + app: { + 'app.routes.ts': ['./home.component', './customers/index'], + 'app.component.ts': ['./customers/customer.component'], + customers: { + 'customer.component.ts': [], + 'customer.routes.ts': ['./customer.component'], + 'index.ts': ['./customer.routes'], + }, + }, + }, + }); + + assertViolation( + '/project/src/app/app.component.ts', + './customers/customer.component', + ); + }); + + it('should mark an unresolvable import', () => { + testInit('src/main.ts', { + 'tsconfig.json': tsConfig(), + src: { + 'main.ts': ['./app/app.component'], + }, + }); + + expect( + violatesEncapsulationRule( + '/project/src/main.ts', + './app/app.component', + true, + getFs().readFile(toFsPath('/project/src/main.ts')), + true, + ), + ).toBe('import ./app/app.component cannot be resolved'); + }); + + it('should return a violation to a barrel-less modules', () => { + testInit('src/app/main.ts', { + 'tsconfig.json': tsConfig(), + 'sheriff.config.ts': sheriffConfig({ + modules: { 'src/app/customers': 'customers' }, + depRules: {}, + enableBarrelLess: true, + }), + src: { + app: { + 'main.ts': ['./customers/internal/customer.component'], + customers: { + internal: { + 'customer.component.ts': [], + }, + }, + }, + }, + }); + + const message = violatesEncapsulationRule( + '/project/src/app/main.ts', + './customers/internal/customer.component', + true, + getFs().readFile(toFsPath('/project/src/app/main.ts')), + false, + ); + expect(message).toBe( + `'./customers/internal/customer.component' cannot be imported. It is encapsulated.`, + ); + }); + + it('should return a violation to a barrel modules', () => { + testInit('src/app/main.ts', { + 'tsconfig.json': tsConfig(), + 'sheriff.config.ts': sheriffConfig({ + modules: {}, + depRules: {}, + enableBarrelLess: true, + }), + src: { + app: { + 'main.ts': ['./customers/customer.component'], + customers: { + 'index.ts': [], + 'customer.component.ts': [], + }, + }, + }, + }); + + const message = violatesEncapsulationRule( + '/project/src/app/main.ts', + './customers/customer.component', + true, + getFs().readFile(toFsPath('/project/src/app/main.ts')), + false, + ); + expect(message).toBe( + `'./customers/customer.component' is a deep import from a barrel module. Use the module's barrel file (index.ts) instead.`, + ); + }); +}); diff --git a/packages/core/src/lib/eslint/violates-encapsulation-rule.ts b/packages/core/src/lib/eslint/violates-encapsulation-rule.ts new file mode 100644 index 0000000..1e46f4c --- /dev/null +++ b/packages/core/src/lib/eslint/violates-encapsulation-rule.ts @@ -0,0 +1,68 @@ +import { toFsPath } from '../file-info/fs-path'; +import { init } from '../main/init'; +import { hasEncapsulationViolations } from '../checks/has-encapsulation-violations'; +import { FileInfo } from '../modules/file.info'; + +let cache: Record = {}; +let cachedFileInfo: FileInfo | undefined; + +/** + * This is the adapter for the ESLint plugin + * + * This file needs to store the encapsulation violations + * in a cache because ESLint calls for every import + * in a file separately. + * + * We need both variables in order to distinguish + * if we have an existing cache. + * In case `cache` is empty we can't say if that + * is because we never or run or because there no + * deep imports. + * + * @param filename Name of the file + * @param importCommand Import command + * @param isFirstRun If this is the first run + * @param fileContent Content of the file + * @param isLegacyDeepImport If this is coming from the deep import rule + */ +export const violatesEncapsulationRule = ( + filename: string, + importCommand: string, + isFirstRun: boolean, + fileContent: string, + isLegacyDeepImport: boolean, +): string => { + if (isFirstRun) { + cache = {}; + cachedFileInfo = undefined; + } + + if (!cachedFileInfo) { + const fsPath = toFsPath(filename); + const projectInfo = init(fsPath, { + traverse: false, + entryFileContent: fileContent, + }); + + cachedFileInfo = projectInfo.fileInfo; + cache = hasEncapsulationViolations(fsPath, projectInfo); + } + + if (cachedFileInfo.isUnresolvableImport(importCommand)) { + return `import ${importCommand} cannot be resolved`; + } + + const imports = Object.keys(cache); + if (!imports.includes(importCommand)) { + return ''; + } + + if (isLegacyDeepImport) { + return "Deep import is not allowed. Use the module's index.ts or path."; + } else { + const importFileInfo = cache[importCommand]; + return importFileInfo.moduleInfo.hasBarrel + ? `'${importCommand}' is a deep import from a barrel module. Use the module's barrel file (index.ts) instead.` + : `'${importCommand}' cannot be imported. It is encapsulated.`; + } +}; diff --git a/packages/eslint-plugin/src/index.ts b/packages/eslint-plugin/src/index.ts index 30f46fc..bd23948 100644 --- a/packages/eslint-plugin/src/index.ts +++ b/packages/eslint-plugin/src/index.ts @@ -1,6 +1,6 @@ import rules from './lib/rules'; -import { legacy } from "./lib/configs/legacy"; -import { all } from "./lib/configs/all"; +import { legacy, legacyBarrelModulesOnly } from "./lib/configs/legacy"; +import { all, barrelModulesOnly } from "./lib/configs/all"; import { name as packageName, version as packageVersion, @@ -10,6 +10,8 @@ const meta = { name: packageName, version: packageVersion }; const configs = { legacy, + legacyBarrelModulesOnly, + barrelModulesOnly, all }; diff --git a/packages/eslint-plugin/src/lib/configs/all.ts b/packages/eslint-plugin/src/lib/configs/all.ts index f2baf25..8c093e3 100644 --- a/packages/eslint-plugin/src/lib/configs/all.ts +++ b/packages/eslint-plugin/src/lib/configs/all.ts @@ -1,7 +1,7 @@ import rules from '../rules'; import type { TSESLint } from '@typescript-eslint/utils'; -export const all: TSESLint.FlatConfig.Config = { +export const barrelModulesOnly: TSESLint.FlatConfig.Config = { languageOptions: { sourceType: 'module', }, @@ -11,7 +11,23 @@ export const all: TSESLint.FlatConfig.Config = { }, }, rules: { + '@softarc/sheriff/dependency-rule': 'error', '@softarc/sheriff/deep-import': 'error', + }, +}; + + +export const all: TSESLint.FlatConfig.Config = { + languageOptions: { + sourceType: 'module', + }, + plugins: { + '@softarc/sheriff': { + rules, + }, + }, + rules: { '@softarc/sheriff/dependency-rule': 'error', + '@softarc/sheriff/encapsulation': 'error', }, }; diff --git a/packages/eslint-plugin/src/lib/configs/legacy.ts b/packages/eslint-plugin/src/lib/configs/legacy.ts index 8083963..3c77d4e 100644 --- a/packages/eslint-plugin/src/lib/configs/legacy.ts +++ b/packages/eslint-plugin/src/lib/configs/legacy.ts @@ -1,10 +1,20 @@ import { ESLint } from "eslint"; -export const legacy: ESLint.ConfigData = { +export const legacyBarrelModulesOnly: ESLint.ConfigData = { parser: '@typescript-eslint/parser', plugins: ['@softarc/sheriff'], rules: { + '@softarc/sheriff/dependency-rule': 'error', '@softarc/sheriff/deep-import': 'error', + }, +} + +export const legacy: ESLint.ConfigData = { + parser: '@typescript-eslint/parser', + plugins: ['@softarc/sheriff'], + rules: { '@softarc/sheriff/dependency-rule': 'error', + '@softarc/sheriff/encapsulation': 'error', }, } + diff --git a/packages/eslint-plugin/src/lib/rules/deep-import.ts b/packages/eslint-plugin/src/lib/rules/deep-import.ts index 9ffd5cb..6e97878 100644 --- a/packages/eslint-plugin/src/lib/rules/deep-import.ts +++ b/packages/eslint-plugin/src/lib/rules/deep-import.ts @@ -1,15 +1,16 @@ -import { hasDeepImport } from '@softarc/sheriff-core'; +import { violatesEncapsulationRule } from '@softarc/sheriff-core'; import { createRule } from './create-rule'; export const deepImport = createRule( 'Deep Import', (context, node, isFirstRun, filename, sourceCode) => { const importValue = (node.source as { value: string }).value; - const message = hasDeepImport( + const message = violatesEncapsulationRule( filename, importValue, isFirstRun, sourceCode, + true ); if (message) { context.report({ diff --git a/packages/eslint-plugin/src/lib/rules/encapsulation.ts b/packages/eslint-plugin/src/lib/rules/encapsulation.ts new file mode 100644 index 0000000..ff0176e --- /dev/null +++ b/packages/eslint-plugin/src/lib/rules/encapsulation.ts @@ -0,0 +1,22 @@ +import { violatesEncapsulationRule } from '@softarc/sheriff-core'; +import { createRule } from './create-rule'; + +export const encapsulation = createRule( + 'Encapsulation', + (context, node, isFirstRun, filename, sourceCode) => { + const importValue = (node.source as { value: string }).value; + const message = violatesEncapsulationRule( + filename, + importValue, + isFirstRun, + sourceCode, + false + ); + if (message) { + context.report({ + message, + node, + }); + } + }, +); diff --git a/packages/eslint-plugin/src/lib/rules/index.ts b/packages/eslint-plugin/src/lib/rules/index.ts index 3e36367..4a65c19 100644 --- a/packages/eslint-plugin/src/lib/rules/index.ts +++ b/packages/eslint-plugin/src/lib/rules/index.ts @@ -1,7 +1,9 @@ import { deepImport } from './deep-import'; import { dependencyRule } from './dependency-rule'; +import { encapsulation } from "./encapsulation"; export default { 'deep-import': deepImport, 'dependency-rule': dependencyRule, + 'encapsulation': encapsulation, }; diff --git a/packages/eslint-plugin/src/lib/rules/tests/deep-import.spec.ts b/packages/eslint-plugin/src/lib/rules/tests/deep-import.spec.ts index b5d43fb..9496dc2 100644 --- a/packages/eslint-plugin/src/lib/rules/tests/deep-import.spec.ts +++ b/packages/eslint-plugin/src/lib/rules/tests/deep-import.spec.ts @@ -9,7 +9,7 @@ const tester = new RuleTester({ }); describe('deep-import', () => { - const spy = vitest.spyOn(sheriffCore, 'hasDeepImport'); + const spy = vitest.spyOn(sheriffCore, 'violatesEncapsulationRule'); afterEach(() => { spy.mockReset(); @@ -34,7 +34,7 @@ describe('deep-import', () => { valid: [{ code }], invalid: [], }); - expect(spy).toHaveBeenCalledWith('', moduleName, true, code); + expect(spy).toHaveBeenCalledWith('', moduleName, true, code, true); }); it('should not check for deep imports if no import are present', () => { diff --git a/packages/eslint-plugin/src/lib/rules/tests/encapsulation.spec.ts b/packages/eslint-plugin/src/lib/rules/tests/encapsulation.spec.ts new file mode 100644 index 0000000..5f84c32 --- /dev/null +++ b/packages/eslint-plugin/src/lib/rules/tests/encapsulation.spec.ts @@ -0,0 +1,85 @@ +import { RuleTester } from 'eslint'; +import { afterEach, describe, expect, it, vitest } from 'vitest'; +import * as sheriffCore from '@softarc/sheriff-core'; +import { parser } from "typescript-eslint"; +import { encapsulation } from "../encapsulation"; + +const tester = new RuleTester({ + languageOptions: { parser, sourceType: 'module' }, +}); + +describe('encapsulation', () => { + const spy = vitest.spyOn(sheriffCore, 'violatesEncapsulationRule'); + + afterEach(() => { + spy.mockReset(); + }); + + it.each([ + { + code: 'import {AppComponent} from "./app.component"', + moduleName: './app.component', + }, + { + code: 'import * as path from "path"', + moduleName: 'path', + }, + { + code: 'import {inject} from "@angular/core"', + moduleName: '@angular/core', + }, + ])('should check for $moduleName in $code', ({ code, moduleName }) => { + spy.mockImplementation(() => ''); + tester.run('encapsulation', encapsulation, { + valid: [{ code }], + invalid: [], + }); + expect(spy).toHaveBeenCalledWith('', moduleName, true, code, false); + }); + + it('should not check for violations if no import is present', () => { + tester.run('encapsulation', encapsulation, { + valid: [{ code: 'const a = 1' }], + invalid: [], + }); + expect(spy).not.toHaveBeenCalled(); + }); + + it('should direclty use the message from encapsulation', () => { + spy.mockImplementation(() => 'nothing works'); + tester.run('encapsulation', encapsulation, { + valid: [], + invalid: [ + { + code: 'import {AppComponent} from "./app.component"', + errors: [ + { + message: 'nothing works', + }, + ], + }, + ], + }); + }); + + it('should report an internal error only once', () => { + spy.mockImplementation(() => { + throw new Error('This is an error'); + }); + tester.run('encapsulation', encapsulation, { + valid: [], + invalid: [ + { + code: + 'import {AppComponent} from "./app.component";' + + 'import {Service} from "somewhere";', + errors: [ + { + message: 'Encapsulation (internal error): This is an error', + }, + ], + }, + ], + }); + }); +}); diff --git a/test-projects/angular-i/integration-test.sh b/test-projects/angular-i/integration-test.sh index 55e27ac..3c6b3e0 100755 --- a/test-projects/angular-i/integration-test.sh +++ b/test-projects/angular-i/integration-test.sh @@ -36,13 +36,13 @@ npx ng lint --force --format json --output-file tests/actual/dynamic-import-lint diff tests/actual/dynamic-import-lint.json tests/expected/dynamic-import-lint.json cp sheriff.config.ts.original sheriff.config.ts -## Deep Import Check -echo 'checking for deep import error' +## Encapsulation Check +echo 'checking for encapsulation error' mv src/app/customers/feature/components/customers-container.component.ts src/app/customers/feature/components/customers-container.component.ts.original cp tests/customers-container.deep-import.component.ts src/app/customers/feature/components/customers-container.component.ts -npx ng lint --force --format json --output-file tests/actual/deep-import-lint.json -../remove-paths.mjs tests/actual/deep-import-lint.json -diff tests/actual/deep-import-lint.json tests/expected/deep-import-lint.json +npx ng lint --force --format json --output-file tests/actual/encapsulation-lint.json +../remove-paths.mjs tests/actual/encapsulation-lint.json +diff tests/actual/encapsulation-lint.json tests/expected/encapsulation-lint.json mv src/app/customers/feature/components/customers-container.component.ts.original src/app/customers/feature/components/customers-container.component.ts ## Dependency Rule Check diff --git a/test-projects/angular-iv/tests/expected/deep-import-lint.json b/test-projects/angular-i/tests/expected/encapsulation-lint.json similarity index 90% rename from test-projects/angular-iv/tests/expected/deep-import-lint.json rename to test-projects/angular-i/tests/expected/encapsulation-lint.json index 994a88b..62b967a 100644 --- a/test-projects/angular-iv/tests/expected/deep-import-lint.json +++ b/test-projects/angular-i/tests/expected/encapsulation-lint.json @@ -3,9 +3,9 @@ "filePath": "./src/app/customers/feature/components/customers-container.component.ts", "messages": [ { - "ruleId": "@softarc/sheriff/deep-import", + "ruleId": "@softarc/sheriff/encapsulation", "severity": 2, - "message": "Deep import is not allowed. Use the module's index.ts or path.", + "message": "'../../data/customers-repository.service' is a deep import from a barrel module. Use the module's barrel file (index.ts) instead.", "line": 4, "column": 1, "nodeType": "ImportDeclaration", diff --git a/test-projects/angular-iv/integration-test.sh b/test-projects/angular-iv/integration-test.sh index 6c97adb..2e080fa 100755 --- a/test-projects/angular-iv/integration-test.sh +++ b/test-projects/angular-iv/integration-test.sh @@ -36,13 +36,13 @@ npx ng lint --force --format json --output-file tests/actual/dynamic-import-lint diff tests/actual/dynamic-import-lint.json tests/expected/dynamic-import-lint.json cp sheriff.config.ts.original sheriff.config.ts -## Deep Import Check -echo 'checking for deep import error' +## Encapsulation Check +echo 'checking for encapsulation error' mv src/app/customers/feature/components/customers-container.component.ts src/app/customers/feature/components/customers-container.component.ts.original cp tests/customers-container.deep-import.component.ts src/app/customers/feature/components/customers-container.component.ts -npx ng lint --force --format json --output-file tests/actual/deep-import-lint.json -../remove-paths.mjs tests/actual/deep-import-lint.json -diff tests/actual/deep-import-lint.json tests/expected/deep-import-lint.json +npx ng lint --force --format json --output-file tests/actual/encapsulation-lint.json +../remove-paths.mjs tests/actual/encapsulation-lint.json +diff tests/actual/encapsulation-lint.json tests/expected/encapsulation-lint.json mv src/app/customers/feature/components/customers-container.component.ts.original src/app/customers/feature/components/customers-container.component.ts ## Dependency Rule Check diff --git a/test-projects/angular-i/tests/expected/deep-import-lint.json b/test-projects/angular-iv/tests/expected/encapsulation-lint.json similarity index 90% rename from test-projects/angular-i/tests/expected/deep-import-lint.json rename to test-projects/angular-iv/tests/expected/encapsulation-lint.json index 994a88b..62b967a 100644 --- a/test-projects/angular-i/tests/expected/deep-import-lint.json +++ b/test-projects/angular-iv/tests/expected/encapsulation-lint.json @@ -3,9 +3,9 @@ "filePath": "./src/app/customers/feature/components/customers-container.component.ts", "messages": [ { - "ruleId": "@softarc/sheriff/deep-import", + "ruleId": "@softarc/sheriff/encapsulation", "severity": 2, - "message": "Deep import is not allowed. Use the module's index.ts or path.", + "message": "'../../data/customers-repository.service' is a deep import from a barrel module. Use the module's barrel file (index.ts) instead.", "line": 4, "column": 1, "nodeType": "ImportDeclaration",