deps: migrate to biomejs from eslint

nazarhussain commented 1 month ago

Motivation

Improve the performance for development environment. Where no other tool compete linting for biomejs, whole project less than 1s vs 43s.

yarn lint
yarn run v1.22.22
$ biome check packages/*/src packages/*/test
Checked 1664 files in 249ms. No fixes applied.
✨  Done in 0.47s.

yarn lint
yarn run v1.22.22
$ eslint --report-unused-disable-directives --color --ext .ts packages/*/src packages/*/test
✨  Done in 43.63s.

Description

These are the reasons to consider Biomejs for better option.

Biomejs is modern and all-in-one solution while eslint is base layer with many extra libraries on top.
There is an gray overlap between prettier and eslint rules, where BiomeJS combined both operations in one solution.
The Biomejs rules are grouped and easy to understand.
- Accessibility
- Complexity
- Correctness
- Nursery
- Performance
- Security
- Stype
- Suspicious
The linting output is unambiguous and very detailed.

NOTE:

Noteable difference you may encounter is that in biomejs there is no file level ignore option with code comment. you have to do that in configurations. Reason is that such file level comments never get focused once added.

Steps to test or reproduce

Run all tests.

codecov[bot] commented 1 month ago

Codecov Report

Attention: Patch coverage is 37.87879% with 41 lines in your changes missing coverage. Please review.

Project coverage is 49.13%. Comparing base (0d1fd9c) to head (ddd66bb). Report is 9 commits behind head on unstable.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## unstable #7108 +/- ## ============================================ + Coverage 49.04% 49.13% +0.09% ============================================ Files 596 597 +1 Lines 39743 39707 -36 Branches 2065 2084 +19 ============================================ + Hits 19493 19512 +19 + Misses 20209 20154 -55 Partials 41 41 ```

github-actions[bot] commented 1 month ago

Performance Report

✔️ no performance regression detected

Full benchmark results

by benchmarkbot/action

wemeetagain commented 1 month ago

What is the feature parity with our existing linter rules?

nflaig commented 1 month ago

What is the feature parity with our existing linter rules?

How can we determine that, I guess we would have to break every single eslint rule and see if the new rules catch the issue as well?

Do people run yarn lint locally or just rely on the extension to show errors pretty much instantly? I don't find myself running a full lint from CLI a lot

nazarhussain commented 1 month ago

What is the feature parity with our existing linter rules?

For that we need to rely on community and our own testing. There is biome migrate which is designed to handle eslint and prettier migration. Most of rules migration was done automatically. Rest I went over each eslint rule one by one manually to match corresponding in biomejs.

A few of biome recommended rules are turned off to match to our styling e.g. noForEach. And my plan is to turned these on one by one specially in suspicious and correctness category.

You can also check https://biomejs.dev/linter/rules-sources/

nazarhussain commented 1 month ago

Do people run yarn lint locally or just rely on the extension to show errors pretty much instantly? I don't find myself running a full lint from CLI a lot

Not sure about others, I heavily rely on the IDE feedback and while 3-4 projects open in the VSCode at the time, I often have to restart the Eslint server in our project, specially when inter-package dependencies are changed.

When I am about to finish some changes, I run full lint as well which is faster than waiting for the CI.

nflaig commented 1 month ago

I often have to restart the Eslint server in our project, specially when inter-package dependencies are changed.

yeah this one is annoying, this mostly happens if I change branches

nazarhussain commented 1 month ago

Here is the full rules comparison.

Rule/Settings	Biome	Note
globals.BigInt	javascript/globals/BigInt
ignorePatterns	vcs.useIgnoreFile	Also overrides.include to turn off some rules for specific files not in Git
@chainsafe/ndoe

file-extension-in-import	lint/correctness/useImportExtensions
no-deprecated-api	Diagnostics \| Biome

