chore(frontend): Migrate from đ ESLint to ⥠Oxlint
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:
> time pnpm eslint frontend/src/scenes/max/maxContextLogic.ts
________________________________________________________
Executed in 7.74 secs fish external
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.
Latency is the mind killer. Latency is the little-death that brings total obliteration.
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:
- split up TypeScript codebase into packages (1 package per product), so that only a subset of TypeScript needs to be parsed each time (and for the rest of the codebase we'd have lightweight TS declaration files)
- rewrite ESLint in Rust
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?
> time pnpm oxlint frontend/src/scenes/max/maxContextLogic.ts
Found 0 warnings and 0 errors.
Finished in 3ms on 1 file using 12 threads.
________________________________________________________
Executed in 231.59 millis fish external
~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â â ī¸ this one we unfortunately can't replicate with Oxlint, it's too custom; considering the good of engineering overall, this is a sacrifice that feels entirely acceptable (@lucasheriques)
How did you test this code?
pnpm oxlint --quiet
No changes other than formatting here.
This is gonna be my PR of the year đ
More eggs to be cracked
I neglected to mention two aspects in the PR description.
Missing plugins
Oxlint 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:
simple-import-sort- we have to fall back to ESLint's/Oxlint's built insort-imports, where the major downside is the lack of an autofix yet â this is the biggest downside of the whole migration IMO, though still seems worth the speed gains (and I'm confident the Oxlint team and community will make this better too)unused-imports- replaced by built-inno-unused-varsrulereact-google-translate- this did potentially flag real issues (#27521), though it was set to warn-only, so it seems like an acceptable loss (@havenbarnes)storybook- sad to lose, but nothing critical (rules)eslint-comments- nice-to-havecompat- seems this was only used for writing custom rules, which we won't be doing nowcypress- we're moving away from Cypress anyway
Rule rigmarole
Some 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:
no-unused-varsâ as opposed to our older ESLint version, this also catches unused imports (replacing theunused-importsplugin), as well as unusederrorincatch (error)(which is new and we have a bunch of violations for)simple-import-sort/imports- becausesort-importshas a different sorting order, it means some changes
Size Change: 0 B
Total Size: 2.58 MB
âšī¸ View Unchanged
| Filename | Size |
|---|---|
frontend/dist/toolbar.js |
2.58 MB |
I just had to bump node memory to get eslint to run in lint-staged
this makes me so happy
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
đ 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 7dcf97e8 for 1-3:
- Running the same
time pnpm eslint ...tscommand : ~13s - Bumping tseslint to the latest, 8.35.1: ~12s
- Disabling type-aware linting & switching from
recommended-type-checkedtorecommended: ~2s - Running the same
time pnpm oxlint ...tscommand: ~0.4s
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 https://github.com/typescript-eslint/typescript-eslint/issues/10940, using tsgo as you suggested. It's technically blocked on tsgo being stable, but they did create initial IPC APIs (https://github.com/microsoft/typescript-go/pull/711) that we'd love to see someone explore using.
ESLint itself is also likely to eventually be restructured for better cross-file linting. https://github.com/eslint/rfcs/pull/102 was the first stab; https://github.com/eslint/eslint/discussions/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 --cache with typed linting. https://github.com/eslint/eslint/issues/19869 has more information. I also prototyped it in https://github.com/JoshuaKGoldberg/flint/pull/113 and hope to pull those learnings into other linters.
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?
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.