Skip to content

Comments

feat: use oxlint to speed up lint#10168

Merged
EurFelux merged 21 commits intomainfrom
chore/oxlint
Sep 15, 2025
Merged

feat: use oxlint to speed up lint#10168
EurFelux merged 21 commits intomainfrom
chore/oxlint

Conversation

@EurFelux
Copy link
Collaborator

@EurFelux EurFelux commented Sep 14, 2025

What this PR does

Add oxlint to speed up the linting process without replacing eslint.

In my development environment, eslint takes about 18 seconds to lint the entire codebase, whereas oxlint takes less than 1 second. However, despite its remarkable speed, oxlint currently supports a limited set of rules, and some of its rule behaviors differ from their corresponding eslint plugins, making it impossible to fully replace eslint.

  • yarn test:lint will run checks using both oxlint and eslint sequentially. By first using oxlint, we can quickly identify and eliminate most issues, throwing errors early on. This significantly improves efficiency, whether in CI or local development.
  • Using the official vscode plugin oxc for oxlint can also greatly enhance linting speed during local development.
  • The job order for PR CI has been adjusted; now the lint check will run before the format check, as oxlint is significantly faster than Prettier. (NOTE: Biome's formatting speed is also much faster than Prettier, but its linting speed isn't as good as oxlint's. We might consider replacing Prettier with Biome.)

Since eslint has not been completely replaced, it still needs to check every file in the CI environment, resulting in no improvement in runtime. However, because oxlint can provide an extremely fast one-time check, the PR CI will fail more quickly when there are lint errors.

Fixes #

Why we need it and why it was done in this way

The following tradeoffs were made:

The following alternatives were considered:

Links to places where the discussion took place:

Breaking changes

If this PR introduces breaking changes, please describe the changes and the impact on users.

Special notes for your reviewer

Checklist

This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.

Release note


Add new eslint plugin to enhance linting capabilities with oxlint rules
Add oxlint plugin as recommended in the documentation to enhance linting capabilities
Add oxlint alongside eslint in test:lint and lint scripts for enhanced static analysis
Configure oxlint with a comprehensive set of rules for JavaScript/TypeScript code quality checks
- Add oxc to editor code actions on save
- Update oxlint configs to use eslint, typescript, and unicorn presets
- Extend ignore patterns in oxlint configuration
- Simplify oxlint command in package.json scripts
- Add oxlint-tsgolint dependency
- Add src/preload to eslint ignore patterns
- Update oxlint env to es2022 and add environment overrides
- Adjust several lint rule severities and configurations
…name

The linter was changed from ESLint to oxlint, so the directive needs to be updated accordingly.
@EurFelux EurFelux requested a review from 0xfullex as a code owner September 14, 2025 18:55
@EurFelux
Copy link
Collaborator Author

@claude review

@claude
Copy link
Contributor

claude bot commented Sep 14, 2025

Claude finished @EurFelux's task —— View job


Code Review: Add oxlint to speed up linting

Summary

This PR successfully integrates oxlint as a pre-linter to speed up the linting process. The implementation is well-structured and maintains compatibility with the existing eslint setup while providing significant performance improvements (18s → <1s for the entire codebase).

✅ Strengths

  1. Non-disruptive Integration: The implementation smartly uses oxlint as a first-pass linter while preserving eslint for rules not yet supported by oxlint. This ensures no regression in code quality.

  2. Proper Configuration Management:

    • The .oxlintrc.json configuration properly mirrors the environment settings from eslint
    • Correct use of eslint-plugin-oxlint to disable overlapping rules and avoid double-checking
    • Appropriate rule selection with clear comments for disabled rules
  3. Command Structure: The test:lint command properly chains oxlint and eslint with --deny-warnings flag, ensuring fast failure on lint errors.

  4. Developer Experience: Including the oxc VS Code extension in recommendations will improve the local development experience.

🔍 Observations

  1. Code Changes: The source file modifications (39 files) appear to be fixes for issues caught by oxlint's stricter or different rule implementations:

    • Removed unused variables/imports
    • Fixed comment formatting (// NODE// NOTE in src/main/utils/index.ts:39)
    • Corrected conditional logic (MessageErrorBoundary.tsx:21)
    • Other minor linting corrections
  2. Rule Differences: Some oxlint rules are intentionally disabled with comments indicating they may be enabled in the future:

    "oxc/only-used-in-recursion": "off", // manually off because of existing warning
    "unicorn/no-empty-file": "off", // may turn it on in the future
  3. TypeScript Rules: Good handling of TypeScript-specific rules with appropriate severity levels (warn vs error).

⚠️ Minor Issues

  1. Duplicate Entry in ignorePatterns: Line 19 in .oxlintrc.json duplicates line 17:

    "src/main/integration/nutstore/sso/lib/**",  // Line 17
    "src/main/integration/cherryin/index.js",
    "src/main/integration/nutstore/sso/lib/**",  // Line 19 (duplicate)
  2. Comment Typo: Line 137 has a typo: "bacause" should be "because"

    "oxc/only-used-in-recursion": "off", // manually off bacause of existing warning

📝 Suggestions

  1. Documentation: Consider adding a brief note in the project's README or contributing guide about the two-stage linting approach, explaining why both oxlint and eslint are used.

  2. CI Pipeline: Ensure the CI pipeline uses the same yarn test:lint command to benefit from the speed improvements.

  3. Future Roadmap: Consider documenting which eslint rules are still needed and tracking oxlint's progress on supporting them, to eventually simplify to a single linter when possible.

  4. Performance Metrics: It would be valuable to add the performance improvement metrics to the PR description for future reference.

Verdict

This is a well-executed integration that provides immediate performance benefits while maintaining code quality standards. The approach is pragmatic and the implementation is clean. With the minor fixes mentioned above, this PR is ready for merge.

Recommended: Approve with minor suggestions

@beyondkmp
Copy link
Collaborator

beyondkmp commented Sep 15, 2025

Note

This comment was translated by Claude.

Currently, eslint uses the simple-import-sort plugin, which oxlint has not implemented yet (oxc-project/oxc#7197). If we keep both, it will cause the import order to be different.


Original Content 目前eslint用了simple-import-sort plugin,oxlint是没有实现的(https://github.com/oxc-project/oxc/discussions/7197), 两个都保留的话,会导致这个import的顺序不一样。

@EurFelux
Copy link
Collaborator Author

EurFelux commented Sep 15, 2025

Note

This comment was translated by Claude.

Why? If oxlint doesn't have it, won't it just use eslint's instead? There's no conflict either.


Original Content

Note

This comment was translated by Claude.

Currently, eslint uses the simple-import-sort plugin, which oxlint has not implemented yet (oxc-project/oxc#7197). If we keep both, it will cause the import order to be different.

Original Content
目前eslint用了simple-import-sort plugin,oxlint是没有实现的(https://github.com/[oxc-project/oxc/discussions/7197](https://github.com/oxc-project/oxc/discussions/7197)), 两个都保留的话,会导致这个import的顺序不一样。

为啥?oxlint没有的话,不是就用eslint的了吗,也没有冲突啊。

@beyondkmp
Copy link
Collaborator

beyondkmp commented Sep 15, 2025

Note

This comment was translated by Claude.

oxlint has its own sorting rules, so it's also fine to not enable oxlint's own sorting.


Original Content oxlint有自己一套排序的,那就是不要启用oxlint自己的排序也是可以的。

@EurFelux
Copy link
Collaborator Author

EurFelux commented Sep 15, 2025

Note

This comment was translated by Claude.

I checked and there is an eslint/sort-imports rule, but it's not enabled in the configuration, and it's not a default rule either.


Original Content

Note

This comment was translated by Claude.

oxlint has its own sorting rules, so it's also fine to not enable oxlint's own sorting.

Original Content

看了一下,是有eslint/sort-imports这个规则,不过配置里面没有开的,这个也不是默认规则。

@beyondkmp
Copy link
Collaborator

beyondkmp commented Sep 15, 2025

Note

This comment was translated by Claude.

image

It seems there's no improvement after all. eslint is still slow, even though oxlint is fast.


Original Content image

好像也没有提升了,eslint还是很慢,虽然oxlint快。

@beyondkmp
Copy link
Collaborator

beyondkmp commented Sep 15, 2025

Note

This comment was translated by Claude.

How about we try a complete replacement? If there are rule differences, we can re-lint and format all the code, and from then on use oxlint as the standard. That would be really awesome.


Original Content 要不就是全部替换看下,如果有规则不一样,直接代码都重新lint和format下,后面都以oxlint为准,这样就真的非常爽了。

@alephpiece
Copy link
Collaborator

alephpiece commented Sep 15, 2025

Note

This comment was translated by Claude.

How about we try a complete replacement? If there are rule differences, we can re-lint and format all the code, and from then on use oxlint as the standard. That would be really awesome.

If we do a complete replacement, it's best to put lint and format in one PR. After it's done, it will also be easier to update the blame ignore.


Original Content

How about we try a complete replacement? If there are rule differences, we can re-lint and format all the code, and from then on use oxlint as the standard. That would be really awesome.

全部替换的话最好把 lint 和 format 放在一个 pr 里面,做完之后也好改 blame ignore

@EurFelux
Copy link
Collaborator Author

EurFelux commented Sep 15, 2025

Note

This comment was translated by Claude.

For example, the exhaustive-deps rule for react hooks - although oxlint will report issues, it reports them in different places. If we want to drop eslint, we'd have to change eslint-disable comments in many places. Also, the auto-fix feature for this rule won't be available, which is quite inconvenient. This rule is mainly what makes me hesitant to replace eslint. I haven't looked closely at other rules yet.


Original Content 比如react hooks的exhaustive-deps这个规则,oxlint虽然会报告问题,但是报告问题的地方不一样,如果要丢掉eslint的话,就要改特别多地方的eslint-disable的注释。而且这个规则的自动修复功能也用不了,还挺不方便的。主要是这个规则让我不太想替掉eslint,其他规则倒是没有细看过。

@EurFelux
Copy link
Collaborator Author

@EurFelux
Copy link
Collaborator Author

It seems there's no improvement after all. eslint is still slow, even though oxlint is fast.

If eslint isn't completely replaced, the entire linting process cannot be fully accelerated, because eslint still needs to be run again after oxlint. While oxlint allows most issues to be flagged earlier, it doesn't yet support all existing rules.

Copy link
Collaborator

@GeorgeDong32 GeorgeDong32 left a comment

Choose a reason for hiding this comment

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

我这边测试没啥问题,oxlint能覆盖基础规则尽早发现基础错误也是件好事,虽然现阶段还是要eslint兜底

@EurFelux
Copy link
Collaborator Author

@0xfullex

@EurFelux EurFelux merged commit 4d1d3e3 into main Sep 15, 2025
6 checks passed
@EurFelux EurFelux deleted the chore/oxlint branch September 15, 2025 16:12
Comment on lines +58 to +59
// We don't use the React plugin here because its behavior differs slightly from that of ESLint's React plugin.
"plugins": ["unicorn", "typescript", "oxc", "import"],
Copy link

Choose a reason for hiding this comment

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

Looks like 2x issue preventing you from using react plugin.

First being a bug which should be fixed in the next version here: oxc-project/oxc#13815

The second being:

  ⚠ eslint-plugin-react(no-children-prop): Avoid passing children using a prop.
     ╭─[src/renderer/src/components/DraggableList/virtual-list.tsx:188:23]
 187 │                       virtualizer={virtualizer}
 188 │                       children={children}
     ·                       ────────
 189 │                       disabled={disabled}
     ╰────
  help: The canonical way to pass children in React is to use JSX elements

Which i think you should just disable in your oxlint config as it's not enabled in your eslint config?

Thanks for using oxlint!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you for following up on this issue! no-children-prop is not a big deal because just few errors here. Once the issue with exhaustive-deps is resolved, we can replace the eslint react plugin with the oxlint React plugin.

Copy link

Choose a reason for hiding this comment

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

awesome, going to try and do a release today 🤞

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.

7 participants