@typescript-eslint
await-thenable
ban-ts-comment
ban-types	lint/complexity/noBannedTypes
explicit-function-return-type
explicit-member-accessibility	lint/nursery/useConsistentMemberAccessibility
func-call-spacing
naming-convention	lint/style/useNamingConvention
no-array-constructor	lint/correctness/useArrayLiterals
no-duplicate-enum-values
no-explicit-any	lint/suspicious/noExplicitAny
no-extra-non-null-assertion	lint/suspicious/noExtraNonNullAssertion
no-floating-promises
no-loss-of-precision	lint/correctness/noPrecisionLoss
no-misused-new	lint/suspicious/noMisleadingInstantiator
no-namespace	lint/style/noNamespace
no-non-null-asserted-optional-chain
no-non-null-assertion	lint/style/noNonNullAssertion
no-require-imports	lint/nursery/noCommonJs
no-this-alias	lint/complexity/noUselessThisAlias
no-unnecessary-type-assertion
no-unnecessary-type-constraint	lint/complexity/noUselessTypeConstraint
no-unsafe-assignment
no-unsafe-call
no-unsafe-declaration-merging	lint/suspicious/noUnsafeDeclarationMerging
no-unsafe-member-access
no-unsafe-return
no-unused-expressions
no-unused-vars	lint/correctness/noUnusedVariables
no-var-requires
prefer-as-const	lint/style/useAsConstAssertion
restrict-template-expressions
return-await
semi
strict-boolean-expressions
triple-slash-reference
type-annotation-spacing

eslint-plugin-import
no-extraneous-dependencies	lint/correctness/noUndeclaredDependencies	!! Not working...
no-relative-packages
order	Imports Sorting \| Biome	Not identical but very similar to eslint plugin.

eslint-plugin-vitest
vitest/consistent-test-it
vitest/no-focused-tests	lint/suspicious/noFocusedTests
vitest/prefer-called-with
vitest/prefer-spy-on
expect-expect
no-identical-title
no-commented-out-tests
valid-title
valid-expect
valid-describe-callback
require-local-test-context-for-concurrent-snapshots
no-import-node-test

eslint
for-direction	lint/correctness/useValidForDirection
func-names
getter-return	lint/suspicious/useGetterReturn
new-parens
no-async-promise-executor	lint/suspicious/noAsyncPromiseExecutor
no-caller
no-case-declarations	lint/correctness/noSwitchDeclarations
no-class-assign	lint/suspicious/noClassAssign
no-compare-neg-zero	lint/suspicious/noCompareNegZero
no-cond-assign	lint/suspicious/noAssignInExpressions
no-console	lint/suspicious/noConsoleLog
no-const-assign	lint/suspicious/noAssignInExpressions
no-constant-condition	lint/correctness/noConstantCondition
no-control-regex	lint/suspicious/noControlCharactersInRegex
no-debugger	lint/suspicious/noDebugger
no-delete-var
no-dupe-args	lint/suspicious/noDuplicateParameters
no-dupe-class-members	lint/suspicious/noDuplicateClassMembers
no-dupe-else-if	lint/nursery/noDuplicateElseIf
no-dupe-keys	lint/suspicious/noDuplicateObjectKeys
no-duplicate-case	lint/suspicious/noDuplicateCase
no-empty	lint/suspicious/noEmptyBlockStatements
no-empty-character-class	lint/correctness/noEmptyCharacterClassInRegex
no-empty-pattern	lint/correctness/noEmptyPattern
no-ex-assign	lint/suspicious/noCatchAssign
no-extra-boolean-cast	lint/complexity/noExtraBooleanCast
no-extra-semi		Not needed as builtin formatter take care of it.
no-fallthrough	lint/suspicious/noFallthroughSwitchClause
no-func-assign	lint/suspicious/noFunctionAssign
no-global-assign	lint/suspicious/noGlobalAssign
no-import-assign	lint/suspicious/noImportAssign
no-inner-declarations	lint/correctness/noInnerDeclarations
no-invalid-regexp
no-irregular-whitespace	lint/nursery/noIrregularWhitespace
no-loss-of-precision	lint/correctness/noPrecisionLoss
no-misleading-character-class	lint/suspicious/noMisleadingCharacterClass
no-mixed-spaces-and-tabs
no-new-symbol	lint/correctness/noInvalidBuiltinInstantiation
no-nonoctal-decimal-escape	lint/correctness/noNonoctalDecimalEscape
no-obj-calls	lint/correctness/noGlobalObjectCalls
no-octal	lint/nursery/noOctalEscape
no-redeclare	lint/suspicious/noRedeclare
no-regex-spaces	lint/complexity/noMultipleSpacesInRegularExpressionLiterals
no-restricted-globals	lint/style/noRestrictedGlobals
no-restricted-imports	lint/nursery/noRestrictedImports
no-restricted-syntax
no-self-assign	lint/correctness/noSelfAssign
no-setter-return	lint/correctness/noSetterReturn
no-shadow-restricted-names	lint/suspicious/noShadowRestrictedNames
no-sparse-arrays	lint/suspicious/noSparseArray
no-this-before-super	lint/correctness/noUnreachableSuper
no-undef	lint/correctness/noUndeclaredVariables
no-unexpected-multiline
no-unreachable	lint/correctness/noUnreachable
no-unsafe-finally	lint/correctness/noUnsafeFinally
no-unsafe-negation	lint/suspicious/noUnsafeNegation
no-unsafe-optional-chaining	lint/correctness/noUnsafeOptionalChaining
no-unused-labels	lint/correctness/noUnusedLabels
no-useless-backreference
no-useless-catch	lint/complexity/noUselessCatch
no-useless-escape	lint/nursery/noUselessEscapeInRegex
no-var	lint/style/noVar
no-with	lint/complexity/noWith
object-curly-spacing
prefer-const	lint/style/useConst
quotes	javascript.formatter.quoteStyle
require-yield	lint/correctness/useYield
use-isnan	lint/correctness/useIsNan
valid-typeof	lint/suspicious/useValidTypeof

