-
Notifications
You must be signed in to change notification settings - Fork 2.2k
chore(frontend): Migrate from π ESLint to β‘ Oxlint #33714
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Summary
Major migration from ESLint to Oxlint for frontend linting, delivering a 97% reduction in linting time (from ~8s to ~200ms per file) while preserving most code quality rules.
- Removed
.eslintrc.jsand custom ESLint rules in favor of.oxlintrc.jsonwith built-in equivalents - Preserved key rules through Oxlint built-ins:
no-schema-index-importβno-restricted-imports,no-spread-in-reduceβno-accumulating-spread - Removed deprecated
warn-elementsrule as Ant Design β Lemon UI migration is complete - Simplified React component exports by standardizing memo wrapper usage and function naming
- Sacrificed
no-survey-string-constantscustom rule as it cannot be replicated in Oxlint
41 files reviewed, 4 comments
Edit PR Review Bot Settings | Greptile
frontend/src/lib/components/TaxonomicFilter/taxonomicFilterLogic.test.ts
Show resolved
Hide resolved
|
This is gonna be my PR of the year π |
adamleithp
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, new toy! I trust this works
More eggs to be crackedI neglected to mention two aspects in the PR description. Missing pluginsOxlint supports a rich set of rules from the most important ESLint plugins, but it doesn't supports other plugins. Here are the ones we have to drop:
Rule rigmaroleSome rules work slightly differently, meaning there are new violations. I'm disabling those rules in this PR to keep the diff minimal, and re-enabling in a separate PR:
|
|
Size Change: 0 B Total Size: 2.58 MB βΉοΈ View Unchanged
|
mariusandra
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing π
rafaeelaudibert
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont like we're losing autofix for imports, but this is just so much nicer, let's do it
We should keep an eye out for an improved version of it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file can go now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, removed
Co-Authored-By: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
|
I just had to bump node memory to get eslint to run in lint-staged this makes me so happy |
| "builtin": true | ||
| }, | ||
| "ignorePatterns": [ | ||
| "node_modules", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i wonder if this needs to be **/node_modules to get all the products folders too
(although it's all super fast so π€· )
π typescript-eslint maintainer here. I'd like to note that this is largely because your ESLint config was opting into type-aware linting. ESLint+typescript-eslint without type-aware linting only parses one file at a time the way Oxlint does. I'm sure you know all that, tried turning off type-aware linting & bumping package versions, and found Oxlint to still be much much faster. I just want to make sure folks don't see this publicly posted PR and think the two behaviors are comparable. Trying things out locally on my (apparently much slower π’) local machine, with 7dcf97e for 1-3:
Not saying you shouldn't make this change, or under a second for a full lint isn't absurdly blazingly fast (seriously, Oxlint is incredible). If you don't want type-aware linting then Oxlint is clearly the better tool for you. Just, this isn't a purely lateral change and isn't a directly comparable performance scenario. You moved from a slower tool with more functionality to a faster tool that does less. Anyway, don't mind me - I normally wouldn't post this for risk of coming off as cantankerous. But because this PR is getting shared on social media & folks are drawing incorrect conclusions from it (not your fault or anybody's fault at all!) I just wanted to clarify. Cheers! π |
|
@JoshuaKGoldberg any future roadmap, improvements, plans, or at least ideas on type-aware linting? maybe after TS is compiled with go? |
|
Yeah! The big one is typescript-eslint/typescript-eslint#10940, using tsgo as you suggested. It's technically blocked on tsgo being stable, but they did create initial IPC APIs (microsoft/typescript-go#711) that we'd love to see someone explore using. ESLint itself is also likely to eventually be restructured for better cross-file linting. eslint/rfcs#102 was the first stab; eslint/eslint#18830 also has ideas. But I don't know that any of that is going to land soon. Personally, I'm also interested in exploring a cross-file-safe cache in ESLint. Right now ESLint's cache doesn't recognize cross-file dependencies so you can't use The tl;dr is: yes eventually, but not anytime soon I'm afraid. |
|
I'm curious of the reason to put linting as part of pre-commit but not type checking? |
pre-commit is bonkers. push, maybe. pr ci - for sure. |
Problem
Any commit to the PostHog frontend is painful. Here's how long it takes to lint a staged TypeScript file, as part of the pre-commit hooks:
7-8 s to lint a single file. For the whole codebase: practically the same - there's insane overhead.
7-8 s to get any feedback whether the commit will go through, or if there's one more error to go back to and fix.
From my earlier tests, almost all this latency is in
typescript-eslint/parser. To lint just one file, it parses the whole codebase, and not particularly fast. I've been trying to find ways to optimize that while still using ESLint. There are none.Only two possible solutions:
Changes
@mariusandra and @adamleithp are working towards the frontend codebase being split up, but the parts of the code are tangled to a level preventing that linting optimization. It'll take weeks or months just to split each product into its own product folder.
But, of course someone has rewritten ESLint in Rust. The folks from Vite and Vitest made
oxlint. Oxlint just hit 1.0, and now is a drop-in replacement for ESLint.What if we try it, with all the same rules we've had in ESLint?
~200 ms to lint one file. (Whole codebase: 600 ms.)
For human intents and purposes, this makes pre-commit feedback instant.
Caveats
To make an omelette you have to crack a few eggs.
Oxlint doesn't support custom rules, so I had to work around out that. Rule by rule:
posthog/no-schema-index-importβ β built into ESlint and Oxlint, ruleno-restricted-imports(@rafaeelaudibert)posthog/no-spread-in-reduceβ β built into Oxlint, ruleno-accumulating-spread(it's actually much more robust, so much that it's caught many more violations that we should fix, hence I'll enable it in a separate PR @benjackwhite)posthog/warn-elementsβ ποΈ same asreact/forbid-elements, we only used it to have a secondreact/forbid-elementsconfig with level "warn" instead of "error", for the Ant Design βΒ Lemon UI migration β but this is no longer neededposthog/no-survey-string-constantsβHow did you test this code?
pnpm oxlint --quietNo changes other than formatting here.