ENG-2147 Upgrade eslint and typescript-eslint - #1293
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
Really great work! Re |
|
thanks for the review MAP. no need to upgrade typescript. upgrading typescript-eslint already widens support for typescript. |
|
fair enough. |
Carries out the upgrade recommended by the ENG-2118 investigation. Needed to unblock ENG-1832: eslint-plugin-obsidianmd@0.4.1 peers on eslint >=9.19.0 and typescript-eslint ^8.35.1, neither of which eslint 8.57.1 / typescript-eslint 7 could satisfy. - 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. No source files change; only ESLint config, the dependency catalog, and the lockfile. Warnings rise 573 -> 754, of which 62 are eslint 9 reporting unused eslint-disable/eslint-enable directives that eslint 8 ignored. All remain warnings, not errors -- 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>
bc6ffa5 to
52c5802
Compare
trangdoan982
changed the title
PR size/scope checkThis PR is over our review-size guideline.
Please split this into smaller PRs unless there is a clear reason the changes need to land together. If keeping it as one PR, please add a brief justification covering:
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
3 participants
Scope check
$scope-checkagainst the ENG ticket and final diff.Done When: None. The diff is the upgrade and the minimum config changes needed to make it lint.Carries out the upgrade recommended by the ENG-2118 investigation. That ticket scoped the upgrade itself out; this is the follow-up that lands it.
Why
ENG-1832 needs
eslint-plugin-obsidianmd@0.4.1, whose peer rangesmaincould not satisfy:eslint>=9.19.0typescript-eslint^8.35.1@eslint/js^9.30.1Testing
git fetch origin && git checkout eng-2118-investigate-whether-to-upgrade-repo-eslint-from-version-8-to pnpm installConfirm the upgrade actually resolved:
pnpm --dir packages/eslint-config exec eslint --versionLint everything. Use
--continueso one workspace failing does not mask the rest:These are same errors that are appearing on main branch. Lint errors not caused by this upgrade
CI is unaffected either way: the
lint-changed-filesjob runseslintdirectly against changed files and never invokesturbo lint.What changed
Dependencies (
pnpm-workspace.yamlcatalog +packages/eslint-config/package.json)eslint8.57.1 →^9.39.5typescript-eslint^7.18.0→^8.67.0@types/eslint8.56.12 →^9.6.1@vercel/style-guideandeslint-config-turbo. Both were declared but never imported by any config, and@vercel/style-guidewas the last hard pin holding eslint below 9.eslint: ^8.57entry fromallowedDeprecatedVersions.Config — three v8/v9 breaking changes had to be handled for the repo to lint at all:
tsconfigRootDirmust be absolute. typescript-eslint v8 rejects".", and every file fails to parse. Now usesimport.meta.dirname, which is safe because the repo already requires Node >=22.project: trueapplied to every file, v8 errors on.mjsfiles that belong to no tsconfig, where v7 silently skipped them. Thefilesglob must include.mts/.ctsor ESLint crashes outright onpackages/database/scripts.disableTypeCheckedfor the same reason — added as a dedicated block inpackages/eslint-config/base.js.No source files are touched. The diff is ESLint config, the dependency catalog, and
pnpm-lock.yaml(which shrinks by ~1,100 lines as the two dropped packages' trees go away).Appendix: Results
Measured on this branch rebased onto
main@da2861f3.turbo lint(7 workspaces)turbo check-typesPer workspace:
apps/roamapps/obsidianapps/websitepackages/databasepackages/uipackages/content-modelpackages/utilsWarnings: 573 → 754
The repo runs
eslint-plugin-only-warn, so nothing here can fail a build — but the count moves and reviewers should know why. 62 of the +181 are ESLint 9 reporting unused suppression directives, which ESLint 8 ignored by default:eslint-disabledirectiveeslint-enabledirectiveThe remainder comes from typescript-eslint v8 broadening its type-aware rules. Top contributors in the current run:
@typescript-eslint/no-floating-promises@typescript-eslint/no-unnecessary-type-assertion@typescript-eslint/no-unused-vars@typescript-eslint/no-unsafe-assignment@typescript-eslint/no-unsafe-member-accessreact/prop-typesreact-hooks/exhaustive-deps@typescript-eslint/no-misused-promises@typescript-eslint/naming-convention@typescript-eslint/no-explicit-anyeslint --fixover this branch. 103 of the warnings inapps/roamalone are reported as auto-fixable, but the fixers here are not safe to apply blind:no-unnecessary-type-assertionand the unused-directive fixers delete code that exists for a reason in places, and a sweeping autofix would bury this dependency upgrade under hundreds of unrelated source changes. Cleaning up warnings belongs in its own ticket.🤖 Generated with Claude Code