8000 chore(frontend): Migrate from 🐌 ESLint to ⚡ Oxlint by Twixes · Pull Request #33714 · PostHog/posthog · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

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

wants to merge 8 commits into from
+641 −2,047

Conversation

Twixes
Copy link
Member
@Twixes Twixes commented Jun 16, 2025 •
edited
Loading

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.

greptile-apps[bot]
greptile-apps bot reviewed Jun 16, 2025
View reviewed changes
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 😍

graphite-app[bot]
graphite-app bot reviewed Jun 16, 2025
View reviewed changes
adamleithp
adamleithp approved these changes Jun 16, 2025
View reviewed changes
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 •
edited
Loading

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 GitHub Actions
Copy link
Contributor
github-actions bot commented Jun 16, 2025 •
edited
Loading

Size Change: -262 B (-0.01%)

Total Size: 2.57 MB

ℹ️ View Unchanged
Filename Size Change
frontend/dist/toolbar.js 2.57 MB -262 B (-0.01%)

compressed-size-action

mariusandra
mariusandra approved these changes Jun 16, 2025
View reviewed changes
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 🙌

rafaeelaudibert
rafaeelaudibert approved these changes Jun 16, 2025
View reviewed changes
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

common/eslint_rules/no-survey-string-constants.js Outdated
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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@graphite-app graphite-app[bot] graphite-app[bot] left review comments

@greptile-apps greptile-apps[bot] greptile-apps[bot] left review comments

@mariusandra mariusandra mariusandra approved these changes

@adamleithp adamleithp adamleithp approved these changes

@rafaeelaudibert rafaeelaudibert rafaeelaudibert approved these changes

@pauldambra pauldambra Awaiting requested review from pauldambra

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Twixes @benjackwhite @mariusandra @adamleithp @rafaeelaudibert
0