Skip to content

Commit

Permalink
feat!: restructures the via* sub-restrictions to also accept & valida…
Browse files Browse the repository at this point in the history
…te against dependency types (BREAKING for API users) (#894)

## Description

- restructures the via* sub restrictions to also accept & validate
against dependency types
- deprecates the viaNot and viaSomeNot restrictions in favor of
viaOnly.pathNot and via.pathNot restrictions respectively. You could
still have them in dependency-cruiser configs, but internally they're
rewritten into `viaOnly` and `via` rules when possible.

This is a breaking change for the API only 
- in the input/ rules instead of a string | string[] the via and viaOnly
restrictions can now also be an object with `path`, `pathNot`,
`dependencyTypes` and `dependencyTypesNot` attributes
- in the cruise result via and viaOnly are _only_ available as that
object (in stead of string | string[])
- in the cruise result the `viaNot` and `viaSomeNot` have disappeared
(rules in the input will have been rewritten as via or viaOnly rules).

## Motivation and Context

There's a long outstanding and much upvoted request (#695) to be able to
exclude e.g. type-only over a whole cycle instead to just the first
dependency of the cycle. This PR enables that.

## How Has This Been Tested?

- [x] green ci
- [x] additional automated non-regression tests

## Types of changes

- [ ] Bug fix (non-breaking change which fixes an issue)
- [ ] Documentation only change
- [ ] Refactor (non-breaking change which fixes an issue without
changing functionality)
- [x] New feature (non-breaking change which adds functionality)
- [x] Breaking change (fix or feature that would cause existing
functionality to change) => for API only
  • Loading branch information
sverweij committed Dec 22, 2023
1 parent 97e7c03 commit 05d9795
Show file tree
Hide file tree
Showing 24 changed files with 1,077 additions and 564 deletions.
2 changes: 1 addition & 1 deletion .c8rc.json
@@ -1,7 +1,7 @@
{
"checkCoverage": true,
"statements": 99.89,
"branches": 98.77,
"branches": 98.78,
"functions": 100,
"lines": 99.89,
"exclude": [
Expand Down
120 changes: 84 additions & 36 deletions doc/rules-reference.md
Expand Up @@ -709,38 +709,79 @@ up at itself.
}
```

### `via` and `viaNot`, `viaOnly` and `viaSomeNot` - restricting what cycles to match
#### `via` and `viaOnly`- restricting what cycles to match

There are two attributes to put restrictions on through which modules
cycles are allowed to pass:

- `via`: matches against _some_ of the modules in the cycle
- `viaOnly`: matches against _all_ of the modules in the cycle

Both take the `path`, `pathNot` and `dependencyTypes`, `dependencyTypesNot`
attributes that have the a meaning similar to the ones in the `from` and `to`
parts of a rule, with dependencyTypes expressing the type of relation a
module has with its predecessor in the cycle.

With the cycle `a/aa.js` -> `a/ab.js` -> `b/bb.js` -> `a/aa.js` the restrictions
to this:

| restriction | what it does | example input | match? | because... |
| -------------- | -------------------------------------------------------- | --------------- | ------- | ----------------------------- |
| `via` | **some** of the modules in the cycle match the condition | `path: "^a/.+"` | `true` | `a/aa.js` and `a/ab.js` match |
| `viaOnly` | **all** of the modules in the cycle match the condition | `path: "^a/.+"` | `false` | `b/bb.js` doesn't match |
| _`viaNot`_ | _deprecated - use `viaOnly.pathNot` in stead_ | `path: "^a/.+"` | `false` | `a/aa.js` and `a/ab.js` match |
| _`viaSomeNot`_ | _deprecated - use `via.pathNot` in stead_ | `path: "^a/.+"` | `true` | `b/bb.js` doesn't match |

##### Example: allow cycles that have a type-only dependency in them

If you're only interested in cycles at run time, you might allow dependencies
that are only used at compile time to be part of a cycle. `type-only` ones
are a good example of that. E.g. at run time this cycle, where `r.ts` imports
some things from `s.ts` as 'type-only':

```mermaid
flowchart LR
p.ts --> q.ts --> r.ts --> |type-only| s.ts --> p.ts
```

... would look like this at run time (so: not a cycle):

```mermaid
flowchart LR
p.ts --> q.ts --> r.ts
s.ts --> p.ts
```

To express this in a rule, you can use the `viaOnly` attribute:

```javascript
// log an error for all circular dependencies which consist of only non-`type-only`
// dependencies
{
name: 'no-circular-at-runtime',
severity: 'error',
from: {
},
to: {
circular: true,
viaOnly: {
dependencyTypesNot: ['type-only']
}
}
},
```

##### Example: exclude cycles from erroring when they go to a known 'knot'

Some codebases include a lot of circular dependencies, sometimes with a few 'knots'
(typically barrel files) that partake in most of them. Fixing these cycles might
take a spell, so you might want to (temporarily :-) ) exclude them from breaking
the build. At the same time you might want to prevent any new violation going
unnoticed because of this.

One solution to this is to use dependency-cruiser's
[`ignore-known`](cli.md#--ignore-known-ignore-known-violations) mechanism, Another
solution is to put restrictions on through which modules the cycles pass; the
"via"'s, in a similar fashion as possible with `path` and `pathNot`. There are
_four_ via-like restrictions in dependency-cruiser, as - different from the
`path`/`pathNot` restrictions the `via` (and `viaNot`) ones always almost have
to check against multiple paths; all the "via"'s in the cycle. The variants
exist to enable matching against only _some_ of the modules in the cycle or
against _all_ of them.

All these restrictions take the whole cycle into account; _including_ the tested
'from'; if `a/aa.js` has a cycle via `a/ab.js` and `b/bb/js` back to `a/aa.js`
the via-like restrictions also take `a/aa.js` into account.

The examples below refer to this cycle: `a/aa.js`, `a/ab.js`, `b/bb.js`, `a/aa.js`

| restriction | what it does | example input | match? | because... |
| ------------ | ------------------------------------------------------------------- | ------------- | ------- | ----------------------------- |
| `via` | **some** of the modules in the cycle **do** match the expression | `^a/.+` | `true` | `a/aa.js` and `a/ab.js` match |
| `viaOnly` | **all** of the modules in the cycle **do** match the expression | `^a/.+` | `false` | `b/bb.js` doesn't match |
| `viaNot` | **all** of the modules in the cycle **don't** match the expression | `^a/.+` | `false` | `a/aa.js` and `a/ab.js` match |
| `viaSomeNot` | **some** of the modules in the cycle **don't** match the expression | `^a/.+` | `true` | `b/bb.js` doesn't match |

#### Usage example: prevent code from going through a 'knot'
The recommended solution for this is dependency-cruiser's
[`ignore-known`](cli.md#--ignore-known-ignore-known-violations) mechanism.
However, you can also use the `via` attribute.

In this example `app/javascript/tables/index.ts` and `app/javascript/tables/index.ts`
are the known 'knots':
Expand All @@ -755,10 +796,12 @@ are the known 'knots':
},
to: {
circular: true,
viaNot: [
'^app/javascript/tables/index.ts',
'^app/javascript/ui/index.tsx',
]
via: {
pathNot: [
'^app/javascript/tables/index.ts',
'^app/javascript/ui/index.tsx',
]
}
}
},

Expand All @@ -771,21 +814,22 @@ are the known 'knots':
},
to: {
circular: true,
via: [
via: {
path: [
'^app/javascript/tables/index.ts',
'^app/javascript/ui/index.tsx',
]
]
}
}
}
```

#### Usage example: prevent cycles from going outside a folder
##### Example: prevent cycles from going outside a folder

This example (adapted from a
[question on GitHub](https://github.com/sverweij/dependency-cruiser/issues/585)
by [@PetFeld-ed](https://github.com/PetFeld-ed))
not only makes use of the `viaSomeNot`, but also displays the use of
[group matching](#group-matching).
by [@PetFeld-ed](https://github.com/PetFeld-ed)) not only uses the `via`, but
also displays [group matching](#group-matching).

```javascript
// in the `forbidden` section of a dependency-cruiser config:
Expand All @@ -802,7 +846,11 @@ not only makes use of the `viaSomeNot`, but also displays the use of
},
to: {
circular: true,
viaSomeNot: '^src/business-components/$1/.+',
via: {
pathNot: '^src/business-components/$1/.+'
},
// previously written with the now deprecated viaSomeNot attribute:
// viaSomeNot: '^src/business-components/$1/.+'
},
}
```
Expand Down
4 changes: 0 additions & 4 deletions src/main/helpers.mjs
Expand Up @@ -9,10 +9,6 @@ const RE_PROPERTIES = [
"licenseNot",
"exoticRequire",
"exoticRequireNot",
"via",
"viaNot",
"viaOnly",
"viaSomeNot",
];

/**
Expand Down
7 changes: 6 additions & 1 deletion src/main/rule-set/assert-validity.mjs
@@ -1,6 +1,7 @@
import Ajv from "ajv";
import safeRegex from "safe-regex";
import has from "lodash/has.js";
import get from "lodash/get.js";
import { assertCruiseOptionsValid } from "../options/assert-validity.mjs";
import { normalizeToREAsString } from "../helpers.mjs";
import configurationSchema from "#configuration-schema";
Expand Down Expand Up @@ -34,7 +35,7 @@ function hasPath(pObject, pSection, pCondition) {
function safeRule(pRule, pSection, pCondition) {
return (
!hasPath(pRule, pSection, pCondition) ||
safeRegex(normalizeToREAsString(pRule[pSection][pCondition]), {
safeRegex(normalizeToREAsString(get(pRule[pSection], pCondition)), {
limit: MAX_SAFE_REGEX_STAR_REPEAT_LIMIT,
})
);
Expand All @@ -49,9 +50,13 @@ function assertRuleSafety(pRule) {
{ section: "to", condition: "license" },
{ section: "to", condition: "licenseNot" },
{ section: "to", condition: "via" },
{ section: "to", condition: "via.path" },
{ section: "to", condition: "viaNot" },
{ section: "to", condition: "viaNot.path" },
{ section: "to", condition: "viaOnly" },
{ section: "to", condition: "viaOnly.path" },
{ section: "to", condition: "viaSomeNot" },
{ section: "to", condition: "viaSomeNot.path" },
];

if (
Expand Down
77 changes: 64 additions & 13 deletions src/main/rule-set/normalize.mjs
@@ -1,5 +1,5 @@
import has from "lodash/has.js";
import { normalizeREProperties } from "../helpers.mjs";
import { normalizeREProperties, normalizeToREAsString } from "../helpers.mjs";

const VALID_SEVERITIES = /^(error|warn|info|ignore)$/;
const DEFAULT_SEVERITY = "warn";
Expand All @@ -23,26 +23,74 @@ function normalizeName(pRuleName) {
}

/**
*
* @param {import("../../../types/shared-types.js").RuleScopeType} pScope?
* @returns {import("../../../types/shared-types.js").RuleScopeType}
* @param {import("../../../types/shared-types.mjs").RuleScopeType} pScope?
* @returns {import("../../../types/shared-types.mjs").RuleScopeType}
*/
function normalizeScope(pScope) {
return pScope ? pScope : DEFAULT_SCOPE;
}

/**
*
* @param {import("../../../types/restrictions.mjs").MiniDependencyRestrictionType} pVia
* @returns {import("../../../types/strict-restrictions.mjs").IStrictMiniDependencyRestriction}
*/
function normalizeVia(pVia) {
let lReturnValue = {};

if (typeof pVia === "string" || Array.isArray(pVia)) {
lReturnValue.path = pVia;
} else {
lReturnValue = structuredClone(pVia);
}
if (has(lReturnValue, "path")) {
lReturnValue.path = normalizeToREAsString(lReturnValue.path);
}
return lReturnValue;
}

// eslint-disable-next-line complexity
function normalizeVias(pRuleTo) {
const lRuleTo = structuredClone(pRuleTo);

if (has(lRuleTo, "via")) {
lRuleTo.via = normalizeVia(lRuleTo.via);
}
if (has(lRuleTo, "viaOnly")) {
lRuleTo.viaOnly = normalizeVia(lRuleTo.viaOnly);
}
if (has(lRuleTo, "viaNot")) {
if (!has(lRuleTo, "viaOnly.pathNot")) {
lRuleTo.viaOnly = {
...lRuleTo.viaOnly,
pathNot: normalizeToREAsString(lRuleTo.viaNot),
};
}
delete lRuleTo.viaNot;
}
if (has(lRuleTo, "viaSomeNot")) {
if (!has(lRuleTo, "via.pathNot")) {
lRuleTo.via = {
...lRuleTo.via,
pathNot: normalizeToREAsString(lRuleTo.viaSomeNot),
};
}
delete lRuleTo.viaSomeNot;
}
return lRuleTo;
}

/**
* @param {import("../../../types/rule-set.mjs").IAnyRuleType} pRule
* @returns {import("../../../types/strict-rule-set.js").IStrictAnyRuleType}
* @returns {import("../../../types/strict-rule-set.mjs").IStrictAnyRuleType}
*/
function normalizeRule(pRule) {
const lRuleTo = normalizeVias(pRule.to);
return {
...pRule,
severity: normalizeSeverity(pRule.severity),
name: normalizeName(pRule.name),
from: normalizeREProperties(pRule.from),
to: normalizeREProperties(pRule.to),
to: normalizeREProperties(lRuleTo),
scope: normalizeScope(pRule.scope),
...(pRule.module ? { module: normalizeREProperties(pRule.module) } : {}),
};
Expand All @@ -66,12 +114,15 @@ export default function normalizeRuleSet(pRuleSet) {
Reflect.deleteProperty(lRuleSet, "allowed");
Reflect.deleteProperty(lRuleSet, "allowedSeverity");
} else {
lRuleSet.allowed = lRuleSet.allowed.map((pRule) => ({
...pRule,
name: "not-in-allowed",
from: normalizeREProperties(pRule.from),
to: normalizeREProperties(pRule.to),
}));
lRuleSet.allowed = lRuleSet.allowed.map((pRule) => {
const lRuleTo = normalizeVias(pRule.to);
return {
...pRule,
name: "not-in-allowed",
from: normalizeREProperties(pRule.from),
to: normalizeREProperties(lRuleTo),
};
});
}
}

Expand Down
1 change: 1 addition & 0 deletions src/schema/baseline-violations.schema.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion src/schema/baseline-violations.schema.mjs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

0 comments on commit 05d9795

Please sign in to comment.