Skip to content
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.

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

chore: Enable a few eslint rules that overlap with biomes rule list #83693

Merged
merged 2 commits into from
Jan 18, 2025

Conversation

ryan953
Copy link
Member

@ryan953 ryan953 commented Jan 18, 2025

Looking at our biome config here we can see all the linty rules that are enabled for JavaScript files:

sentry/biome.json

Lines 14 to 87 in fc9ccb0

"rules": {
"recommended": false,
"a11y": {
"noBlankTarget": "error"
},
"correctness": {
"noGlobalObjectCalls": "error",
"noUnreachable": "error",
"useIsNan": "error",
"noUnusedPrivateClassMembers": "error",
"noInvalidUseBeforeDeclaration": "error",
"noNodejsModules": "error"
},
"complexity": {
"useFlatMap": "error",
"useOptionalChain": "error",
"noEmptyTypeParameters": "error",
"noUselessLoneBlockStatements": "error",
"noUselessEmptyExport": "error",
"noUselessConstructor": "error",
"noUselessTypeConstraint": "error",
"noExcessiveNestedTestSuites": "error"
},
"nursery": {
"noRestrictedImports": {
"level": "warn",
"options": {
"paths": {}
}
}
},
"performance": {
"noBarrelFile": "error"
},
"security": {
"noDangerouslySetInnerHtmlWithChildren": "error"
},
"suspicious": {
"noDebugger": "error",
"noDoubleEquals": "error",
"noDuplicateJsxProps": "error",
"noDuplicateObjectKeys": "error",
"noDuplicateParameters": "error",
"noDuplicateCase": "error",
"noFallthroughSwitchClause": "error",
"noRedeclare": "error",
"noSparseArray": "error",
"noUnsafeDeclarationMerging": "error",
"noUnsafeNegation": "error",
"useIsArray": "error",
"noApproximativeNumericConstant": "error",
"noMisrefactoredShorthandAssign": "error",
"useAwait": "error",
"useNamespaceKeyword": "error",
"noFocusedTests": "error",
"noDuplicateTestHooks": "error"
},
"style": {
"noCommaOperator": "error",
"noShoutyConstants": "error",
"noParameterProperties": "error",
"noVar": "error",
"useConst": "error",
"useShorthandFunctionType": "error",
"useExportType": "error",
"useImportType": "error",
"useNodejsImportProtocol": "error",
"useLiteralEnumMembers": "error",
"useEnumInitializers": "error",
"useAsConstAssertion": "error",
"useBlockStatements": "error"
}
}
},

The docs provide the name of the equiv rule inside eslint core, or inside other eslint plugins if available. I mapped the names of the biome rules to those eslint rules here (below).

Then I went through each rule and enabled it within the codebase. It was meant to be trivial because biome should've prevented violations, but a few places did need to be fixed up. Anywhere that had a biome-ignore statement needed an eslint one too.

There are still a few rules that biome implements which are not in our eslint config right now. The categories are broadly:

  1. Rules from plugins that i haven't installed yet, like from unicorn/* or from barrel-files/*. I'll import these plugins in a followup and check things off the list.
  2. Some rules have no eslint equiv: noApproximativeNumericConstant & noMisrefactoredShorthandAssign, they're also not important to have enabled imo
  3. Some rules have an eslint equiv but are also not important: noShoutyConstants & noInvalidUseBeforeDeclaration would cause lots of churn in the repo for example. Others are repalced by prettier.
  4. Some rules depend on type annotations, these are challenging because they are often nice to have but type annotations are slow in eslint. It would make pre-commit checks really terrible if that was enabled. consistent-type-imports is an example.

I intend to get plugins to fill these specific biome gaps while maintaining pre-commit perf.


The rules table is here:

