Skip to content

Commit dce404c

Browse files
committed
feat(lint): detect import cycles with import-x/no-cycle
A circular import leaves one module in the cycle holding `undefined`, and which module loses depends on where the graph is entered. That makes it invisible to the built CLI and to most tests — the migrate/reconcile-marker cycle surfaced in exactly two tests, by luck, as `TypeError: migrate is not a function`. Nothing in the repo looked for it. Adds eslint-plugin-import-x and turns on `import-x/no-cycle` over the TypeScript sources. Only that rule; no other rules from the plugin. Two settings are load-bearing and easy to get wrong: - `import-x/extensions` must list the TS extensions. Its default is `['.js', '.mjs', '.cjs']`, and a file outside that list is dropped by `ExportMap.get` before its imports are read. Without it the rule resolves our files, walks into them, finds nothing, and passes on a tree that provably contains a cycle. - `import-x/resolver-next` needs the same list for a different reason: the built-in resolver defaults to `['.mjs', '.cjs', '.js', '.json', '.node']` and our sources import extensionlessly. Type-only edges are ignored, which is the rule's own non-configurable behavior and the behavior we want: an `import type` edge is erased before the module runs, so it cannot produce the `undefined` binding this rule exists to catch. `verbatimModuleSyntax` is what makes that safe to lean on. The only cycle on main is `filesystem/migrate` <-> `rules/reconcile-marker`, which PR #388 already breaks by splitting out `filesystem/manifest.ts`. Exempted at both ends with a comment rather than fixed here, to avoid conflicting with #388 in the same file; the disables become unused-disable warnings once it lands. The guard test asks ESLint whether the rule is on and whether it reports a real cycle written into packages/cli/src. It does not re-implement cycle detection. It exists because the failure mode is silence: a green lint run looks identical whether the rule works or is inert.
1 parent 07afcbc commit dce404c

4 files changed

Lines changed: 515 additions & 0 deletions

File tree

‎eslint.config.js‎

Lines changed: 99 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import eslint from "@eslint/js";
22
import tseslint from "typescript-eslint";
33
import unicorn from "eslint-plugin-unicorn";
4+
import importX, { createNodeResolver } from "eslint-plugin-import-x";
45
import prettierConfig from "eslint-config-prettier";
56

