deps: migrate to biomejs from eslint #7108
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ 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 |
Performance Report✔️ no performance regression detected Full benchmark results
|
|
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 |
For that we need to rely on community and our own testing. There is A few of You can also check https://biomejs.dev/linter/rules-sources/ |
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. |
yeah this one is annoying, this mostly happens if I change branches |
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. |
| @@ -225,7 +224,7 @@ export type Endpoints = { | |||
| >; | |||
| }; | |||
|
|
|||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |||
| // biome-ignore lint/suspicious/noExplicitAny: <explanation> | |||
There was a problem hiding this comment.
would be good to add an explanation here (and in all other places), otherwise we set a bad precedence
There was a problem hiding this comment.
There should be, but as the earlier code comments didn't had that so we don't have it for now.
It's good that biome enforce to provide some explanation if you disable any rule. I would suggest to add those explanations later as someone touches the code. Each occurrence may have different reason and I can't do it globally for all files.
| @@ -289,7 +288,6 @@ export function getTypeByEvent(config: ChainForkConfig): {[K in EventType]: Type | |||
| }; | |||
| } | |||
|
|
|||
| // eslint-disable-next-line @typescript-eslint/explicit-function-return-type | |||
There was a problem hiding this comment.
so return types don't need to be explicit anymore?
There was a problem hiding this comment.
Currently for that rule, biomejs didn't differentiate between functions and anonymous functions. So we get over a thousand errors in our codebase for anonymous functions. I hope they will extend this rule in upcoming release and will bring it to error level.
https://biomejs.dev/linter/rules/use-explicit-function-return-type
There was a problem hiding this comment.
this one is sad, hoping this gets resolved quickly
| @@ -198,8 +198,7 @@ export class HttpClient implements IHttpClient { | |||
| this.logger?.debug("Requesting fallback URL", {routeId, baseUrl: printableUrl, score: this.urlsScore[i]}); | |||
| } | |||
|
|
|||
| // eslint-disable-next-line @typescript-eslint/naming-convention | |||
There was a problem hiding this comment.
naming convention no longer covered?
There was a problem hiding this comment.
It is disabled for certain files to fix these later to have consistent naming convention across packages.
https://github.com/ChainSafe/lodestar/blob/nh%2Fbiomejs/biome.jsonc#L297-L298
There was a problem hiding this comment.
Not ideal that we disable this for the whole file if there are just 1-2 lines affected
There was a problem hiding this comment.
Ultimately all disabled overrides will be removed, one by one, and will be much better to review those changes.
| @@ -20,11 +19,8 @@ export type EmptyRequest = Record<never, never>; | |||
| export type EmptyResponseData = void; | |||
| export type EmptyMeta = void; | |||
|
|
|||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |||
There was a problem hiding this comment.
No, this rule is disabled for all files in packages/api/src/utils/**/*.ts, as there are a lot of occurrences to disable per line.
There was a problem hiding this comment.
we disabled it per line before, why should that change now?
There was a problem hiding this comment.
I didn't went through line by line manually for such rules, searched through and if there I found more occurrences of disabled rules in single file added those as override. Considering that all those overrides will be removed very soon.
| @@ -44,7 +42,7 @@ function aggregate_verify(input: {pubkeys: string[]; messages: string[]; signatu | |||
| pubkeys.map((pk) => PublicKey.fromHex(pk)), | |||
| Signature.fromHex(signature) | |||
| ); | |||
| } catch (e) { | |||
There was a problem hiding this comment.
it might be useful to have the error for debugging purposes, maybe we can rename _e to fix lint error
There was a problem hiding this comment.
If we need and use it, we can add it then.
There was a problem hiding this comment.
if you have a running process and attach a debugger you can't just change it on the fly
There was a problem hiding this comment.
I usually don't debug a process on the fly, if I had to debug a specific code and specific code flow, will work on it.
But if everyone else feels the same we can keep add unused variables for exceptions. Doing it in a separate PR will much ideal to track for future that we explicitly added. it. @wemeetagain
There was a problem hiding this comment.
I agree with @nflaig we should avoid code updates in this PR, please rename error with underscore on unused error variables.
| @@ -3,7 +3,6 @@ import {describe, expect, it} from "vitest"; | |||
| import {ForkName, SLOTS_PER_EPOCH} from "@lodestar/params"; | |||
| import {ssz} from "@lodestar/types"; | |||
| import {LodestarError} from "@lodestar/utils"; | |||
| // eslint-disable-next-line import/no-relative-packages | |||
There was a problem hiding this comment.
we need ot be careful with those imports as it would break production code if done there, the lint rule seems important
There was a problem hiding this comment.
I added an issue to bring this behavior back. #7137
| "linter": { | ||
| "rules": { | ||
| "suspicious": { | ||
| "noExplicitAny": "off" |
There was a problem hiding this comment.
can't this be applied on per line basis?
There was a problem hiding this comment.
Yes we can disable by single line, but for these files we are overriding there are a lot of places where any is used.
Unfortunately the only way to disable by files is these overrides, we can't do it with code comments.
There was a problem hiding this comment.
I see a lot of files in the diff where it's just a single line in a file where the eslint ignore is removed but no new ignore added, see #7108 (comment)
There was a problem hiding this comment.
Those files would have been added in this override list. As few files were using single line comments, but had a lot of occurrences of same disable rules.
There was a problem hiding this comment.
There seems to be quite a few short comings but seems like biomejs is gonna address them sooner or later.
I generally go by "if it ain't broke, don't fix it" and for me eslint isn't really broken but I can see why the speed up increase is a nice-to-have.
Don't feel strongly about it though if others in the team are fine with the short comings and like the extra performance as a trade-off.
| } | ||
| throw e; | ||
| }), | ||
| chain |
There was a problem hiding this comment.
why did this formatting get updated?
There was a problem hiding this comment.
There is a subtle difference between Prettier and Biome formatter. specially calling function chaining with anonymous functions. These formatting changes are because of it.
| @@ -44,6 +44,7 @@ function getPeerState(status: StreamStatus): routes.node.PeerState { | |||
| case "closing": | |||
| return "disconnecting"; | |||
| case "closed": | |||
| return "disconnected"; | |||
There was a problem hiding this comment.
Such switch -> cases are considered useless. One may left those mistakenly without providing their body, so either to have a case body or remove such cases.
| @@ -396,7 +396,9 @@ export class NetworkProcessor { | |||
| if (item) { | |||
| this.gossipTopicConcurrency[topic] += numMessages; | |||
| this.processPendingGossipsubMessage(item) | |||
| .finally(() => (this.gossipTopicConcurrency[topic] -= numMessages)) | |||
| .finally(() => { | |||
There was a problem hiding this comment.
As described above.
| @@ -44,7 +42,7 @@ function aggregate_verify(input: {pubkeys: string[]; messages: string[]; signatu | |||
| pubkeys.map((pk) => PublicKey.fromHex(pk)), | |||
| Signature.fromHex(signature) | |||
| ); | |||
| } catch (e) { | |||
There was a problem hiding this comment.
I agree with @nflaig we should avoid code updates in this PR, please rename error with underscore on unused error variables.
| constructor( | ||
| readonly currentSlot: Slot, | ||
| public genesisTime = 0 | ||
| genesisTime = 0 |
There was a problem hiding this comment.
To enforce consistent accessibility modifiers everywhere.
We can either enforce explicitly specifying for every attribute or skip it for public. In our codebase most of the scenario were already following later case, so use noPublic option for that rule.
https://biomejs.dev/linter/rules/use-consistent-member-accessibility/
| }, | ||
| "globals": ["BigInt"] | ||
| }, | ||
| "overrides": [ |
There was a problem hiding this comment.
I tend to think we should not include any override here that is "temporary".
Things like noConsoleLog: off in cli is fine, but noExplicitAny shouldn't be here.
Instead we should maintain per-line overrides.
I would prefer this PR to NOT change how we manage overrides, rather we just want faster linting.
There was a problem hiding this comment.
Earlier we used disabling some rules on file level with code comments on top of the file.
In Biome we can't disable rules at file level with code comments, only way is to add such files in the overrides. Which I believe is better place as it gives more visibility to developers. The code comments just hide from sight once added.
| @@ -289,7 +288,6 @@ export function getTypeByEvent(config: ChainForkConfig): {[K in EventType]: Type | |||
| }; | |||
| } | |||
|
|
|||
| // eslint-disable-next-line @typescript-eslint/explicit-function-return-type | |||
There was a problem hiding this comment.
this one is sad, hoping this gets resolved quickly
|
@wemeetagain Created a PR myself for Biome to have that feature soon. biomejs/biome#4233 |
|
@wemeetagain I had been investigating and found some weird behavior from eslint. e.g. We have that rule to enforce return type for functions. Line 35 in 068fbae 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 |
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 |
I am not sure this rule is even evaluated for those, have you tried if you set |
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. |
* Replace eslint with biomejs * Update the inclusion of word * Fix the formatting * Update the lint task to do all checks * Update biome rules from eslint config * Replace eslint with biomejs * Update the inclusion of word * Fix the formatting * Update the lint task to do all checks * Update biome rules from eslint config * Fix all lint issues * Fix formatting * Add extension recomendation for vscode * Enable recommended rules * Enable rule noUselessSwitchCase * Enable rule noUselessConstructor * Fix the types * Fix unit tests * Enforce import extensions * Update the cli command * Enforce useConsistentMemberAccessibility * Update rules * Fix rules * Upgrade biomejs to latest version * Update the rules * Update and format the config file * Fix types break during merge * Fix unused check * Add comment for explicit-return-type * Remove eslint file * Add _e objects for empty catch blocks * Update formatter config * Fix formatting
|
🎉 This PR is included in v1.23.0 🎉 |
Motivation
Improve the performance for development environment. Where no other tool compete linting for biomejs, whole project less than 1s vs 43s.
Description
These are the reasons to consider Biomejs for better option.
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