build(lint): migrate from ESLint to oxlint - #468
Draft
tkislan wants to merge 4 commits into
Draft
Conversation
Faithful port of .eslintrc.cjs's 21 active rules to oxlint.config.mts, run alongside ESLint for now (both `npm run lint` and `npm run lint:oxlint` are green). Mirrors deepnote-internal PR #20639. - Native rules (ban-ts-comment, no-dupe-class-members, no-explicit-any, no-restricted-imports, no-unused-vars, no-use-before-define, no-useless-constructor, no-void, no-labels, no-with, jsx-filename-extension) map straight onto oxlint's built-ins. - no-floating-promises now runs type-aware via tsgolint instead of ESLint's parserOptions.project (the sole reason for that option). - import/no-restricted-paths (the 12 architecture-layering zones) runs through the real eslint-plugin-import as a jsPlugin, aliased to import-plugin since oxlint reserves the `import` plugin name. eslint-plugin-import is bumped to latest v2 for ESLint-9 API compat, which the jsPlugin runtime requires. - build/eslint-rules/index.js (the 4 custom architecture rules) is modernized in place for the same ESLint-9-shaped runtime: context.getFilename() -> context.filename, 2-arg context.report() -> object form, and the node-builtin check now uses node:module's isBuiltin instead of eslint-plugin-import's internals. Added local-rules/no-for-in, since oxlint has no equivalent to no-restricted-syntax (that rule relied on ESLint's esquery selectors, which can't run via jsPlugins) - the other two selectors it carried, LabeledStatement and WithStatement, map onto oxlint's native no-labels and no-with. - tsgolint requires a baseUrl-free, non-relative-paths-free tsconfig (typescript-go doesn't support `baseUrl`): root tsconfig.json drops `baseUrl` and switches `paths` to the `./types/*` form (supported without baseUrl since TS 4.1); src/renderers/client/tsconfig.json (IDE-only - esbuild bundles that entry point directly and never reads this file) gets the same treatment plus an explicit rootDir, since its import of a sibling directory made TS's inferred rootDir ambiguous. npm run typecheck stays green either way. Suppression comments: the 1092 existing eslint-disable comments are untouched - oxlint honours them natively, including normalizing the @typescript-eslint/ prefix to typescript/. Only the comments whose rule id is actually changing were touched, and since ESLint hard-errors on a disable comment naming a rule it doesn't recognize (while oxlint quietly ignores one it doesn't), each of those sites now carries both an unchanged eslint-disable comment for the old id and a new oxlint-disable-line/-next-line comment for the new one, so both tools stay green during the parallel-run window: - import/no-restricted-paths -> import-plugin/no-restricted-paths (8 sites, all still real zone violations). - no-restricted-syntax's ForInStatement selector -> local-rules/no-for-in (3 sites that are genuine for..in loops). The other 5 sites referencing no-restricted-syntax turned out to guard for..of loops, which that selector never matched in the first place (dead suppressions predating this migration) - the stale token is dropped rather than mapped to a rule that wouldn't fire there either. - The for..in sites use a comment inside the loop's parens rather than a trailing same-line comment, since Prettier unconditionally hoists a trailing comment after a block's opening brace onto its own line inside the block, which silently breaks the suppression. package.json also adds a lint:oxlint script (oxlint src) alongside the existing eslint-based lint script, and moves eslint-plugin-import to ^2.32.0. The eslint->oxlint script swap, CI wiring, and dependency removal land in a follow-up commit once the findings below are cleared. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NeHawfLMtvXn252EkCip2k
tsgolint's no-floating-promises implements a check the old @typescript-eslint/eslint-plugin@6.9.0 didn't have: a bare array-of- Promises expression statement (e.g. `arr.map(x => asyncFn(x))` used without await/void) is now also flagged, not just a bare single Promise. This surfaced 8 sites, 1 of them a real bug: - emptyNotebookCellLanguageService.ts: `await emptyCodeCells.map(async cell => ...)` doesn't actually wait for the mapped promises - await on a plain array resolves immediately, it does not wait for the array's contents to settle. Fixed with Promise.all so the language update genuinely completes before chainWithPendingUpdates' callback resolves. - The other 7 (events.ts, remoteKernelFinderController.ts, localPythonEnvKernelSourceSelector.node.ts, and 4 sites in executionService.vscode.test.ts) are intentional fire-and-forget: each promise in the array already has its own .catch(noop)/.then(noop, noop), so nothing goes unhandled - the array itself was just never meant to be awaited. Marked with `void`, the same convention the ported no-floating-promises config already used for this (ignoreVoid: true). Verified with the full unit test suite (2545 passing) and npm run typecheck; `npx oxlint src` is now clean except for 27 pre-existing warn-level no-unused-vars findings on unused catch parameters (unused-catch-var support the old @typescript-eslint/no-unused-vars@6.9.0 apparently never enforced either, despite matching config) - warn severities don't fail CI, matching prior behavior, so those are left as-is; flagged separately for whoever picks this up next. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NeHawfLMtvXn252EkCip2k
`npm run lint` now runs oxlint directly (~5-7s vs the old 47s baseline);
lint-staged and the CI lint step follow. Removes the 14 now-unused
eslint packages (eslint itself, @typescript-eslint/{parser,eslint-plugin,
eslint-plugin-tslint}, eslint-config-{airbnb,prettier}, and eslint-plugin-
{header,jsdoc,jsx-a11y,no-null,prefer-arrow,prettier,react,react-hooks}),
keeping eslint-plugin-import, eslint-plugin-local-rules and eslint-plugin-
no-only-tests since oxlint's config still runs them as jsPlugins.
Editor/devcontainer/docs recommendations move from dbaeumer.vscode-eslint
to oxc.oxc-vscode (the reference deepnote-internal PR skipped this;
verified oxc.oxc-vscode and the source.fixAll.oxc code-action id against
oxc-project's own docs). tasks.json's npm-lint task also drops the
$eslint-stylish problem matcher (oxlint's output isn't in that format)
and its detail text, which was already stale (referenced a `.eslintrc.js`
that never existed here - the file was always `.eslintrc.cjs`).
Removing the 14 packages required a fresh npm install, which surfaced an
unrelated latent bug: src/test/interpreters/condaService.node.ts (test-
only fixture code, not part of the shipped extension) imports `untildify`
without ever declaring it - it only resolved because untildify happened
to be hoisted to the top level as a transitive dependency of an unrelated
package (default-browser-id, via @vscode/vsce). That package no longer
needs untildify after this dependency churn, so the phantom import broke
`npm run typecheck`. Declared explicitly as a devDependency, pinned to
the 4.0.0 that was already the resolved (if undeclared) version.
Verified: npm run typecheck, npm run lint (oxlint, clean except the 27
pre-existing warn-level findings noted in the prior commit), npm run
format, npm ci (matching the CI install step exactly), and the full unit
test suite (2545 passing) all green.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NeHawfLMtvXn252EkCip2k
Contributor
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Excluded labels (none allowed) (3)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #468 +/- ##
===========================
===========================
🚀 New features to boost your workflow:
|
…mports
Porting `node-imports` off `eslint-module-utils/moduleVisitor` kept only two
of the five AST node types that visitor subscribed to. `ExportNamedDeclaration`,
`ExportAllDeclaration` and `ImportExpression` were dropped, so in web/shared
code (anything not `.node.ts`, `.test.ts`, or under `src/test`) these forms
stopped being checked entirely:
export * from 'fs'
export { readFile } from 'node:fs'
await import('child_process')
The gap is not theoretical. `src/platform/common/crypto.ts:34` already does
`await import('node:crypto')` behind an `eslint-disable-next-line
local-rules/node-imports` comment that predates the oxlint migration — proof
the rule used to fire there. Since the port, the rule could not see that line
at all, leaving the suppression comment inert and the web/node boundary
(specs/architecture.md) unenforced for every one of these forms going forward.
Restores the three missing visitor keys. `checkSource` guards on `node.source`
because `export { x }` and `export const x = 1` are `ExportNamedDeclaration`
nodes with a null source; `ImportExpression` checks for a string literal so
`import(someVariable)` is skipped rather than crashing. oxlint 1.77.0's
jsPlugin runtime supports all three keys directly, so no CallExpression
fallback is needed for dynamic imports.
Verified: each of the three restored forms plus the `import`/`require`
controls reports exactly once (no double-firing); `export { x }` reports
nothing and does not throw; stripping crypto.ts's disable comment now
produces the expected error and restoring it suppresses again; and full
`npm run lint` is unchanged at exit 0 with 0 errors and the same 27
pre-existing no-unused-vars warnings. `npm run typecheck` and `npm run
format` green.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NeHawfLMtvXn252EkCip2k
Contributor
Author
Addressed:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Replaces ESLint with oxlint (native rules + jsPlugins for the rest, type-aware rules via tsgolint). Mirrors deepnote-internal PR #20639. Faithful port — zero policy change in the swap itself, every deliberate drop documented in
oxlint.config.mts's docblock.npm run lint(1153 files): 47s → 5.4s.Commits (reviewable independently)
build(lint): port ESLint config to oxlint—oxlint.config.mtswith all 21 previously-active rules mapped,build/eslint-rules/index.jsmodernized for the ESLint-9-shaped jsPlugin runtime, the tsconfig changes tsgolint's type-aware checking needs. Bothnpm run lint(old ESLint) andnpm run lint:oxlint(new) are green on this commit — the ~16 suppression comments whose rule id changes carry both an ESLint- and an oxlint-flavored disable comment during this parallel-run window.fix(lint): resolve floating-promise findings surfaced by oxlint— 8 findings from tsgolint's stricterno-floating-promises(it also flags a bare array of promises, which the old@typescript-eslint@6.9.0didn't). One real bug fixed (await arr.map(async fn)doesn't actually wait — needsPromise.all); the other 7 are intentional fire-and-forget, markedvoidper this codebase's existingignoreVoid: trueconvention.build(lint): delete ESLint— script/CI/lint-staged swap, removes the 14 now-unused ESLint packages, moves editor/devcontainer/CONTRIBUTING.md recommendations tooxc.oxc-vscode, deletes.eslintrc.cjs/.eslintignore.Verification
npm run typecheck,npm run lint,npm run format,npm ci(matches the CI install step) all green.process.env,.fsPath,__dirname,for..in, labels,with,test.only, restricted imports, floating promises,any, undescribed@ts-ignore) run after commit 1 and re-run after commit 3.Worth knowing before merging
no-unused-varson unused catch parameters (e.g.catch (e) { console.error(e); }) — oxlint catches these, the old config apparently never did despite matching options. Warn severity doesn't fail CI (same as before), so this migration doesn't touch those 27 call sites; flagging for a follow-up.baseUrl): roottsconfig.jsonandsrc/renderers/client/tsconfig.json(editor-only, esbuild never reads it — zero effect on the real build).npm run typecheckverified green after both.src/test/interpreters/condaService.node.ts(test-only) importeduntildifywithout ever declaring it — it only worked because of accidental hoisting from an unrelated package's transitive tree. Declared explicitly now.oxc.oxc-vscodeextension's actual in-editor behavior — worth a manual check.Out of scope (noted, not done)
[!test]-style zone globs inno-restricted-paths(ported verbatim — tightening them is a follow-up).build/**/*.ts/gulpfile.js(never linted before this either).🤖 Generated with Claude Code