67
export default tseslint.config(
@@ -100,6 +101,104 @@ export default tseslint.config(
100101
],
101102
},
102103
},
104+
// Import cycle detection.
105+
//
106+
// A circular import leaves one of the modules in the cycle holding
107+
// `undefined` for whatever it imported, and which module loses depends on
108+
// which one the graph is entered at first. That makes it a bug that the
109+
// built CLI and most tests never see: it only fires when something enters
110+
// the graph at the unlucky module. We hit exactly that — a migration
111+
// registry that read as `undefined` and failed with
112+
// `TypeError: migrate is not a function`, visible in two tests by luck.
113+
// This rule is the check that would have caught it at author time.
114+
{
115+
files: ["**/*.ts", "**/*.tsx"],
116+
plugins: { "import-x": importX },
117+
settings: {
118+
// Which extensions the plugin will parse when it follows an edge out of
119+
// the file being linted. This is NOT cosmetic and it is not the same
120+
// knob as the resolver below: the default is `['.js', '.mjs', '.cjs']`,
121+
// and a file whose extension is not on this list is dropped by
122+
// `ExportMap.get` before its imports are ever read. Without `.ts` here
123+
// the rule resolves our files correctly, walks into them, finds nothing,
124+
// and reports no cycles — on a tree that provably contains one. A lint
125+
// run that is green because the rule is inert looks exactly like a lint
126+
// run that is green because the code is clean, which is why the
127+
// reintroduced-cycle check in this PR's description exists.
128+
"import-x/extensions": [
129+
".ts",
130+
".tsx",
131+
".mts",
132+
".cts",
133+
".js",
134+
".jsx",
135+
".mjs",
136+
".cjs",
137+
],
138+
// The plugin's own resolver, configured for TypeScript. It is backed by
139+
// `unrs-resolver`, a direct dependency of eslint-plugin-import-x, so
140+
// this needs no separate resolver package. It does need the extension
141+
// list spelled out: the built-in default is
142+
// `['.mjs', '.cjs', '.js', '.json', '.node']`, which resolves no `.ts`
143+
// at all, and our sources import extensionlessly under
144+
// `moduleResolution: "bundler"`. `.js` stays in the list for the handful
145+
// of specifiers that carry an explicit extension.
146+
"import-x/resolver-next": [
147+
createNodeResolver({
148+
extensions: [
149+
".ts",
150+
".tsx",
151+
".mts",
152+
".cts",
153+
".js",
154+
".mjs",
155+
".cjs",
156+
".json",
157+
],
158+
}),
159+
],
160+
},
161+
rules: {
162+
// On `import type` edges: the rule ignores them, in both directions —
163+
// it returns early on an `ImportDeclaration` whose `importKind` is
164+
// `type` (or whose every specifier is), and it filters
165+
// `isOnlyImportingTypes` edges out of the graph walk. That is the
166+
// behavior we want and it is not configurable, so there is no option
167+
// below for it. It is also correct for us: a type-only edge is erased
168+
// before the module ever runs, so it cannot produce the `undefined`
169+
// binding this rule exists to catch, and flagging it would push people
170+
// toward restructuring real code to satisfy an import that has no
171+
// runtime existence. `verbatimModuleSyntax: true` in `tsconfig.base.json`
172+
// is what makes this safe to lean on: it forces a type-only import to be
173+
// written as `import type`, so the erasure is explicit in the syntax the
174+
// rule reads rather than something the compiler infers later.
175+
"import-x/no-cycle": [
176+
"error",
177+
{
178+
// `maxDepth` is deliberately not set. Omitting it means unlimited
179+
// (the rule reads it as `Number.POSITIVE_INFINITY` unless a number
180+
// is given), and unlimited is what we want: the cycle we shipped was
181+
// not a two-module A->B->A, and capping the depth would trade away
182+
// exactly the cycles that are hard to spot by reading the code,
183+
// which are the only ones worth spending a lint rule on. It is left
184+
// out rather than passed as `Infinity` because the rule's schema
185+
// accepts only an integer or the string "∞" there.
186+
// Do not traverse into node_modules. A cycle that runs through a
187+
// published dependency is not ours to break — we cannot edit it, so
188+
// a report on it is noise we would have to suppress — and walking
189+
// the dependency graph is where this rule's cost actually goes.
190+
ignoreExternal: true,
191+
// Keep the default (false). This would suppress a cycle whenever any
192+
// edge in it is a dynamic `import()`, on the theory that the deferred
193+
// evaluation breaks the loop. It does not reliably: a dynamic import
194+
// awaited during module init is as circular as a static one, and the
195+
// failure mode is the same `undefined`. We have no cycle that needs
196+
// the escape hatch, so we do not open it.
197+
allowUnsafeDynamicCyclicDependency: false,
198+
},
199+
],
200+
},
201+
},
103202
// File naming conventions - enforce kebab-case for all TS/TSX files
104203
{
105204
files: ["**/*.ts", "**/*.tsx"],

‎package.json‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@
4848
"@types/node": "^25.3.0",
4949
"eslint": "^9.39.2",
5050
"eslint-config-prettier": "^10.1.8",
51+
"eslint-plugin-import-x": "^4.17.1",
5152
"eslint-plugin-unicorn": "^62.0.0",
5253
"husky": "^9.1.7",
5354
"lint-staged": "^15.4.3",
Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,84 @@
1+
import { mkdtemp, rm, writeFile } from "node:fs/promises";
2+
import { join, resolve } from "node:path";
3+
4+
import { ESLint } from "eslint";
5+
import { afterAll, describe, expect, it } from "vitest";
6+
7+
/**
8+
* A guard that `import-x/no-cycle` is actually ON and actually reaching
9+
* `packages/cli/src`.
10+
*
11+
* This does NOT re-implement cycle detection — that would be exactly the
12+
* "re-derive what the tool already knows" mistake the style guide forbids.
13+
* Every assertion below asks ESLint, running the repository's real
14+
* `eslint.config.js`, and checks what it answers.
15+
*
16+
* It exists because the rule's failure mode is silence. While this rule was
17+
* being added, the config resolved correctly, matched the right files, and
18+
* reported `import-x/no-cycle` as an enabled error — and still found nothing on
19+
* a tree that provably contained a cycle, because `import-x/extensions`
20+
* defaults to `['.js', '.mjs', '.cjs']` and so every `.ts` file was dropped
21+
* before its imports were read. A green `pnpm lint` looked identical whether
22+
* the rule was working or inert. Nothing but an actual cycle distinguishes
23+
* those two states, which is why the second test below writes one.
24+
*/
25+
26+
const REPO_ROOT = resolve(import.meta.dirname, "..", "..", "..");
27+
const CLI_SOURCE = resolve(REPO_ROOT, "packages/cli/src");
28+
29+
const temporaryDirectories: string[] = [];
30+
31+
afterAll(async () => {
32+
await Promise.all(
33+
temporaryDirectories.map(async (directory) =>
34+
rm(directory, { recursive: true, force: true })
35+
)
36+
);
37+
});
38+
39+
function createESLint(): ESLint {
40+
return new ESLint({ cwd: REPO_ROOT });
41+
}
42+
43+
describe("import-x/no-cycle", () => {
44+
it("is enabled as an error for files in packages/cli/src", async () => {
45+
const config = (await createESLint().calculateConfigForFile(
46+
join(CLI_SOURCE, "index.ts")
47+
)) as { rules?: Record<string, unknown> };
48+
49+
// "error" is 2 once ESLint normalizes it. A config block that stopped
50+
// matching `packages/cli/src` would leave this undefined.
51+
expect(config.rules?.["import-x/no-cycle"]).toBeDefined();
52+
expect((config.rules?.["import-x/no-cycle"] as unknown[])[0]).toBe(2);
53+
});
54+
55+
it("reports a value cycle written into packages/cli/src", async () => {
56+
// Written inside `packages/cli/src` on purpose: the point of the check is
57+
// that the rule reaches THIS tree, so linting a fixture parked somewhere
58+
// the config does not match would prove nothing. The directory name is
59+
// prefixed so it is obviously not product code if cleanup is ever missed.
60+
const directory = await mkdtemp(join(CLI_SOURCE, "__cycle-guard-"));
61+
temporaryDirectories.push(directory);
62+
63+
// A -> B -> A over VALUE imports. Kept to real value edges because
64+
// type-only edges are erased before the module runs and the rule ignores
65+
// them by design; see the note in eslint.config.js.
66+
await writeFile(
67+
join(directory, "a.ts"),
68+
'import { b } from "./b";\n\nexport const a = (): string => b();\n'
69+
);
70+
await writeFile(
71+
join(directory, "b.ts"),
72+
'import { a } from "./a";\n\nexport const b = (): string => a();\n'
73+
);
74+
75+
const results = await createESLint().lintFiles([join(directory, "*.ts")]);
76+
const cycleMessages = results.flatMap((result) =>
77+
result.messages.filter(
78+
(message) => message.ruleId === "import-x/no-cycle"
79+
)
80+
);
81+
82+
expect(cycleMessages.length).toBeGreaterThan(0);
83+
});
84+
});

0 commit comments

Comments
 (0)