Skip to content

Conversation

natemoo-re
Copy link
Member

@natemoo-re natemoo-re commented Feb 20, 2025

See https://github.com/getsentry/frontend-tsc/issues/67 for more details. We've had numerous discussions about Biome vs ESLint in the Frontend TSC. Biome is in a half-implemented state and having multiple linters is a frequent source of confusion, so this PR is a bias towards action and just removes Biome.

There's an important trade-off here! Rule coverage is not exactly 1:1 and some @typescript-eslint rules that replicate Biome's behavior require type-checking to be enabled. Type-aware linting can be painfully slow on the first run (100s+) in a project of our size, but subsequent runs from the cache are much faster.

So... is the performance hit worth the tradeoff? IMO yes—simplifying our dependency tree and removing yet another configuration file helps everyone understand our tooling setup. We're also eliminating a potential source of divergence between the tools. These small papercuts really add up over time.

If Biome adds support for all the rules we use down the road, it will be fairly trivial to revisit this decision.

enabled biome eslint commit
useOptionalChain @typescript-eslint/prefer-optional-chain 33126cf
useExportType @typescript-eslint/consistent-type-exports aadec31
useImportType @typescript-eslint/consistent-type-imports aadec31
noBarrelFile eslint-plugin-barrel-files is broken
noApproximativeNumericConstant no equivalent
noMisrefactoredShorthandAssign no equivalent
noShoutyConstants no equivalent

closes https://github.com/getsentry/frontend-tsc/issues/67

@github-actions github-actions bot added Scope: Frontend Automatically applied to PRs that change frontend components Scope: Backend Automatically applied to PRs that change backend components labels Feb 20, 2025
Copy link
Contributor

🚨 Warning: This pull request contains Frontend and Backend changes!

It's discouraged to make changes to Sentry's Frontend and Backend in a single pull request. The Frontend and Backend are not atomically deployed. If the changes are interdependent of each other, they must be separated into two pull requests and be made forward or backwards compatible, such that the Backend or Frontend can be safely deployed independently.

Have questions? Please ask in the #discuss-dev-infra channel.

@ryan953
Copy link
Member

ryan953 commented Feb 20, 2025

I have been auditing the config as it applies to JS files. This is the list of biome rules we have enabled for js & their eslint equiv. https://gist.github.com/ryan953/d4b3949118090bec36ab50efbef3394c

A green checkmark is added to indicate that we have that eslint config rule enabled:

@TkDodo
Copy link
Collaborator

TkDodo commented Feb 21, 2025

⛔ noBarrelFile

I’ve written this as a custom lint rule before, it wasn’t that complicated and I’m quite sure I could add it as a custom rule for sentry too if we want that? Would also be a good precedent for having a custom rule in the repo because I don’t think we have that yet; setting that up once is likely a bit more work, but it would enable us to add more custom rules in the future.

@BYK
Copy link
Member

BYK commented Feb 24, 2025

Can we fix the caching issue by putting the cache in the repo or pulling it from some place if it's not there?

@TkDodo
Copy link
Collaborator

TkDodo commented Feb 24, 2025

Can we fix the caching issue by putting the cache in the repo or pulling it from some place if it's not there?

using the eslint cache is fundamentally incompatible with rules that work across multiple files, which is pretty much all rules that need type information, so it’s not recommended:

https://typescript-eslint.io/troubleshooting/faqs/eslint/#can-i-use-eslints---cache-with-typescript-eslint

@anonrig
Copy link
Contributor

anonrig commented Mar 11, 2025

@natemoo-re the link you shared is not available outside of the sentry organization.

@natemoo-re
Copy link
Member Author

Updated now that #88072 has landed (cc @TkDodo)

Copy link
Member

@ryan953 ryan953 left a comment

Choose a reason for hiding this comment

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

we will need to run the linter again. Looks like static/app/views/explore/contexts/logs/logsPageParams.tsx has new changes in it

Copy link
Collaborator

@TkDodo TkDodo left a comment

Choose a reason for hiding this comment

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

interesting to see so many change around prefer-optional-chain. The result is definitely better than what we had before 🚀

@natemoo-re natemoo-re marked this pull request as ready for review April 17, 2025 01:50
@natemoo-re natemoo-re requested review from a team as code owners April 17, 2025 01:50
Copy link

codecov bot commented Apr 17, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #85552      +/-   ##
==========================================
+ Coverage   81.92%   87.79%   +5.86%     
==========================================
  Files       10205    10210       +5     
  Lines      575457   575644     +187     
  Branches    22657    22586      -71     
==========================================
+ Hits       471451   505375   +33924     
+ Misses     103572    69835   -33737     
  Partials      434      434              

Copy link
Member

@ryan953 ryan953 left a comment

Choose a reason for hiding this comment

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

Lets merge!

@natemoo-re natemoo-re merged commit 6ca3ed5 into master Apr 17, 2025
68 checks passed
@natemoo-re natemoo-re deleted the chore/remove-biome branch April 17, 2025 02:56
andrewshie-sentry pushed a commit that referenced this pull request Apr 22, 2025
**See getsentry/frontend-tsc#67 for more
details.** We've had numerous discussions about Biome vs ESLint in the
Frontend TSC. Biome is in a half-implemented state and having multiple
linters is a frequent source of confusion, so this PR is a _bias towards
action_ and just removes Biome.

**There's an important trade-off here!** Rule coverage is not exactly
1:1 and some `@typescript-eslint` rules that replicate Biome's behavior
require type-checking to be enabled. Type-aware linting can be painfully
slow on the first run (100s+) in a project of our size, but subsequent
runs from the cache are much faster.

So... is the performance hit worth the tradeoff? IMO yes—simplifying our
dependency tree and removing yet another configuration file helps
everyone understand our tooling setup. We're also eliminating a
potential source of divergence between the tools. These small papercuts
really add up over time.

If Biome adds support for all the rules we use down the road, it will be
fairly trivial to revisit this decision.

| enabled | biome | eslint | commit |
|--------|--------|--------|--------|
| ✅ | `useOptionalChain` | `@typescript-eslint/prefer-optional-chain` |
33126cf |
| ✅ | `useExportType` | `@typescript-eslint/consistent-type-exports` |
aadec31 |
| ✅ | `useImportType` | `@typescript-eslint/consistent-type-imports` |
aadec31 |
| ⛔ | `noBarrelFile` |
[`eslint-plugin-barrel-files`](https://github.com/thepassle/eslint-plugin-barrel-files)
is broken | |
| ⛔ | `noApproximativeNumericConstant` | no equivalent |  |
| ⛔ | `noMisrefactoredShorthandAssign` | no equivalent |   |
| ⛔ | `noShoutyConstants` | no equivalent |   |

closes getsentry/frontend-tsc#67

---------

Co-authored-by: getsantry[bot] <66042841+getsantry[bot]@users.noreply.github.com>
@github-actions github-actions bot locked and limited conversation to collaborators May 2, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Scope: Backend Automatically applied to PRs that change backend components Scope: Frontend Automatically applied to PRs that change frontend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants