Skip to content

ENG-2118 Investigate whether to upgrade repo ESLint from version 8 to 9 - #1293

Draft
trangdoan982 wants to merge 1 commit into
mainfrom
eng-2118-investigate-whether-to-upgrade-repo-eslint-from-version-8-to
Draft

ENG-2118 Investigate whether to upgrade repo ESLint from version 8 to 9#1293
trangdoan982 wants to merge 1 commit into
mainfrom
eng-2118-investigate-whether-to-upgrade-repo-eslint-from-version-8-to

Conversation

@trangdoan982

@trangdoan982 trangdoan982 commented Aug 13, 2026

Copy link
Copy Markdown
Member

Scope check

  • Ran $scope-check against the ENG ticket and final diff.
  • Scope beyond Done When:

Yes — deliberately, and this PR is not meant to merge as-is.

ENG-2118 lists "upgrade directly here" as out of scope, and its Done When is a written recommendation. This PR carries the working upgrade anyway, as a reference implementation, for two reasons:

  1. The repo has no docs/ or RFC convention, so the findings have no natural home as a committed file — they live in this PR body instead.
  2. "Will anything break?" is far more credible as a reviewable diff with CI actually running against it than as prose.

If the recommendation is accepted, the upgrade should land under its own ticket (blocking ENG-1832), and this PR should be closed rather than merged. Keeping it draft to make that explicit.


Recommendation: upgrade

Measured empirically by performing the upgrade in a worktree off main @ 5f4d9ea0, not inferred from release notes.

Why now

eslint-plugin-obsidianmd@0.4.1, needed by ENG-1832, requires:

Peer Required On main today
eslint >=9.19.0 8.57.1 ❌
@eslint/js ^9.30.1 9.34.0 ✅
typescript-eslint ^8.35.1 7.18.0 ❌

Verified: with this branch applied, pnpm add -D eslint-plugin-obsidianmd installs with zero ESLint-related peer warnings. (That probe was removed from the diff — it belongs to ENG-1832.)

Why it's cheaper than it looks

The expensive part of an 8→9 migration is the flat-config rewrite, and that is already done. All 7 linted workspaces use eslint.config.mjs, there are zero .eslintrc* files, and ESLint is pinned in exactly one catalog: entry.

Only one dependency genuinely blocks v9: typescript-eslint@7.18.0 (peers eslint ^8.56.0). Every other plugin already declares v9 support. The one other blocker, @vercel/style-guide@6.0.0 (peers eslint <9), turns out to be dead — declared but never imported. Deleting it removes every remaining peer conflict.

Also worth noting: @eslint/js is already at 9.34.0 while eslint is 8.57.1. That mismatch is latent today; this resolves it. And 8.57.1 is end-of-life, currently parked in allowedDeprecatedVersions.

Will anything break? No — for normal linting

CI is structurally protected. lint-changed-files doesn't lint the repo — it lints changed files and pipes them through reviewdog with -filter-mode=added -fail-level=warning. Only warnings on lines a PR adds can fail. The new warnings land on untouched lines and are invisible to CI. No existing PR starts failing.

This answers the ticket's own question — "current CI only fails for newly added files? is this good enough of a stop gap?" — with yes, and stronger: newly added lines, not just files.

Everything else checks out:

  • turbo check-types8/8 pass on this branch.
  • No source file anywhere imports eslint or @types/eslint. (That catalog entry is dead and could be dropped outright.)
  • lint-staged runs prettier only, not ESLint — no new pre-commit friction.
  • ESLint is a devDependency: zero runtime or shipped-artifact impact.

What actually changes: 573 → 754 warnings

Full-repo lint before and after. Errors stay at 0 in both (the onlyWarn plugin downgrades everything), every workspace exits 0, and both runs linted the same 515 files — apples to apples.

Workspace ESLint 8 ESLint 9
apps/roam 431 538
apps/obsidian 71 111
apps/website 60 86
packages/database 7 15
packages/ui 4 4
packages/content-model 0 0
packages/utils 0 0
TOTAL 573 754