Biome Rule ESLint Rule Handled by
noBlankTarget react/jsx-no-target-blank ✅ react/recommended
noGlobalObjectCalls no-obj-calls ✅ tsc
noUnreachable no-unreachable ✅ tsc
useIsNan use-isnan ✅ eslint/recommended
noUnusedPrivateClassMembers no-unused-private-class-members ✅ eslint/recommended
noInvalidUseBeforeDeclaration @typescript-eslint/no-use-before-define ✅ Disabled in sentry
noNodejsModules import/no-nodejs-modules ✅ eslint.config (except scripts)
useFlatMap unicorn/prefer-array-flat-map TODO
useOptionalChain @typescript-eslint/prefer-optional-chain TODO typescript/stylistic-type-checked
noEmptyTypeParameters Prettier? TODO
noUselessLoneBlockStatements no-lone-blocks ✅ eslint.config
noUselessEmptyExport @typescript-eslint/no-useless-empty-export ✅ eslint.config
noUselessConstructor @typescript-eslint/no-useless-constructor ✅ typescript/strict
noUselessTypeConstraint @typescript-eslint/no-unnecessary-type-constraint ✅ typescript/recommended
noExcessiveNestedTestSuites jest/max-nested-describe (max=5) ✅ eslint.config
noBarrelFile barrel-files/avoid-barrel-files TODO
noDangerouslySetInnerHtmlWithChildren react/no-danger-with-children ✅ react/recommended
noDebugger no-debugger ✅ eslint/recommended
noDoubleEquals eqeqeq ✅ eslint.config
noDuplicateJsxProps react/jsx-no-duplicate-props ✅ react/recommended
noDuplicateObjectKeys no-dupe-keys ✅ tsc
noDuplicateParameters no-dupe-args ✅ tsc
noDuplicateCase no-duplicate-case ✅ eslint/recommended
noFallthroughSwitchClause no-fallthrough ✅ eslint/recommended
noRedeclare @typescript-eslint/no-redeclare ✅ tsc
noSparseArray no-sparse-arrays ✅ eslint recommended
noUnsafeDeclarationMerging @typescript-eslint/no-unsafe-declaration-merging ✅ typescript/recommended
noUnsafeNegation no-unsafe-negation ✅ tsc
useIsArray unicorn/no-instanceof-array TODO
noApproximativeNumericConstant approx_constant TODO
noMisrefactoredShorthandAssign misrefactored_assign_op TODO
useAwait require-await & @typescript-eslint/require-await ✅ esint.config
useNamespaceKeyword @typescript-eslint/prefer-namespace-keyword ✅ typescript/recommended
noFocusedTests jest/no-focused-tests ✅ jest/recommended
noDuplicateTestHooks jest/no-duplicate-hooks ✅ eslint.config
noCommaOperator no-sequences ✅ eslint.config
noShoutyConstants ??? TODO
noParameterProperties @typescript-eslint/parameter-properties ✅ typescript
noVar no-var ✅ eslint.config
useConst prefer-const ✅ tsc
useShorthandFunctionType @typescript-eslint/prefer-function-type ✅ typescript/stylistic
useExportType @typescript-eslint/consistent-type-exports TODO requires types
useImportType @typescript-eslint/consistent-type-imports TODO requires types
useNodejsImportProtocol unicorn/prefer-node-protocol TODO
useLiteralEnumMembers @typescript-eslint/prefer-literal-enum-member ✅ typescript/strict
useEnumInitializers @typescript-eslint/prefer-enum-initializers
useAsConstAssertion @typescript-eslint/prefer-as-const ✅ typescript/recommended
useBlockStatements curly ✅ prettier

New plugins:

@ryan953 ryan953 requested review from a team as code owners January 18, 2025 01:28
@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Jan 18, 2025
Comment on lines +9 to +14
export type UIFramesRendererConstructor = new (
canvas: HTMLCanvasElement,
uiFrames: UIFrames,
theme: FlamegraphTheme,
options?: {draw_border: boolean}
) => UIFramesRenderer;
Copy link
Member Author

Choose a reason for hiding this comment

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

Something formatting it this way.

@ryan953 ryan953 enabled auto-merge (squash) January 18, 2025 01:52
@ryan953 ryan953 merged commit 9543dd7 into master Jan 18, 2025
43 checks passed
@ryan953 ryan953 deleted the ryan953/eslint-biome-compat branch January 18, 2025 01:58
ryan953 added a commit that referenced this pull request Jan 21, 2025
A little followup to #83693

This enables 3 rules from biome that are implemented in eslint as part
of this plugin.

This plugin has a lot of rules, the recommended set is too opinionated,
but a handful would be nice to enable inside sentry. I'll make a list
with TODO comments as a followup, and then work against that list rule
by rule in followup PRs. For now we just need to pluck these three rules
to match what's already happening.
andrewshie-sentry pushed a commit that referenced this pull request Jan 22, 2025
…83693)

Looking at our biome config here we can see all the linty rules that are
enabled for JavaScript files:
https://github.com/getsentry/sentry/blob/fc9ccb0f4754939a4cf19bfdb08b526f5a491f5a/biome.json#L14-L87

The docs provide the name of the equiv rule inside eslint core, or
inside other eslint plugins if available. I mapped the names of the
biome rules to those eslint rules here (below).

Then I went through each rule and enabled it within the codebase. It was
meant to be trivial because biome should've prevented violations, but a
few places did need to be fixed up. Anywhere that had a biome-ignore
statement needed an eslint one too.

There are still a few rules that biome implements which are not in our
eslint config right now. The categories are broadly:
1. Rules from plugins that i haven't installed yet, like from
`unicorn/*` or from `barrel-files/*`. I'll import these plugins in a
followup and check things off the list.
2. Some rules have no eslint equiv: `noApproximativeNumericConstant` &
`noMisrefactoredShorthandAssign`, they're also not important to have
enabled imo
3. Some rules have an eslint equiv but are also not important:
`noShoutyConstants` & `noInvalidUseBeforeDeclaration` would cause lots
of churn in the repo for example. Others are repalced by prettier.
4. Some rules depend on type annotations, these are challenging because
they are often nice to have but type annotations are _slow_ in eslint.
It would make pre-commit checks really terrible if that was enabled.
`consistent-type-imports` is an example.

I intend to get plugins to fill these specific biome gaps while
maintaining pre-commit perf.

