Skip to content

Migrate action to run oxlint instead of eslint#1

Closed
geoffharcourt wants to merge 28 commits into
masterfrom
oxlint-migration
Closed

Migrate action to run oxlint instead of eslint#1
geoffharcourt wants to merge 28 commits into
masterfrom
oxlint-migration

Conversation

@geoffharcourt

Copy link
Copy Markdown
Member

Summary

Breaking rewrite of this composite action to run oxlint through reviewdog instead of eslint.

  • Replaces the bundled eslint-formatter-rdjson with a small Node converter (oxlint-to-rdjsonl.js) that turns oxlint --format=json into rdjsonl lines for reviewdog -f=rdjsonl.
  • Renames input eslint_flagsoxlint_flags; drops node_options (no-op for a Rust binary); tool_name default becomes oxlint.
  • Deletes eslint-formatter-rdjson/, the repo-root .eslintrc.js / package.json / package-lock.json, and the npm-publish workflow (nothing to publish).
  • Test fixtures in testdata/ and test-subproject/ updated so oxlint produces deterministic diagnostics.
  • Design and implementation plan committed under docs/superpowers/.

35 converter unit tests pass (node --test oxlint-to-rdjsonl.test.js). End-to-end smoke-tested against oxlint 1.61.0.

Breaking changes for consumers

  • Must depend on oxlint, not eslint.
  • eslint_flagsoxlint_flags.
  • node_options removed.

A short "Migrating from action-eslint" section was added to the README.

Test Plan

  • test workflow runs node --test oxlint-to-rdjsonl.test.js and is green.
  • reviewdog workflow exercises the action across Node 18/20/22 against testdata/ and test-subproject/sub-testdata/; annotations appear on this PR.
  • Manual: clone locally, run oxlint --format=json testdata/ | node oxlint-to-rdjsonl.js and confirm rdjsonl output.

🤖 Generated with Claude Code

geoffharcourt and others added 22 commits April 21, 2026 22:22
In-place rewrite of the action-eslint composite action to run oxlint
through reviewdog instead of eslint. Captures input renames, the new
oxlint→rdjsonl converter, file additions/deletions, and testing plan.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Twenty bite-sized tasks covering: TDD of the JSON→rdjsonl converter,
action.yml/script.sh rewrite, deletion of the bundled eslint formatter
and repo-root eslint detritus, testdata/test-subproject fixture
updates, workflow audit (test.yml rewrite, reviewdog.yml rename,
npm-publish.yml deletion), and README rewrite with migration callout.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Validated against oxlint 1.61.0: the output is an object wrapping the
diagnostics array (`{"diagnostics": [...], "number_of_files": N, ...}`),
not a bare array as the initial spec assumed. Bare arrays still parse
for forward-compatibility. Cache file reads so multi-diagnostic files
aren't re-read per diagnostic.

Spec updated to reflect the real shape.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ptions

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Smoke-tested against oxlint 1.61.0: empty.js emits no diagnostics;
error.js fires no-var + no-unused-vars; test.js fires no-var + no-empty
(with a multi-line span).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Needed a subproject-local .oxlintrc.json to shadow the repo-root
config's ignorePatterns (which include test-subproject/) — otherwise
oxlint considers every subproject file ignored when invoked inside
the subproject. Existing sub-testdata/test.js already fires no-var,
no-unused-vars, and for-direction under oxlint defaults, so the JS
fixture is unchanged.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Comment thread testdata/error.js
@@ -1 +1 @@
🐶 // test for unexpected character
var unusedVar = 1;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚫 [oxlint] <no-unused-vars> reported by reviewdog 🐶
Variable 'unusedVar' is declared but never used. Unused variables should start with a '_'.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The errors here are duplicated because the workflow tests all three styles of Reviewdog comments. I've confirmed it only reports once per issue on repo code.

Comment thread testdata/error.js
@@ -1 +1 @@
🐶 // test for unexpected character
var unusedVar = 1;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚫 [oxlint] <no-var> reported by reviewdog 🐶
Unexpected var, use let or const instead.

Comment thread testdata/test.js
Comment on lines +2 to 3
for (var i = 0; i < 10; i++) {
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ [oxlint] <no-empty> reported by reviewdog 🐶
Unexpected empty block statements

Comment thread testdata/test.js
var a;var b;//comment
console.log(i);
function sample() {
for (var i = 0; i < 10; i++) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚫 [oxlint] <no-var> reported by reviewdog 🐶
Unexpected var, use let or const instead.

@socket-security

socket-security Bot commented Apr 22, 2026

Copy link
Copy Markdown

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff Package Supply Chain
Security
Vulnerability Quality Maintenance License
Addedoxlint@​1.61.0991009196100

View full report

geoffharcourt and others added 2 commits April 22, 2026 09:29
Previously the action always went through `npx --no-install -c 'oxlint
--version'` to detect oxlint, then fell back to `npm install` if that
check failed. This broke two pnpm/monorepo cases:

1. pnpm's symlink + workspace:* resolution can make the npx check
   return non-zero even when oxlint is present.
2. Falling back to `npm install` fails with EUNSUPPORTEDPROTOCOL on
   any `workspace:*` dep.

Now the script resolves oxlint by preference:
- binary on PATH -> use directly
- ./node_modules/.bin/oxlint -> use directly
- neither -> `npm install` as before (still fails on pnpm, but the
  consumer can opt out by installing oxlint themselves beforehand)

Invocation goes through the resolved path; `npx` is only used as a
last resort after the install step.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Comment thread oxlint-to-rdjsonl.js
@@ -0,0 +1,147 @@
'use strict';

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a port of the test output formatter that Reviewdog used for ESLint. It's been modified to work for Oxlint's JSON output and tested against real results.

jobs:
eslint:
name: runner / eslint
oxlint:

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This action is used for testing, it's not the actual action that gets run in repo use.

Comment thread action.yml
@@ -1,5 +1,5 @@
name: 'Run eslint with reviewdog'
description: '🐶 Run eslint with reviewdog on pull requests to improve code review experience.'
name: 'Run oxlint with reviewdog'

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the actual production action definition.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Comment thread script.sh
# project has no oxlint on PATH and no ./node_modules/.bin/oxlint — this
# avoids breaking pnpm / yarn / monorepo setups where `npm install` would
# reject workspace protocols.
if command -v oxlint >/dev/null 2>&1; then

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use the locally-installed oxlint when possible. Removes the need to work with the repo's specific npm/yarn/pnpm setup.

geoffharcourt and others added 2 commits April 22, 2026 10:03
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… two examples

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@geoffharcourt

Copy link
Copy Markdown
Member Author

Closing for now, we'll return.

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.

1 participant