Where +181 comes from

Rule v8 v9 Δ
@typescript-eslint/no-unnecessary-type-assertion 7 79 +72
unused eslint-disable directives 0 62 +62
@typescript-eslint/no-unused-vars 29 61 +32
@typescript-eslint/no-base-to-string 1 9 +8
@typescript-eslint/no-require-imports 0 5 +5
@typescript-eslint/prefer-promise-reject-errors 0 3 +3
@typescript-eslint/no-unused-expressions 0 3 +3
@typescript-eslint/await-thenable 1 3 +2
@typescript-eslint/no-empty-object-type 0 1 +1
@typescript-eslint/restrict-template-expressions 14 13 −1
@typescript-eslint/ban-types 1 0 −1
@typescript-eslint/no-var-requires 5 0 −5

Three are just renames, not new findings: ban-typesno-empty-object-type, no-var-requiresno-require-imports.

Two of these are explained directly by the release notes:

  • The 62 unused-directive reports are new v9 default behavior (linterOptions.reportUnusedDisableDirectives now defaults to "warn"; it was off in v8). Pure cleanup — they flag eslint-disable comments that no longer suppress anything.
  • The +32 unused vars are the no-unused-vars caughtErrors default flipping "none""all" in ESLint 9, which typescript-eslint v8 realigned to match. Settable back with caughtErrors: 'none' if we want v7 behavior.

⚠️ eslint --fix silently breaks the roam build

This is the one genuine hazard, and it is not visible in lint output.

ESLint 8 ESLint 9
eslint . 573 warnings 754 warnings
after real eslint --fix 534 warnings (−39) 612 warnings (−142)
turbo check-types after --fix ✅ passes roam fails: 25 type errors / 10 files

Under ESLint 8, --fix is benign — it modestly improves things and the tree still compiles.

Under ESLint 9, --fix touches 83 files and makes lint look better (754 → 612, still 0 errors, every workspace exits 0) while breaking tsc:

14 × TS18046  'result' is of type 'unknown'
 5 × TS7053   Element implicitly has an 'any' type ... no index signature
 5 × TS2345   Argument of type ... is not assignable
 1 × TS2769   No overload matches this call

Cause: no-unnecessary-type-assertion autofix strips assertions that were narrowing any out of untyped host APIs — gray-matter's data, Obsidian's frontmatter, Roam query results:

- frontmatter: (data ?? {}) as ParsedFrontmatter,
+ frontmatter: (data ?? {}),

- const frontmatter = cache?.frontmatter as Record<string, unknown> | undefined;
+ const frontmatter = cache?.frontmatter;

The rule is locally correct — the assertion is redundant to ESLint. But the assertion is load-bearing: it's how we pin down an any at the boundary. Removing it propagates unknown/any across module boundaries and the consumers stop compiling.

Mitigations, in order of preference:

  1. Simply don't run eslint --fix repo-wide during the upgrade. Fix the 62 stale directives by hand or with a targeted --rule filter.
  2. turbo check-types already runs in CI (ci.yaml:35), so this cannot reach main silently — but it will waste someone's afternoon if they run --fix locally and don't connect the two.
  3. Note that disabling no-unnecessary-type-assertion is not sufficient on its own — I tested it, and other autofixes still degrade types. Treat --fix as unsafe wholesale rather than trying to allowlist around it.

packages/database ships a committed lint:fix script (eslint --fix . && …). I checked it specifically: it is safe — 15 → 10 warnings, no type errors in that package. The hazard is scoped to roam.

Pros and cons

Pros

  • Unblocks ENG-1832, the reason this came up.
  • Off end-of-life 8.57.1 and out of allowedDeprecatedVersions.
  • Resolves the latent @eslint/js@9.34.0 / eslint@8.57.1 mismatch.
  • Removes two dead dependencies.
  • typescript-eslint v8 genuinely catches more (no-base-to-string, prefer-promise-reject-errors, 32 more unused vars).
  • Small and reversible — this diff is ~30 lines outside the lockfile.

