Skip to content

Conversation

@Twixes
Copy link
Member

@Twixes Twixes commented Jun 16, 2025

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.

Copy link
Contributor

@greptile-apps greptile-apps bot left a 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.js and custom ESLint rules in favor of .oxlintrc.json with 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-elements rule as Ant Design β†’ Lemon UI migration is complete
  • Simplified React component exports by standardizing memo wrapper usage and function naming
  • Sacrificed no-survey-string-constants custom rule as it cannot be replicated in Oxlint

41 files reviewed, 4 comments
Edit PR Review Bot Settings | Greptile

@benjackwhite
Copy link
Contributor

This is gonna be my PR of the year 😍

Copy link
Contributor

@adamleithp adamleithp left a 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

@Twixes
Copy link
Member Author

Twixes commented Jun 16, 2025

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 (chore(): add eslint-plugin-react-google-translate to posthog appΒ #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

@github-actions
Copy link
Contributor

github-actions bot commented Jun 16, 2025

Size Change: 0 B

Total Size: 2.58 MB

ℹ️ View Unchanged
Filename Size
frontend/dist/toolbar.js 2.58 MB

compressed-size-action

Copy link
Collaborator

@mariusandra mariusandra left a comment

Choose a reason for hiding this comment

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

Amazing πŸ™Œ

Copy link
Member

@rafaeelaudibert rafaeelaudibert left a 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

Copy link
Member

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, removed

@Twixes Twixes enabled auto-merge (squash) June 23, 2025 10:12
@pauldambra
Copy link
Member

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",
Copy link
Member

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 🀷 )

@Twixes Twixes merged commit b4199b5 into master Jun 23, 2025
121 checks passed
@Twixes Twixes deleted the oxlint branch June 23, 2025 10:48
@JoshuaKGoldberg
Copy link

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 7dcf97e 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! πŸ™‚

@ColtHands
Copy link

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

@JoshuaKGoldberg
Copy link

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 --cache with typed linting. eslint/eslint#19869 has more information. I also prototyped it in flint-fyi/flint#113 and hope to pull those learnings into other linters.

The tl;dr is: yes eventually, but not anytime soon I'm afraid.

@ArnaudBarre
Copy link

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

@patroza
Copy link

patroza commented Jul 3, 2025

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.

@PostHog PostHog locked as off-topic and limited conversation to collaborators Jul 3, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.