----

The rules table is here:

| Biome Rule | ESLint Rule | Handled by |
| ------------------------------------- |
------------------------------------------------- |
------------------------ |
| noBlankTarget | react/jsx-no-target-blank | ✅ react/recommended
| noGlobalObjectCalls | no-obj-calls | ✅ tsc
| noUnreachable | no-unreachable | ✅ tsc
| useIsNan | use-isnan | ✅ eslint/recommended
| noUnusedPrivateClassMembers | no-unused-private-class-members | ✅
eslint/recommended
| noInvalidUseBeforeDeclaration |
@typescript-eslint/no-use-before-define | ✅ Disabled in sentry
| noNodejsModules | import/no-nodejs-modules | ✅ eslint.config (except
scripts)
| useFlatMap | unicorn/prefer-array-flat-map | TODO
| useOptionalChain | @typescript-eslint/prefer-optional-chain | TODO
typescript/stylistic-type-checked
| noEmptyTypeParameters | Prettier? | TODO
| noUselessLoneBlockStatements | no-lone-blocks | ✅ eslint.config
| noUselessEmptyExport | @typescript-eslint/no-useless-empty-export | ✅
eslint.config
| noUselessConstructor | @typescript-eslint/no-useless-constructor | ✅
typescript/strict
| noUselessTypeConstraint |
@typescript-eslint/no-unnecessary-type-constraint | ✅
typescript/recommended
| noExcessiveNestedTestSuites | jest/max-nested-describe (max=5) | ✅
eslint.config
| noBarrelFile | barrel-files/avoid-barrel-files | TODO
| noDangerouslySetInnerHtmlWithChildren | react/no-danger-with-children
| ✅ react/recommended
| noDebugger | no-debugger | ✅ eslint/recommended
| noDoubleEquals | eqeqeq | ✅ eslint.config
| noDuplicateJsxProps | react/jsx-no-duplicate-props | ✅
react/recommended
| noDuplicateObjectKeys | no-dupe-keys | ✅ tsc
| noDuplicateParameters | no-dupe-args | ✅ tsc
| noDuplicateCase | no-duplicate-case | ✅ eslint/recommended
| noFallthroughSwitchClause | no-fallthrough | ✅ eslint/recommended
| noRedeclare | @typescript-eslint/no-redeclare | ✅ tsc
| noSparseArray | no-sparse-arrays | ✅ eslint recommended
| noUnsafeDeclarationMerging |
@typescript-eslint/no-unsafe-declaration-merging | ✅
typescript/recommended
| noUnsafeNegation | no-unsafe-negation | ✅ tsc
| useIsArray | unicorn/no-instanceof-array | TODO
| noApproximativeNumericConstant | approx_constant | TODO
| noMisrefactoredShorthandAssign | misrefactored_assign_op | TODO
| useAwait | require-await & @typescript-eslint/require-await | ✅
esint.config
| useNamespaceKeyword | @typescript-eslint/prefer-namespace-keyword | ✅
typescript/recommended
| noFocusedTests | jest/no-focused-tests | ✅ jest/recommended
| noDuplicateTestHooks | jest/no-duplicate-hooks | ✅ eslint.config
| noCommaOperator | no-sequences | ✅ eslint.config
| noShoutyConstants | ??? | TODO
| noParameterProperties | @typescript-eslint/parameter-properties | ✅
typescript
| noVar | no-var | ✅ eslint.config
| useConst | prefer-const | ✅ tsc
| useShorthandFunctionType | @typescript-eslint/prefer-function-type | ✅
typescript/stylistic
| useExportType | @typescript-eslint/consistent-type-exports | TODO
requires types
| useImportType | @typescript-eslint/consistent-type-imports | TODO
requires types
| useNodejsImportProtocol | unicorn/prefer-node-protocol | TODO
| useLiteralEnumMembers | @typescript-eslint/prefer-literal-enum-member
| ✅ typescript/strict
| useEnumInitializers | @typescript-eslint/prefer-enum-initializers | ✅
| useAsConstAssertion | @typescript-eslint/prefer-as-const | ✅
typescript/recommended
| useBlockStatements | curly | ✅ prettier

New plugins: 
-
https://github.com/sindresorhus/eslint-plugin-unicorn/blob/main/docs/rules/prefer-array-flat-map.md
-
https://github.com/thepassle/eslint-plugin-barrel-files/blob/main/docs/rules/avoid-barrel-files.md
- https://tanstack.com/query/v5/docs/eslint/eslint-plugin-query
andrewshie-sentry pushed a commit that referenced this pull request Jan 22, 2025
A little followup to #83693

This enables 3 rules from biome that are implemented in eslint as part
of this plugin.

This plugin has a lot of rules, the recommended set is too opinionated,
but a handful would be nice to enable inside sentry. I'll make a list
with TODO comments as a followup, and then work against that list rule
by rule in followup PRs. For now we just need to pluck these three rules
to match what's already happening.
@github-actions github-actions bot locked and limited conversation to collaborators Feb 2, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Frontend Automatically applied to PRs that change frontend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants