From ab3f7bc2fb10dee1a27f9fef108453bd13ef9736 Mon Sep 17 00:00:00 2001 From: Diogo Esteves Date: Thu, 29 Oct 2026 09:14:07 +0000 Subject: [PATCH 1/2] fix(eslint): answer lints while the design system is looked at again; config edits, rule options, suppression typos The plugin's loader made ESLint wait for a new look at the project each time what it held was two seconds old, so every lint after a pause in an editor paid for that look. It now answers from what it holds and looks again in the background; the lints after the look get what it found. The version the plugin compares now covers the whole config, so a rule turned off in the rules or overrides of onsystem.config.json or .mjs stops being reported without a restart. Projects are kept per target, so two configs of one project in one process do not answer for each other. Rules take [{ allow: string[] }] in ESLint's config, over the project's allow for that rule, and onsystem/suppression reports a misspelled rule in a suppression comment as a warning, as check does. docs/eslint.md says where rule options live, what each ESLint run pays for the plugin, and that --cache does not see design-system changes. OS2-45 --- .changeset/eslint-refresh-options.md | 13 ++ docs/eslint.md | 10 +- src/eslint/index.ts | 79 +++++-- src/eslint/worker.ts | 176 ++++++++++++--- test/eslint-bridge.test.ts | 316 +++++++++++++++++++++++++++ test/eslint.test.ts | 10 +- 6 files changed, 553 insertions(+), 51 deletions(-) create mode 100644 .changeset/eslint-refresh-options.md create mode 100644 test/eslint-bridge.test.ts diff --git a/.changeset/eslint-refresh-options.md b/.changeset/eslint-refresh-options.md new file mode 100644 index 00000000..9ba90689 --- /dev/null +++ b/.changeset/eslint-refresh-options.md @@ -0,0 +1,13 @@ +--- +'onsystem': patch +--- + +The ESLint plugin no longer holds up a lint while it looks at the design system again. In an editor, every lint after a pause of two seconds or more waited for that look, which takes longer the larger the design system. Now the lint is answered at once from the design system already loaded, the look runs in the background, and the lints after it get what it found. + +Edits to the project's config reach an editor's ESLint server without a restart: a rule turned off in the `rules` or `overrides` of `onsystem.config.json`, `.mjs`, `.js` or `.ts` is no longer reported once the plugin has looked at the project again. Before, the plugin kept the rules it loaded first until the server restarted. + +A rule's options can be set in ESLint's config, as in `onsystem.config.json`: `'onsystem/no-hardcoded-color': ['error', { allow: ['#fff'] }]` no longer fails ESLint's config validation ("should NOT have more than 0 items"), and its `allow` takes the place of the one the project's config gives that rule. + +A misspelled rule in a suppression comment, such as `// onsystem-disable-next-line no-hardcoded-colour`, is reported by the new `onsystem/suppression` rule, a warning in the recommended config, with the message `check` gives. Through ESLint, nothing said why the comment suppressed nothing. + +docs/eslint.md says where rule options live, what each ESLint run pays for the plugin (prefer `onsystem check` or the GitHub Action for a CI gate), and that ESLint's `--cache` does not see design-system changes. diff --git a/docs/eslint.md b/docs/eslint.md index b76a6117..f8792f84 100644 --- a/docs/eslint.md +++ b/docs/eslint.md @@ -40,7 +40,9 @@ Each file is checked as `onsystem check` checks it, with zero config in a shadcn - with the `// onsystem-disable-*` [suppression comments](rules.md#suppressing-findings), next to ESLint's own `eslint-disable` comments, - leaving out what `check .` leaves out: the files the config's `exclude` lists, the design system's own components, and files that render an image or an email. -Findings have the same messages, positions and fixes as `check`, and `eslint --fix` applies the fixes. ESLint's config sets each rule's severity (`'onsystem/no-hardcoded-spacing': 'error'`). The [real-world corpus](how-it-works.md#on-real-codebases) also runs through the plugin (`pnpm corpus eslint`): on every one of its runs, it reports exactly the findings `check` reports. +Findings have the same messages, positions and fixes as `check`, and `eslint --fix` applies the fixes. A misspelled rule in a suppression comment, which suppresses nothing, is reported by `onsystem/suppression` as a warning, as `check` reports it. ESLint's config sets each rule's severity (`'onsystem/no-hardcoded-spacing': 'error'`). The [real-world corpus](how-it-works.md#on-real-codebases) also runs through the plugin (`pnpm corpus eslint`): on every one of its runs, it reports exactly the findings `check` reports. + +A rule's options (`allow`) belong in the project's [config](configuration.md#config-file), where `check`, the MCP server and the Claude Code hook read them too. ESLint's config can set them as well, for ESLint only: `'onsystem/no-hardcoded-color': ['error', { allow: ['#fff'] }]` takes the place of the `allow` the project's config gives that rule. A rule the project's config turns off for a file stays off. When the project root or the config is not where it would be found from ESLint's working directory, say where in the settings, as `--root` and `--config` do: @@ -51,7 +53,11 @@ export default [ ]; ``` -The plugin loads each design system once, in a worker thread (ESLint rules run synchronously and extraction does not), and looks at its files again at most every two seconds, so an editor's ESLint server picks up a new component or variant without a restart. +The plugin loads each design system once, in a worker thread (ESLint rules run synchronously and extraction does not), and answers each lint from what it has loaded. When that is more than two seconds old, the worker looks at the project's config and the design system's files again in the background, and the lints after that look get what it found, so an editor's ESLint server picks up a new component or variant, or a rule turned off in the config, without a restart. The lint that starts a look does not wait for it. + +Each ESLint run pays for the plugin on top of ESLint's own work: every file is parsed a second time, by TypeScript as `check` parses it, and each ESLint thread (`--concurrency`) loads the design system in a worker of its own. For a CI gate, `onsystem check` or the [GitHub Action](ci.md) does the same checks with neither cost; the plugin is for editors, and for a setup that runs only ESLint. + +ESLint's `--cache` keys each file on its own content and ESLint's config, not on the design system. After a design-system change, a file that did not change keeps the findings ESLint cached for it: a variant added to a component is still reported as unknown, and a variant taken out passes where the file uses it. Leave `--cache` out of runs with the onsystem rules, or delete `.eslintcache` when the design system changes. `check` and the Action cache no findings. A few things stay with the CLI. The [baseline](ci.md#adopting-it-in-an-existing-codebase) applies to `check` only. ESLint has its own: on ESLint 9.24 and later, `npx eslint . --suppress-all` records the findings already in the code in `eslint-suppressions.json`, which you commit, and later runs leave them out ([bulk suppressions](https://eslint.org/docs/latest/use/suppressions)). Once a recorded finding is fixed, ESLint fails the run until `npx eslint . --prune-suppressions` takes it out of the file. On older versions, use `eslint-disable` comments, or set the onsystem rules to `warn` and cap them with `--max-warnings` (it counts warnings only, and most onsystem rules default to `error`). ESLint reports syntax errors itself. Which files ESLint lints is up to its config (`ignores`), where `check` leaves out what git ignores. diff --git a/src/eslint/index.ts b/src/eslint/index.ts index 1300e9a1..1509a54a 100644 --- a/src/eslint/index.ts +++ b/src/eslint/index.ts @@ -9,7 +9,8 @@ * working directory), with the project's config: its `rules` and `overrides` turn rules off * per file, `// onsystem-disable-*` comments suppress findings, and the files `check .` leaves * out (the config's `exclude`, the design system's own components) are not checked. ESLint's - * config sets each rule's severity. + * config sets each rule's severity, and can set a rule's options (`allow`) over the project's. + * `onsystem/suppression` reports a misspelled rule in a suppression comment, as check does. */ import path from 'node:path'; @@ -19,10 +20,14 @@ import { DEFAULT_SEVERITY, DEFAULT_TEST_EXCLUDE, DEFAULT_TEST_FOLDERS, + RULE_IDS, + rulesFor, + type ResolvedRule, type RuleId, + type RuleOptions, } from '../config.js'; import { DesignSystem } from '../design-system.js'; -import { RULES } from '../lint/index.js'; +import { checkSource, RULES } from '../lint/index.js'; import type { CheckResult, TextEdit } from '../types.js'; import { relativePath } from '../util/paths.js'; import { NAME, VERSION } from '../version.js'; @@ -43,10 +48,15 @@ const passes = new WeakMap( /** * A checked file's findings, with `fromSystem`: whether an import specifier in that file leads - * to the design system. + * to the design system, and `checkWith`: the file checked by one rule alone, with options from + * ESLint's config over the ones the project's config gives the rule for the file. */ type Outcome = - | { result: CheckResult; fromSystem: (specifier: string) => boolean } + | { + result: CheckResult; + fromSystem: (specifier: string) => boolean; + checkWith: (id: RuleId, options: RuleOptions) => CheckResult; + } | { skipped: true } | { error: string }; @@ -86,7 +96,22 @@ function run(cwd: string, file: string, text: string, settings: Settings): Outco const relative = relativePath(ds.root, file); const result = ds.check(text, relative); if (result.skipped) return { skipped: true }; - return { result, fromSystem: (specifier) => ds.lint.isDesignSystemImport(specifier, relative) }; + return { + result, + fromSystem: (specifier) => ds.lint.isDesignSystemImport(specifier, relative), + checkWith(id, options) { + // A rule the project's config turns off for the file stays off. + const own = rulesFor(ds.config, relative)[id]; + const off: ResolvedRule = { severity: 'off', options: {} }; + const rules = Object.fromEntries( + RULE_IDS.map((rule) => [ + rule, + rule === id ? { ...own, options: { ...own.options, ...options } } : off, + ]), + ) as Record; + return checkSource(text, ds.lint, { filename: relative, rules }); + }, + }; } /** The part of an ESLint AST node this file reads; JSX nodes are not in ESTree's types. */ @@ -158,18 +183,34 @@ function typeOnly(def: Scope.Definition): boolean { return def.type === 'ImportBinding' && (kind(def.node) === 'type' || kind(def.parent) === 'type'); } -function createRule(id: RuleId, description: string): Rule.RuleModule { +/** A rule's options in ESLint's config, as in onsystem.config.json: `['error', { allow: ['#fff'] }]`. */ +const OPTIONS_SCHEMA: Rule.RuleMetaData['schema'] = [ + { + type: 'object', + properties: { allow: { type: 'array', items: { type: 'string' } } }, + additionalProperties: false, + }, +]; + +/** + * Check's misspelled rule names in `// onsystem-disable-*` comments, which suppress nothing. A rule + * of its own, so that it is a warning, as check reports it, and can be turned off like the others. + */ +const SUPPRESSION = 'suppression'; + +function createRule(id: RuleId | typeof SUPPRESSION, description: string): Rule.RuleModule { + const suppression = id === SUPPRESSION; return { meta: { type: 'problem', - docs: { description, url: `${RULES_PAGE}#${id}` }, - fixable: 'code', - hasSuggestions: true, - schema: [], + docs: { description, url: `${RULES_PAGE}#${suppression ? 'suppressing-findings' : id}` }, + ...(suppression ? {} : { fixable: 'code', hasSuggestions: true }), + schema: suppression ? [] : OPTIONS_SCHEMA, }, create(context) { const file = path.resolve(context.cwd, context.filename); const settings = (context.settings.onsystem ?? {}) as Settings; + const options = (context.options as RuleOptions[] | undefined)?.[0]; return { 'Program:exit'(node) { const pass = passFor(context, file, settings); @@ -185,7 +226,11 @@ function createRule(id: RuleId, description: string): Rule.RuleModule { }); return; } - for (const d of outcome.result.diagnostics) { + const { diagnostics } = + !suppression && options && Object.keys(options).length + ? outcome.checkWith(id, options) + : outcome.result; + for (const d of diagnostics) { if (d.ruleId !== id) continue; const edits = d.fix ?? []; const fix: Rule.ReportFixer = (fixer) => @@ -218,6 +263,7 @@ function createRule(id: RuleId, description: string): Rule.RuleModule { const rules = Object.fromEntries( RULES.map((rule) => [rule.id, createRule(rule.id, rule.description)]), ); +rules[SUPPRESSION] = createRule(SUPPRESSION, 'Suppression comments must name rules that exist.'); const plugin = { meta: { name: NAME, version: VERSION }, @@ -226,17 +272,18 @@ const plugin = { } satisfies ESLint.Plugin; /** - * Every rule at its default severity, for `.tsx` and `.jsx` files other than tests and stories, - * which `check` also leaves out unless asked (`--include-tests`). + * Every rule at its default severity, and `suppression` as a warning, for `.tsx` and `.jsx` files + * other than tests and stories, which `check` also leaves out unless asked (`--include-tests`). */ plugin.configs.recommended = { name: 'onsystem/recommended', files: ['**/*.{tsx,jsx}'], ignores: [DEFAULT_TEST_EXCLUDE, DEFAULT_TEST_FOLDERS], plugins: { onsystem: plugin }, - rules: Object.fromEntries( - RULES.map((rule) => [`onsystem/${rule.id}`, DEFAULT_SEVERITY[rule.id]]), - ), + rules: { + ...Object.fromEntries(RULES.map((rule) => [`onsystem/${rule.id}`, DEFAULT_SEVERITY[rule.id]])), + [`onsystem/${SUPPRESSION}`]: 'warn', + }, }; export default plugin; diff --git a/src/eslint/worker.ts b/src/eslint/worker.ts index e8d8002c..2b30cab2 100644 --- a/src/eslint/worker.ts +++ b/src/eslint/worker.ts @@ -2,13 +2,20 @@ * The ESLint plugin's loader, in a worker thread. ESLint rules run synchronously, and finding a * project's design system does not (config files written in code, globs, the extraction * cache), so the plugin asks this worker and waits for the answer (see `bridge.ts`). + * + * The worker keeps what it loaded and answers from it at once. Once that is two seconds old, the + * next lint starts a look at the project again (its config, then its design system's files) in + * the background, and the lints after the look get what it found. Waiting for the look instead + * held up every lint after a pause in an editor for as long as the look took, which grows with + * the design system. */ +import { createHash } from 'node:crypto'; import fs from 'node:fs'; import path from 'node:path'; import { parentPort, type MessagePort } from 'node:worker_threads'; import { excludedBy, type ResolvedConfig } from '../config.js'; -import { buildModel, componentFiles } from '../design-system.js'; +import { buildModel, componentFiles, fingerprint } from '../design-system.js'; import type { DesignSystemModel } from '../types.js'; import { relativePath } from '../util/paths.js'; import { loadTarget } from '../workspace.js'; @@ -50,20 +57,42 @@ export interface Call { /** What the configs said (a design-system package's skipped, or its paths ignored), each said once. */ const warned = new Set(); -/** How long a loaded design system is trusted before its files are looked at again. */ +/** How long what a look found is used before a lint starts the next look. */ const FRESH_MS = 2000; -const targets = new Map }>(); -const projects = new Map< - string, - { - at: number; - version: string; - config: ResolvedConfig; - model: DesignSystemModel; - own: Set; - } ->(); +/** The project and config found from where ESLint runs and `settings.onsystem`, and when. */ +interface KeptTarget { + at: number; + /** A load that failed is kept too: the lints until the next look say why. */ + target: Promise; + /** A look at it, then at the project of the file whose lint started the look, under way. */ + looking?: Promise | undefined; +} + +/** A project's design system, as built for `config`. */ +interface Project { + at: number; + /** The fingerprint of what the model is built from: the files, and the config but its rules. */ + key: string; + /** Files beyond the tracked ones that the model depends on, for the next fingerprint. */ + dependencies: string[]; + /** The fingerprint and the whole config, rules too: what the plugin holds when it has it. */ + version: string; + config: ResolvedConfig; + model: DesignSystemModel; + /** The design system's own component files, by real path. */ + own: Set; + looking?: Promise | undefined; +} + +/** By where ESLint runs and its settings. */ +const targets = new Map(); +/** + * By target and project root: two targets can reach the same project with configs of their own + * (runs from two folders, two ESLint configs), and neither may be answered with the other's. + */ +const projects = new Map(); +const looks = new Set>(); function realPath(file: string): string { try { @@ -73,24 +102,104 @@ function realPath(file: string): string { } } -async function targetFor({ cwd, root, config }: LoadRequest): Promise { - const key = JSON.stringify([cwd, root, config]); - const cached = targets.get(key); - if (cached && Date.now() - cached.at < FRESH_MS) return cached.target; - const target = loadTarget({ +function loadTargetFor({ cwd, root, config }: LoadRequest): Promise { + return loadTarget({ cwd, root: root === undefined ? undefined : path.resolve(cwd, root), config: config === undefined ? undefined : path.resolve(cwd, config), }); - targets.set(key, { at: Date.now(), target }); - return target; +} + +function keep(target: Promise): KeptTarget { + // Awaited by later lints, which say why it failed: not an unhandled rejection meanwhile. + target.catch(() => undefined); + return { at: Date.now(), target }; +} + +/** The config a file is checked with: its own project's, from a monorepo root. */ +async function configFor(target: Target, file: string): Promise { + return target.workspace ? (await target.workspace.projectFor(file)).config : target.config; +} + +function versionOf(key: string, config: ResolvedConfig): string { + return createHash('sha256').update(key).update(JSON.stringify(config)).digest('hex'); +} + +async function build(config: ResolvedConfig): Promise { + const { + model, + key = String(Date.now()), + dependencies = [], + } = await buildModel(config, { cache: true }); + const own = new Set((await componentFiles(config)).map(realPath)); + return { at: Date.now(), key, dependencies, version: versionOf(key, config), config, model, own }; +} + +/** + * `project` for `config` as the files are now: the same model when nothing it is built from has + * changed (a config edit that only changes rules, or no change at all), else a new build. + */ +async function rebuild(project: Project, config: ResolvedConfig): Promise { + if ((await fingerprint(config, project.dependencies)) !== project.key) return build(config); + const { key, dependencies, model, own } = project; + return { at: Date.now(), key, dependencies, version: versionOf(key, config), config, model, own }; +} + +/** Runs a look in the background, held in `looks` until it ends. */ +function background(look: () => Promise): Promise { + const running: Promise = look() + .catch(() => undefined) + .finally(() => looks.delete(running)); + looks.add(running); + return running; +} + +function projectKey(key: string, config: ResolvedConfig): string { + return JSON.stringify([key, config.root]); +} + +/** A build that fails drops the project, so that the next lint builds it again and says why. */ +function lookAtProject(slot: string, project: Project, config: ResolvedConfig): Promise { + project.looking = background(async () => { + try { + projects.set(slot, await rebuild(project, config)); + } catch (error) { + if (projects.get(slot) === project) projects.delete(slot); + throw error; + } + }); + return project.looking; +} + +function lookAtTarget(key: string, kept: KeptTarget, request: LoadRequest): Promise { + kept.looking = background(async () => { + const loading = loadTargetFor(request); + await loading.catch(() => undefined); + targets.set(key, keep(loading)); + // The config may have changed: the file's project is looked at with the new one. + const config = await configFor(await loading, request.file); + const slot = projectKey(key, config); + const project = projects.get(slot); + if (project && !project.looking) await lookAtProject(slot, project, config); + }); + return kept.looking; +} + +/** Resolves when the looks under way have ended. Nothing waits on them but tests. */ +export function looking(): Promise { + return Promise.all(looks); } export async function load(request: LoadRequest): Promise { - const target = await targetFor(request); - const config = target.workspace - ? (await target.workspace.projectFor(request.file)).config - : target.config; + const key = JSON.stringify([request.cwd, request.root, request.config]); + let kept = targets.get(key); + if (!kept) { + kept = keep(loadTargetFor(request)); + targets.set(key, kept); + } else if (!kept.looking && Date.now() - kept.at >= FRESH_MS) { + void lookAtTarget(key, kept, request); + } + const config = await configFor(await kept.target, request.file); // On stderr, once per process: ESLint has no place for a warning that is about no one file. for (const text of config.warnings ?? []) { @@ -99,12 +208,19 @@ export async function load(request: LoadRequest): Promise { process.stderr.write(`onsystem: ${text}\n`); } - let project = projects.get(config.root); - if (!project || Date.now() - project.at >= FRESH_MS) { - const { model, key } = await buildModel(config, { cache: true }); - const own = new Set((await componentFiles(config)).map(realPath)); - project = { at: Date.now(), version: key ?? String(Date.now()), config, model, own }; - projects.set(config.root, project); + const slot = projectKey(key, config); + let project = projects.get(slot); + if (!project) { + project = await build(config); + projects.set(slot, project); + } else if ( + !kept.looking && + !project.looking && + (project.config !== config || Date.now() - project.at >= FRESH_MS) + ) { + // Another project of a monorepo root, whose config a look at the root found again, or whose + // design system's files are due a look. + void lookAtProject(slot, project, config); } // What `check .` leaves out of a folder: the config's `exclude`, by the same test, and the diff --git a/test/eslint-bridge.test.ts b/test/eslint-bridge.test.ts new file mode 100644 index 00000000..31ba7787 --- /dev/null +++ b/test/eslint-bridge.test.ts @@ -0,0 +1,316 @@ +import fs from 'node:fs'; +import path from 'node:path'; +import { pathToFileURL } from 'node:url'; + +import { ESLint, type Linter } from 'eslint'; +import { build } from 'tsdown'; +import tseslint from 'typescript-eslint'; +import { afterEach, beforeAll, describe, expect, it, vi } from 'vitest'; + +import { main } from '../src/cli.js'; +import { load, looking, type LoadRequest } from '../src/eslint/worker.js'; +import { fixture } from './helpers.js'; + +// What the ESLint plugin carries over from the project and from ESLint's config in a process that +// runs on, as an editor's ESLint server does: config edits, rule options and suppression typos, +// and a design system that is looked at again without holding up a lint. Built as it ships, as in +// eslint.test.ts, into a folder of its own, since test files run in parallel. +const REPO = path.resolve(import.meta.dirname, '..'); +const OUT = path.join(REPO, 'node_modules/.cache/onsystem-eslint-bridge-test'); + +let onsystem: { rules: Record; configs: { recommended: Linter.Config } }; + +beforeAll(async () => { + await build({ + cwd: REPO, + config: false, + entry: { eslint: 'src/eslint/index.ts', 'eslint-worker': 'src/eslint/worker.ts' }, + outDir: OUT, + format: 'esm', + platform: 'node', + target: 'node22.18', + fixedExtension: false, + dts: false, + publint: false, + clean: true, + logLevel: 'silent', + }); + ({ default: onsystem } = (await import(pathToFileURL(path.join(OUT, 'eslint.js')).href)) as { + default: typeof onsystem; + }); +}); + +/** The recommended config with typescript-eslint's parser, then `extra` after it. */ +function eslintFor(cwd: string, extra: Linter.Config = {}): ESLint { + return new ESLint({ + cwd, + overrideConfigFile: true, + overrideConfig: [ + { files: ['**/*.{ts,tsx,js,jsx}'], languageOptions: { parser: tseslint.parser } }, + onsystem.configs.recommended, + extra, + ], + }); +} + +/** + * `file:line:column rule` for each finding, as `checkJson` gives them. Not the severity: ESLint's + * config sets it, where check reports a color of a hue no token has as a warning. + */ +async function lint(eslint: ESLint, cwd: string, files: string[]): Promise { + const results = await eslint.lintFiles(files); + return results.flatMap((r) => + r.messages.map( + (m) => + `${path.relative(cwd, r.filePath).split(path.sep).join('/')}:${m.line}:${m.column} ${m.ruleId ?? 'fatal'}`, + ), + ); +} + +async function checkJson(cwd: string, args: string[]) { + const out: string[] = []; + await main(['check', ...args, '--format', 'json', '--no-cache'], { + cwd, + color: false, + stdout: (t) => out.push(t), + stderr: () => undefined, + }); + const results = JSON.parse(out.join('\n')) as { + file: string; + diagnostics: { ruleId: string; line: number; column: number }[]; + }[]; + return results.flatMap((r) => + r.diagnostics.map((d) => `${r.file}:${d.line}:${d.column} onsystem/${d.ruleId}`), + ); +} + +const sleep = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)); + +const BUTTON = + 'export function Button(props: React.ComponentProps<"button">) { return