posthog icon indicating copy to clipboard operation
posthog copied to clipboard

chore(frontend): Migrate from 🐌 ESLint to ⚡ Oxlint

Open Twixes opened this issue 6 months ago â€ĸ 3 comments

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:

  1. 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)
  2. 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:

  1. posthog/no-schema-index-import – ✅ built into ESlint and Oxlint, rule no-restricted-imports (@rafaeelaudibert)
  2. posthog/no-spread-in-reduce – ✅ built into Oxlint, rule no-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)
  3. posthog/warn-elements – đŸ—‘ī¸ same as react/forbid-elements, we only used it to have a second react/forbid-elements config with level "warn" instead of "error", for the Ant Design →  Lemon UI migration – but this is no longer needed
  4. posthog/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.

Twixes avatar Jun 16 '25 09:06 Twixes

This is gonna be my PR of the year 😍

benjackwhite avatar Jun 16 '25 09:06 benjackwhite

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:

  1. simple-import-sort - we have to fall back to ESLint's/Oxlint's built in sort-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)
  2. unused-imports - replaced by built-in no-unused-vars rule
  3. react-google-translate - this did potentially flag real issues (#27521), though it was set to warn-only, so it seems like an acceptable loss (@havenbarnes)
  4. storybook - sad to lose, but nothing critical (rules)
  5. eslint-comments - nice-to-have
  6. compat - seems this was only used for writing custom rules, which we won't be doing now
  7. cypress - 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 the unused-imports plugin), as well as unused error in catch (error) (which is new and we have a bunch of violations for)
  • simple-import-sort/imports - because sort-imports has a different sorting order, it means some changes

Twixes avatar Jun 16 '25 10:06 Twixes

Size Change: 0 B

Total Size: 2.58 MB

â„šī¸ View Unchanged
Filename Size
frontend/dist/toolbar.js 2.58 MB

compressed-size-action

github-actions[bot] avatar Jun 16 '25 10:06 github-actions[bot]

I just had to bump node memory to get eslint to run in lint-staged

this makes me so happy

pauldambra avatar Jun 23 '25 10:06 pauldambra

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:

  1. Running the same time pnpm eslint ...ts command : ~13s
  2. Bumping tseslint to the latest, 8.35.1: ~12s
  3. Disabling type-aware linting & switching from recommended-type-checked to recommended: ~2s
  4. Running the same time pnpm oxlint ...ts command: ~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 avatar Jul 02 '25 12:07 JoshuaKGoldberg

@JoshuaKGoldberg any future roadmap, improvements, plans, or at least ideas on type-aware linting? maybe after TS is compiled with go?

ColtHands avatar Jul 02 '25 12:07 ColtHands

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.

JoshuaKGoldberg avatar Jul 02 '25 13:07 JoshuaKGoldberg

I'm curious of the reason to put linting as part of pre-commit but not type checking?

ArnaudBarre avatar Jul 02 '25 15:07 ArnaudBarre

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.

patroza avatar Jul 03 '25 15:07 patroza