prettier
prettier	formatter.enabled	Differences with Prettier \| Biome

nflaig commented 1 month ago

Here is the full rules comparison.

This list seems incomplete, what about no-restricted-imports, no-restricted-syntax, no-restricted-globals? (likely more missing)

And how easy is it to have custom rules with biomejs like we have here https://github.com/ChainSafe/lodestar/blob/c4952eef7ee2e362e08a313c128e22e394bc44e0/.eslintrc.js#L140-L146 and here https://github.com/ChainSafe/lodestar/blob/c4952eef7ee2e362e08a313c128e22e394bc44e0/.eslintrc.js#L266-L275

nazarhussain commented 1 month ago

This list seems incomplete, what about no-restricted-imports, no-restricted-syntax, no-restricted-globals? (likely more missing)

Some rows were missed during copying from the markdown notes to github. Updated the list now.

nazarhussain commented 1 month ago

And how easy is it to have custom rules with biomejs like we have here

Currently we can't! But that's a topic to be covered soon. Biome plugin support #2463. The proposal for that is up here RFC: Biome Plugins Proposal #1762.

What community is doing right now is to have biomejs as standard and eslint only for custom rules to get both performance and customization.

Keeping that list in focus, I don't think we need any custom rules at the moment or near future. There are tons of beneficial rules which are enabled with recommended settings under the hood already.

nazarhussain commented 1 month ago

@wemeetagain Created a PR myself for Biome to have that feature soon. https://github.com/biomejs/biome/pull/4233

nazarhussain commented 1 month ago

@wemeetagain I had been investigating and found some weird behavior from eslint. e.g. We have that rule to enforce return type for functions.

https://github.com/ChainSafe/lodestar/blob/068fbae928989295706443887c645d476bb95695/.eslintrc.js#L35

But found a lot of occurrences in the code which is not been linted properly for that rule. For a lot of these cases I don't consider are expressions, so the definition of expression is not clear from the eslint perspective.

https://github.com/ChainSafe/lodestar/blob/068fbae928989295706443887c645d476bb95695/packages/api/src/utils/urlFormat.ts#L85 https://github.com/ChainSafe/lodestar/blob/068fbae928989295706443887c645d476bb95695/packages/beacon-node/src/api/impl/beacon/index.ts#L25 https://github.com/ChainSafe/lodestar/blob/068fbae928989295706443887c645d476bb95695/packages/beacon-node/src/api/impl/beacon/rewards/index.ts#L15 https://github.com/ChainSafe/lodestar/blob/068fbae928989295706443887c645d476bb95695/packages/cli/test/utils/crucible/assertions/defaults/attestationParticipationAssertion.ts#L27

nflaig commented 1 month ago

But found a lot of occurrences in the code which is not been linted properly for that rule.

those are all functions that are returned inside another function which has the return type declared

nazarhussain commented 1 month ago

those are all functions that are returned inside another function which has the return type declared

That's what I explained above I don't consider a lot of these cases as expressions so allowExpresions options seems wage to me.

nflaig commented 1 month ago

allowExpresions options seems wage to me.

I am not sure this rule is even evaluated for those, have you tried if you set allowExpresions to false? I would expect that there is still no error because return type is inferred from top-level return or interface

nazarhussain commented 1 month ago

I am not sure this rule is even evaluated for those, have you tried if you set allowExpresions to false? I would expect that there is still no error because return type is inferred from top-level return or interface

Yes it's evaluated and consider all parameters of a function or an attribute of an object as an expression. So if any function is passed as arguments or any function passed to object attributes it skip error for these. I am not aligned with their definition of the expression in these cases.

wemeetagain commented 1 week ago

:tada: This PR is included in v1.23.0 :tada:

ChainSafe / lodestar

deps: migrate to biomejs from eslint #7108

Codecov Report

Performance Report