chore: replace eslint with oxlint - #2010
Conversation
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Dp8HUuTkriy2qNUP2GNgcs
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
Looks good, straightforward tooling migration from ESLint to oxlint. What was reviewed: the .oxlintrc.json config and package.json/yarn.lock script changes (confirmed no leftover references to eslint or lint:ts anywhere else in the repo); the mechanical source fixes in files.ts, ipc.ts, windows.ts, commands-bisect.tsx, settings-electron.tsx, settings-general-github.tsx, versions.ts, editor.tsx, output.tsx, and highlight-text.tsx (redundant try/catch removal, !!/Boolean() drop in truthy checks, ternary-to-if/else, duplicate destructure alias, regex escape cleanup) — all behavior-preserving; and the test assertion fixes in app.spec.tsx, file-manager.spec.ts, and editor-utils.spec.ts that add missing .toBe(...) matchers to previously no-op expect() calls.
Extended reasoning...
Overview
This PR replaces ESLint 8 with oxlint across the project: deletes .eslintrc.js, adds .oxlintrc.json, updates package.json (scripts, lint-staged, devDependencies) and yarn.lock, and applies the small source/test fixes that the new linter surfaced (redundant boolean coercions, a duplicate destructure alias, unreachable try/catch-rethrow blocks, a ternary-as-statement, an unnecessary regex escape, and several test assertions that were missing .toBe(...) matchers).
Security risks
None. This is a dev-tooling/lint-config change with no runtime code paths touching auth, crypto, network, or user input handling in any security-relevant way. The files.ts try/catch removals are behaviorally identical (both old and new code let the error propagate), and the ipc.ts change from x.isDestroyed() || x.send(...) to an explicit if is a pure refactor with equivalent short-circuit semantics.
Level of scrutiny
Low-to-moderate is appropriate: this is a mechanical, low-risk chore PR. I read every changed source/test file's diff in full (not just the config), verified the removed try/catch blocks and ||-as-statement change preserve behavior, confirmed the regex escape change doesn't alter matching semantics, and confirmed the test assertion fixes make the tests strictly stronger (they now check the expected boolean value rather than being no-ops). I also grepped the repo for any leftover reference to eslint or lint:ts outside yarn.lock/.yarn/releases and found none, confirming the migration is complete and self-consistent.
Other factors
The PR description is transparent about trade-offs it accepted (losing import/order, tsdoc/syntax enforcement, keeping standard for the static/ templates which pulls ESLint 8 back in transitively) — these are documented, reasonable engineering trade-offs for a lint-tool swap, not correctness bugs. Test coverage is unaffected since only lint config and assertion strength changed. The PR author states yarn lint, yarn tsc, and the vitest suite were run locally; I did not have a working shell in this session to independently execute the build, but the diff itself is small, self-contained, and each individual hunk is easy to verify by inspection, which is sufficient here.
Requested by Samuel Attard · Slack thread
Before:
lint:ts/lint:jsran ESLint 8 with@typescript-eslint,eslint-plugin-import,eslint-plugin-react,eslint-plugin-tsdocandeslint-config-prettierfrom.eslintrc.js.After: a single
lint:jsscript runsoxlint(1.81.0) configured by.oxlintrc.json;standardstill lints thestatic/templates, and prettier/stylelint/markdownlint are unchanged.eslint,eslint-config-prettier,eslint-import-resolver-typescript,eslint-plugin-import,eslint-plugin-react,eslint-plugin-tsdoc,@typescript-eslint/eslint-plugin,@typescript-eslint/parser; addedoxlint@^1.81.0; deleted.eslintrc.js..oxlintrc.json:correctnesscategory as errors plus thetypescript,import,reactandvitestplugins; portssort-imports,no-unused-vars(^_args, ignore catch params),import/default,import/no-duplicates,import/no-named-as-defaultand the react/recommended rules oxlint provides; keeps the same ignore list (out,.webpack,coverage,static).lint:tsandlint:jsintolint:js(oxlint, which lints js/ts/tsx itself) and collapsed the matching lint-staged entries into one;lint:fix/lintkeep working vianpm-run-all.Boolean()/!!in ternaries,monacoOptions: monacoOptionsrenames, two uselesstry/catchwrappers insrc/main/files.ts,||/ternary used as statements, an unnecessary regex escape, and sevenexpect(x);calls in tests that had no matcher (now.toBe(true/false)).eslint-disable-next-linecomment (rule no longer enabled); added oneoxlint-disable-next-line no-unused-varsinsrc/renderer/app.tsxwhere React is only used by JSX.import/order(import grouping/alphabetizing),tsdoc/syntax,import/named/import/export(still nursery). Turned off new vitest rules that would need test rewrites (expect-expect,no-conditional-expect,no-standalone-expect,require-to-throw-message,require-mock-type-parameters) andreact/no-did-mount-set-state.Verified with
yarn lint,yarn tscand the vitest suite (902/903 pass; the one failure isnpm-searchneeding network access to Algolia), and the husky/lint-staged pre-commit hook ran the newlint:jssuccessfully on commit.Open question:
standardis kept only forstatic/**/*.jsbecause it enforces the no-semicolon template style that oxlint cannot check; it does pull ESLint 8 back in transitively. Happy to drop it if you'd rather lint the templates with oxlint and accept losing the style enforcement.🤖 Generated with Claude Code
https://claude.ai/code/session_01Dp8HUuTkriy2qNUP2GNgcs
Generated by Claude Code