Cons

  • +181 warnings on the board. Invisible to CI, but they surface the next time someone edits those lines.
  • 62 stale eslint-disable comments want a mechanical cleanup pass.
  • eslint --fix becomes a foot-gun that breaks the roam build without failing lint. Needs documenting.
  • Incidental cleanup may attach itself to unrelated future PRs that touch affected lines.

Why 9 and not 10

ESLint 10.8.1 exists, but eslint-plugin-react@7.37.5 declares eslint: "^3 || … || ^9.7" — it does not permit v10. Nine is the right target today.

Useful de-risking option: typescript-eslint@8.67.0 peers eslint ^8.57.0 || ^9.0.0 || ^10.0.0. The 7→8 bump can ship on its own first, while still on ESLint 8, making the v9 bump a near-trivial follow-up.


Appendix A — breaking changes hit, and their fixes

All three were encountered for real, in this order:

  1. tsconfigRootDir must be absolute. typescript-eslint v8 rejects ".", which v7 accepted and resolved against process.cwd(). Symptom: 515 fatal parse errors — the entire repo fails to lint. Fix: import.meta.dirname (safe; repo requires Node ≥22). This is also strictly more correct, since the old value only worked because turbo lint happens to run eslint . from inside each workspace.
  2. Type-aware config must be scoped to TS files. With project: true on every file, v8 errors on .mjs absent from any tsconfig (packages/database/src/dbDotEnv.mjs) where v7 skipped it. Fix: files: ["**/*.{ts,tsx,mts,cts}"] plus disableTypeChecked for plain JS. The glob must include .mtspackages/database/scripts/*.mts exists and a **/*.ts-only glob crashes ESLint outright.
  3. reportUnusedDisableDirectives now defaults to "warn" — source of the 62 new unused-directive warnings.

Appendix B — release-note items checked and cleared

Audited every breaking change in the ESLint v9.0.0 and typescript-eslint v8 announcements against this repo. Non-issues, with the reason:

Breaking change Why it doesn't affect us
.eslintignore no longer supported No .eslintignore files exist
--ext, --no-eslintrc, --rulesdir removed Not used in any script or workflow
context/SourceCode API removals, function-style rules, CodePath#currentSegments No custom rules or local plugins in the repo
RuleTester strictness No rule tests
7 formatters removed (checkstyle, compact, junit, tap, unix, …) CI uses --format stylish, which is retained
valid-jsdoc / require-jsdoc / no-new-symbol removed Not used
no-throw-literalonly-throw-error, prefer-ts-expect-errorban-ts-comment Not used
Node.js floor raised to ^18.18 || ^20.9 || >=21.1 Repo requires node >=22; CI pins 22
TypeScript floor >=4.8.4 On 5.5.4; typescript-eslint@8.67 supports <6.1.0
Rule schema strictness (options prohibited without meta.schema) No invalid rule options — config loads clean
Duplicate /* eslint */ comments now error None present

One item worth a second look: prefer-nullish-coalescing's ignoreConditionalTests default flipped to true in v8, which makes the rule quieter. It didn't appear in the diff, so it is likely not enabled in our preset — but it's the only change here that removes coverage rather than adding noise.

projectService is now the recommended replacement for parserOptions.project and is claimed to be faster. This PR deliberately keeps project: true to hold the diff minimal; switching is a reasonable follow-up but should be measured separately.

Appendix C — plugin compatibility audit

Package Installed Peer range v9 OK?
typescript-eslint 7.18.0 ^8.56.0 ❌ → 8.67.0
@vercel/style-guide 6.0.0 >=8.48.0 <9 ❌ → deleted, unused
eslint-plugin-react 7.37.5 ^3 … ^9.7 ✅ (blocks v10)
eslint-plugin-react-hooks 5.2.0 … ^9.0.0
eslint-config-prettier 10.1.8 >=7.0.0
eslint-plugin-prefer-arrow-functions 3.4.2 >=8.0.0
eslint-plugin-turbo 2.5.6 >6.6.0
eslint-plugin-only-warn 1.1.0 none
@next/eslint-plugin-next 15.0.4 none
@eslint/js 9.34.0 none ✅ (already v9)

