docs: unwrap the ESLint design doc (#60)

Hard-wrapped at 100 columns, which assumes a viewer width neither Gitea's web UI nor the VS Code markdown preview has, so the wrap points landed mid-sentence for the person reading it. Paragraphs and list items are now one line each; tables, code fences and the header block keep their own breaks because those are structure rather than wrapped prose.

Refs #60
This commit is contained in:
2026-08-19 13:44:12 -05:00
parent 8e859e58ec
commit 3cb6a42fb3
@@ -1,25 +1,16 @@
# ESLint — Design # ESLint — Design
**Issue:** [#60 — No ESLint anywhere](https://gitea.bermudalamb.synology.me/bermudalamb/redefined-designs/issues/60) **Issue:** [#60 — No ESLint anywhere](https://gitea.bermudalamb.synology.me/bermudalamb/redefined-designs/issues/60) **Date:** 2026-08-19 **Status:** Approved
**Date:** 2026-08-19
**Status:** Approved
## Goal ## Goal
Put a linter in front of the code that catches, at authoring time, the classes of defect this Put a linter in front of the code that catches, at authoring time, the classes of defect this project has already shipped and then fixed by hand. Two workspaces, one CI job, green on the day it lands.
project has already shipped and then fixed by hand. Two workspaces, one CI job, green on the day it
lands.
The issue's own framing is the target: TypeScript's `strict: true` checks types and nothing else, so The issue's own framing is the target: TypeScript's `strict: true` checks types and nothing else, so `rules-of-hooks`, `exhaustive-deps`, `no-floating-promises` and the SonarJS rules currently run either never or on a server after the fact.
`rules-of-hooks`, `exhaustive-deps`, `no-floating-promises` and the SonarJS rules currently run
either never or on a server after the fact.
## What the code actually reports ## What the code actually reports
Every number below comes from running the candidate rule set against `backend/src` and Every number below comes from running the candidate rule set against `backend/src` and `frontend/src` before writing any config, rather than from estimating. A full-strength config — `typescript-eslint` recommendedTypeChecked, `sonarjs`, `react-hooks` v7, `jsx-a11y` — reports **435 violations across 50 files**.
`frontend/src` before writing any config, rather than from estimating. A full-strength config —
`typescript-eslint` recommendedTypeChecked, `sonarjs`, `react-hooks` v7, `jsx-a11y` — reports **435
violations across 50 files**.
| Rule family | Count | Disposition | | Rule family | Count | Disposition |
| --- | --- | --- | | --- | --- | --- |
@@ -37,17 +28,9 @@ violations across 50 files**.
### Two of the issue's premises do not survive measurement ### Two of the issue's premises do not survive measurement
**`exhaustive-deps` flags 2, not 10.** The issue inferred ten suspects from ten empty dependency **`exhaustive-deps` flags 2, not 10.** The issue inferred ten suspects from ten empty dependency arrays. The rule's own analysis clears eight of them — an empty array is only wrong when the effect closes over something that changes, and most of these genuinely do not. The two real ones are `Admin.tsx:62` (missing `load`) and `Cart.tsx:132` (missing `refreshCartContext`). The "decide case by case rather than blanket-autofix" instinct in the issue is still right; it applies to two cases.
arrays. The rule's own analysis clears eight of them — an empty array is only wrong when the effect
closes over something that changes, and most of these genuinely do not. The two real ones are
`Admin.tsx:62` (missing `load`) and `Cart.tsx:132` (missing `refreshCartContext`). The "decide case
by case rather than blanket-autofix" instinct in the issue is still right; it applies to two cases.
**The backend is already clean on the defect rules.** #59 wrapped every async route, and it shows: **The backend is already clean on the defect rules.** #59 wrapped every async route, and it shows: zero floating promises in `backend/src`. Its three `no-misused-promises` hits are all benign, and each was read rather than assumed — `asyncRoute.ts:13` is the wrapper performing its entire purpose, and the two `server.ts` scheduled jobs (`setInterval` cart sweep, `cron.schedule` reminder emails) already `try`/`catch` internally, so no rejection escapes.
zero floating promises in `backend/src`. Its three `no-misused-promises` hits are all benign, and
each was read rather than assumed — `asyncRoute.ts:13` is the wrapper performing its entire purpose,
and the two `server.ts` scheduled jobs (`setInterval` cart sweep, `cron.schedule` reminder emails)
already `try`/`catch` internally, so no rejection escapes.
## Decisions ## Decisions
@@ -64,64 +47,40 @@ already `try`/`catch` internally, so no rejection escapes.
### Why not `recommendedTypeChecked` ### Why not `recommendedTypeChecked`
It is the obvious choice and it is the wrong one here. Its `no-unsafe-*` family accounts for 325 of It is the obvious choice and it is the wrong one here. Its `no-unsafe-*` family accounts for 325 of the 435 findings — 75% — and every one of them traces back to two untyped boundaries: `pool.query()` returning `any` rows on the backend, and `fetch(...).json()` on the frontend. Fixing them means typing those boundaries, which is the entire content of [#65](https://gitea.bermudalamb.synology.me/bermudalamb/redefined-designs/issues/65).
the 435 findings — 75% — and every one of them traces back to two untyped boundaries: `pool.query()`
returning `any` rows on the backend, and `fetch(...).json()` on the frontend. Fixing them means
typing those boundaries, which is the entire content of
[#65](https://gitea.bermudalamb.synology.me/bermudalamb/redefined-designs/issues/65).
Turning them on here would produce a lint run whose output is 75% another issue's backlog, which is Turning them on here would produce a lint run whose output is 75% another issue's backlog, which is the reliable way to teach everyone to ignore lint output. Instead this issue enables `recommended` plus the two type-aware rules that catch defects rather than describe type debt:
the reliable way to teach everyone to ignore lint output. Instead this issue enables `recommended`
plus the two type-aware rules that catch defects rather than describe type debt:
```js ```js
'@typescript-eslint/no-floating-promises': 'error', '@typescript-eslint/no-floating-promises': 'error',
'@typescript-eslint/no-misused-promises': ['error', { checksVoidReturn: { attributes: false } }], '@typescript-eslint/no-misused-promises': ['error', { checksVoidReturn: { attributes: false } }],
``` ```
Both need type information, so `parserOptions.projectService` is still required — the cost of Both need type information, so `parserOptions.projectService` is still required — the cost of type-aware linting is paid either way. This is a choice about which findings to surface, not about whether to run the type-aware engine.
type-aware linting is paid either way. This is a choice about which findings to surface, not about
whether to run the type-aware engine.
#65 turns the `no-unsafe-*` rules on as it types those boundaries. That is the change that makes Issue #65 turns the `no-unsafe-*` rules on as it types those boundaries. That is the change that makes them green, so that is the change that should own them.
them green, so that is the change that should own them.
### Why `checksVoidReturn: { attributes: false }` ### Why `checksVoidReturn: { attributes: false }`
Left at its default, `no-misused-promises` flags every `onClick={async () => …}` — 25 of its 28 Left at its default, `no-misused-promises` flags every `onClick={async () => …}` — 25 of its 28 hits, all antd buttons in the admin screens. Passing an async function to a React event handler is idiomatic and safe when the function handles its own errors, which these do. typescript-eslint's own documentation recommends this option for React codebases. Without it the rule is 89% noise and would be switched off within a week; with it, the three remaining hits are all real signal.
hits, all antd buttons in the admin screens. Passing an async function to a React event handler is
idiomatic and safe when the function handles its own errors, which these do. typescript-eslint's own
documentation recommends this option for React codebases. Without it the rule is 89% noise and would
be switched off within a week; with it, the three remaining hits are all real signal.
### Why the React Compiler rules only warn ### Why the React Compiler rules only warn
`eslint-plugin-react-hooks` v7 ships the React Compiler rule set — `set-state-in-effect`, `purity`, `eslint-plugin-react-hooks` v7 ships the React Compiler rule set — `set-state-in-effect`, `purity`, `static-components` and others — alongside the two classic rules. This is React 18 with no compiler in the build, so those rules are advising on a stricter model than the code is written against.
`static-components` and others — alongside the two classic rules. This is React 18 with no compiler
in the build, so those rules are advising on a stricter model than the code is written against.
They are not worthless: `purity` correctly flags `Date.now()` being called during render in They are not worthless: `purity` correctly flags `Date.now()` being called during render in `Cart.tsx:160`. That is a genuine render-purity smell worth seeing. It is not worth failing a build over in a codebase that never opted into the model, and fixing all nine would mean restructuring effects that behave correctly today — behaviour change well outside what #60 asked for.
`Cart.tsx:160`. That is a genuine render-purity smell worth seeing. It is not worth failing a build
over in a codebase that never opted into the model, and fixing all nine would mean restructuring
effects that behave correctly today — behaviour change well outside what #60 asked for.
Warn keeps the finding visible and the build honest. Warn keeps the finding visible and the build honest.
### Why per-workspace configs rather than one at the root ### Why per-workspace configs rather than one at the root
CI already runs `npm install` separately in `backend` and `frontend`, and the two have genuinely CI already runs `npm install` separately in `backend` and `frontend`, and the two have genuinely different rule needs — React and a11y rules are meaningless in `backend/src`, and each workspace has its own `tsconfig.json` that type-aware linting must point at. A root config would need file-pattern overrides to express a split the directory structure already expresses, and would put React plugins in the backend's dependency tree for no reason.
different rule needs — React and a11y rules are meaningless in `backend/src`, and each workspace has
its own `tsconfig.json` that type-aware linting must point at. A root config would need file-pattern
overrides to express a split the directory structure already expresses, and would put React plugins
in the backend's dependency tree for no reason.
The cost is a small amount of duplication between the two config files. That is cheaper than the The cost is a small amount of duplication between the two config files. That is cheaper than the alternative, and the files are short.
alternative, and the files are short.
## Fail/warn split ## Fail/warn split
The issue asks whether to fail CI on day one or start as warnings, and names the middle path: fail The issue asks whether to fail CI on day one or start as warnings, and names the middle path: fail on the rules with real defect-catching value, warn on the rest. That is what this does.
on the rules with real defect-catching value, warn on the rest. That is what this does.
**Error — build fails (≈37 sites to fix):** **Error — build fails (≈37 sites to fix):**
@@ -133,24 +92,17 @@ on the rules with real defect-catching value, warn on the rest. That is what thi
| `jsx-a11y/alt-text` | 2 | Add `alt` | | `jsx-a11y/alt-text` | 2 | Add `alt` |
| `rules-of-hooks` | 0 | — | | `rules-of-hooks` | 0 | — |
**Warn — visible, does not block (≈73 sites):** SonarJS complexity and style, remaining jsx-a11y, **Warn — visible, does not block (≈73 sites):** SonarJS complexity and style, remaining jsx-a11y, React Compiler rules, `no-explicit-any`.
React Compiler rules, `no-explicit-any`.
No `--max-warnings` flag is needed: ESLint exits non-zero on errors and zero on warnings, so the No `--max-warnings` flag is needed: ESLint exits non-zero on errors and zero on warnings, so the split falls out of rule severity alone.
split falls out of rule severity alone.
### On `void load()` being a real fix rather than a disguised suppression ### On `void load()` being a real fix rather than a disguised suppression
Thirty floating promises sounds like thirty bugs; it is not, and the distinction matters because Thirty floating promises sounds like thirty bugs; it is not, and the distinction matters because `void` can be either an honest annotation or a way to silence a rule without thinking.
`void` can be either an honest annotation or a way to silence a rule without thinking.
These are fire-and-forget calls to `load()`-style `useCallback`s in effects and event handlers. The These are fire-and-forget calls to `load()`-style `useCallback`s in effects and event handlers. The callbacks were read before choosing this fix: each one sets its own error state — `App.tsx`'s `load` calls `setFailed(true)` — so a rejection is already handled inside, and nothing is being swallowed. `void` states "deliberately not awaited," which is exactly true here.
callbacks were read before choosing this fix: each one sets its own error state — `App.tsx`'s `load`
calls `setFailed(true)` — so a rejection is already handled inside, and nothing is being swallowed.
`void` states "deliberately not awaited," which is exactly true here.
Had any of them lacked internal handling, the fix for that one would be a `.catch`, not a `void`. Had any of them lacked internal handling, the fix for that one would be a `.catch`, not a `void`. The implementation plan checks each site rather than applying `void` mechanically.
The implementation plan checks each site rather than applying `void` mechanically.
## Files ## Files
@@ -172,28 +124,20 @@ The linter is its own test: `npm run lint` in each workspace must exit 0. Succes
1. `npm run lint` passes in both workspaces with zero errors. 1. `npm run lint` passes in both workspaces with zero errors.
2. The warning count is reported, not zero, and that is expected. 2. The warning count is reported, not zero, and that is expected.
3. `npm run build` still passes in both workspaces. 3. `npm run build` still passes in both workspaces.
4. Backend unit and integration suites still pass — the `void` and disable edits touch frontend 4. Backend unit and integration suites still pass — the `void` and disable edits touch frontend files almost exclusively, but `server.ts` and `asyncRoute.ts` are on the backend.
files almost exclusively, but `server.ts` and `asyncRoute.ts` are on the backend.
5. Playwright e2e passes, since ≈37 edits land in rendered components. 5. Playwright e2e passes, since ≈37 edits land in rendered components.
6. The `lint` job runs in CI and fails the build when a rule in the error set is violated. 6. The `lint` job runs in CI and fails the build when a rule in the error set is violated.
Point 6 is verified by deliberately introducing a violation locally and confirming a non-zero exit, Point 6 is verified by deliberately introducing a violation locally and confirming a non-zero exit, not by assuming the job's configuration is correct.
not by assuming the job's configuration is correct.
## Out of scope ## Out of scope
- **The `no-unsafe-*` family and the `any` backlog** — #65. - **The `no-unsafe-*` family and the `any` backlog** — #65.
- **Prettier or any formatting tool.** The issue asks for defect detection. Formatting is a separate - **Prettier or any formatting tool.** The issue asks for defect detection. Formatting is a separate argument with separate churn.
argument with separate churn. - **Linting `tests/` and Playwright specs.** Worth doing; a second step once the source is green, so that this change's error count stays the measured one.
- **Linting `tests/` and Playwright specs.** Worth doing; a second step once the source is green, so - **Replacing SonarQube.** The CI scan stays. This puts the same rule families in the editor, where they are cheaper to act on.
that this change's error count stays the measured one.
- **Replacing SonarQube.** The CI scan stays. This puts the same rule families in the editor, where
they are cheaper to act on.
## Follow-ups this creates ## Follow-ups this creates
- **#65 owns turning on `no-unsafe-*`** as it types `pool.query` results and `fetch` responses. - **#65 owns turning on `no-unsafe-*`** as it types `pool.query` results and `fetch` responses.
- **`routesAreWrapped.test.ts` can retire** once `no-misused-promises` runs on the backend in CI. - **`routesAreWrapped.test.ts` can retire** once `no-misused-promises` runs on the backend in CI. The test predates the linter and covers the same ground by scanning source; it is more targeted and its failure message is better, so this is a judgement call for whoever next touches either. Recorded here so the duplication is deliberate rather than forgotten.
The test predates the linter and covers the same ground by scanning source; it is more targeted
and its failure message is better, so this is a judgement call for whoever next touches either.
Recorded here so the duplication is deliberate rather than forgotten.