Appendix D — method

Two worktrees off main @ 5f4d9ea0: one at the pre-upgrade commit (verified eslint --versionv8.57.1) and one with the upgrade applied (v9.39.5). For each of the 7 workspaces with an eslint.config.mjs, npx eslint . --format json was run and tallied by rule and severity. --fix behavior was measured by actually applying fixes and re-running both lint and turbo check-types, then reverting.

Intermediate states of the upgrade, for the record:

State Result
ESLint 8 baseline 573 warnings, 0 errors
Version bump only 515 fatal parse errors
+ absolute tsconfigRootDir 754 warnings, 1 error
+ TS-only scoping ESLint crashed on .mts
+ .mts/.cts in glob 754 warnings, 0 errors ← this PR

Methodology note / correction. An earlier revision of this PR body claimed --fix inflated the repo to ~22,700 warnings. That number came from --fix-dry-run and was an artifact: with type-aware linting, --fix-dry-run checks modified in-memory source against a TypeScript program still built from the on-disk files, so types desync and it emits a flood of spurious no-unsafe-* reports. A real --fix writes to disk and stays coherent — the true figure is 754 → 612 warnings. Do not use --fix-dry-run to estimate impact on a type-checked config. The conclusion that --fix is unsafe still holds, but the actual damage is the 25 tsc errors documented above, not a warning explosion.

🤖 Generated with Claude Code

Working proof-of-concept for the ENG-2118 investigation. Not intended to
merge as-is: ENG-2118 scopes the actual upgrade out and the upgrade should
land under its own ticket.

Pushed so the diff and CI results are reviewable alongside the findings.

- eslint 8.57.1 -> ^9.39.5, @types/eslint 8.56.12 -> ^9.6.1 (catalog)
- typescript-eslint ^7.18.0 -> ^8.67.0
- drop @vercel/style-guide and eslint-config-turbo: both were declared but
  never imported, and @vercel/style-guide was the only remaining hard pin
  on eslint <9
- drop the now-unneeded eslint ^8.57 allowedDeprecatedVersions entry

Three breaking changes had to be handled for the repo to lint at all:

- tsconfigRootDir must be an absolute path under typescript-eslint v8;
  "." is rejected and every file fails to parse. Uses import.meta.dirname
  (safe: the repo already requires Node >=22).
- Type-aware linting must be scoped to TS files. With project: true applied
  to every file, v8 errors on .mjs files absent from any tsconfig where v7
  skipped them. The glob must include .mts/.cts or ESLint crashes outright
  on packages/database/scripts.
- Plain JS needs disableTypeChecked for the same reason.

Result: 0 errors across all 7 linted workspaces, turbo check-types 8/8.
Repo-wide warnings go 573 -> 754; see the PR body for the rule-level
breakdown and why `eslint --fix` must not be run over this.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@linear-code

linear-code Bot commented Aug 13, 2026

Copy link
Copy Markdown

ENG-2118

@vercel

vercel Bot commented Aug 13, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
discourse-graph Ready Ready Preview Aug 13, 2026 9:36pm

Request Review

@supabase

supabase Bot commented Aug 13, 2026

Copy link
Copy Markdown

This pull request has been ignored for the connected project zytfjzqyijgagqxrzbmz because there are no changes detected in packages/database/supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.


Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

@maparent

Copy link
Copy Markdown
Collaborator

Really great work!
Two questions: Is it easy to fix the typescript-eslint@7.18.0 constraint? Latest version is 8.67.0, maybe it forces us to upgrade typescript?

Re lint --fix.
My reflex would be to run it with the old ESLint (no negative impact) before migrating.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants