Switch to biome from eslint #672

Lunarequest · 2024-07-23T10:59:53Z

This pr is to start the discussion about using tooling alternative to eslint. I'm proposing biome as it is a good balance of fast and it produces meaningful errors. This also includes a autoformatting pass by biome in a separate commit. Biome is also a formatter and will also eventually support css/scss which would also allow us to drop stylelint in the future

The biome config was automatically generated from the eslintrc by the biome tool.

this is a formating pass done by biome to remove errors related to the formatter

martinpitt · 2024-07-23T11:45:04Z

Thanks @Lunarequest ! Aside from introducing tabs (argh no) and introducing quite a lot of opinion on the source format (which I can live with as long as it still keeps the code readable -- i.e. not as opinionated as black, I'd like to discuss that first for a little bit:

How does biome's rule set compares to eslint, oxlint, etc? Aside from the formatting, how many rule violations do you get? Can you please do a local run and paste the output? I'm interested in how legible and sensible they are. For that reason I didn't want to try this on starter-kit, as that's too simple to have meaningful results. Of course after agreeing the actual change should be done to starter-kit.
biome isn't currently run anywhere. eslint is run through test/static-code imported from cockpit, so we need to teach it to optionally run multiple linters based on which config files are present.

@jelly @allisonkarlitskaya I know you have some opinions and prior research about oxlint and possibly other alternatives. Opinions, data points?

jelly

If we want this, this requires changes in test/static-code as well as that now still runs eslint. The configuration also seems to only handle Typescript files while we have plenty of JavaScript / JSX files which should also be linted.

Some things which make it a bit more complex, in files/podman and ostree we use eslint's import plugin to order imports. Supporting this would be very nice.

Testing this PR by just picking the config file up, I have the idea that the ignores don't work as linting takes 2 seconds for me. Maybe this is configured wrong? https://biomejs.dev/reference/configuration/#filesignore

Testing with biome 1.8.3 on Cockpit itself, doesn't feel amazing.

~/Downloads/biome-linux-x64 lint pkg  1.74s user 0.08s system 571% cpu 0.319 total

But that's Cockpit, on files:

Checked 653 files in 245ms. No fixes applied.
~/Downloads/biome-linux-x64 lint  0.86s user 0.14s system 340% cpu 0.294 total

So it seems node_modules is taken into account:

Just linting src:

~/Downloads/biome-linux-x64 lint src  0.23s user 0.05s system 303% cpu 0.090 total
Checked 20 files in 31ms. No fixes applied.

Against oxlint:

[jelle@t14s][~/projects/cockpit-files]%time oxlint --config .eslintrc.json
Finished in 33ms on 20 files with 94 rules using 8 threads.
Found 0 warnings and 0 errors.
oxlint --config .eslintrc.json  0.07s user 0.03s system 229% cpu 0.043 total

But in Cockpit it makes me feel that oxlint is way faster:

oxlint --config .eslintrc.json pkg
Finished in 90ms on 293 files with 94 rules using 8 threads.
Found 23 warnings and 19 errors.

~/Downloads/biome-linux-x64 lint  pkg
Diagnostics not shown: 1930.
Checked 245 files in 265ms. No fixes applied.

The amount of diagnostic messages of course don't help :)

Some things I like more of oxlint is their diagnostics:

  ⚠ oxc(const-comparisons): Right-hand side of `&&` operator has no effect.
     ╭─[pkg/networkmanager/ip-settings.jsx:153:20]
 152 │             return { ...config, netmask: "255.255.0.0" };
 153 │         } else if (split[0] <= 192 && split[0] <= 223) {
     ·                    ───────┬───────    ───────┬───────
     ·                           │                  ╰── If this evaluates to `true`
     ·                           ╰── This will always evaluate to true.
 154 │             return { ...config, netmask: "255.255.255.0" };
     ╰────
  help: if `split[0] <= 192` evaluates to true, `split[0] <= 223` will always evaluate to true as well

Output of biome is also interesting, but feels a bit of an information overload:

pkg/apps/packagekit.js:55:19 lint/complexity/useArrowFunction  FIXABLE  ━━━━━━━━━━━━━━━━━━━━━━━━━━━━

  ✖ This function expression can be turned into an arrow function.

    53 │ function resolve(method, filter, name, progress_cb) {
    54 │     return resolve_many(method, filter, [name], progress_cb)
  > 55 │             .then(function (ids) {
       │                   ^^^^^^^^^^^^^^^^
  > 56 │                 if (ids.length === 0)
  > 57 │                     return Promise.reject(new PK.TransactionError("not-found", "Can't resolve package"));
  > 58 │                 else
  > 59 │                     return ids[0];
  > 60 │             });
       │             ^
    61 │ }
    62 │

  ℹ Function expressions that don't use this can be turned into arrow functions.

  ℹ Safe fix: Use an arrow function instead.

     53  53 │   function resolve(method, filter, name, progress_cb) {
     54  54 │       return resolve_many(method, filter, [name], progress_cb)
     55     │ - ············.then(function·(ids)·{
         55 │ + ············.then((ids)·=>·{
     56  56 │                   if (ids.length === 0)
     57  57 │                       return Promise.reject(new PK.TransactionError("not-found", "Can't resolve package"));

So maybe we should compare oxlint vs. biome. Biome has a clear advantage in that it's a bit older, has a formatter (oxlint has one but it's not worked on). Biome seems to work on supporting CSS, as far as I know oxlint won't.

Writing rules for oxlint is farily easily doable for our team-members, for biome I have no idea. The rules seem to be very similiar to oxlint, https://github.com/biomejs/biome/blob/fe09cab96b5abccb03b73d46ce6c3bddec1c95d8/crates/biome_js_analyze/src/lint/style/use_template.rs#L67

oxlint tries to implement all kinds of eslint rules and plugins (except deprecated rules)
biome implements eslint and plugin rules but re-thinks them and names them different. Making compatibility a more difficult question

(Having seen some of the eslint rules, I think we should also cut down on the ones we have enabled, currently we sit on around ~ 250 rules and some of them are just wacky)

npx eslint --print-config .eslintrc.json | jq -r '.rules | keys | join("\n")'  | wc -l

Last but not least both should support being an LSP, I'd say this is a must these days :)

An alternative is looking at eslint 9, but I have no idea how fast that is or how much better then to eslint 8.

jelly · 2024-07-23T11:51:51Z

biome.jsonc

+				"useValidTypeof": "error"
+			}
+		},
+		"ignore": ["node_modules/*", "pkg/lib/*", "bots/*"]


pkg/lib contains shared component which should be linted I'd say but this was already in ignore. So leave it be, was more a surprise for me :)

jelly · 2024-07-23T11:53:44Z

.eslintrc.json

@@ -1,81 +0,0 @@
-{


So this was converted with biome migrate eslint --write I assume.

https://biomejs.dev/guides/migrate-eslint-prettier/

Yes, it was, however this also had the --include-inspired flag

jelly · 2024-07-23T12:04:27Z

biome.jsonc

+		"ignore": ["node_modules/*", "pkg/lib/*", "bots/*"]
+	},
+	"javascript": {
+		"globals": ["require", "module", "document", "navigator", "window"]


Does biome not support setting the env, we developer for a browser so the linter should know about it like ESLint does?

✖ Use Number.parseInt instead of the equivalent global. 30 │ method, 31 │ address: u.hostname, > 32 │ port: u.port ? parseInt(u.port) : 80, │ ^^^^^^^^ 33 │ path: u.pathname + u.hash, 34 │ };

This is maybe something we should fix...

subhoghoshX · 2024-07-23T14:51:27Z

Wow! I'm so happy to see this PR 😄. I've been moving some of my personal projects to Biome and making a comparison of existing eslint rules and biome's rules, I was about to make a comment on @jelly's oxlint issue.

Some things which make it a bit more complex, in files/podman and ostree we use eslint's import plugin to order imports.

Biome has three parts linter, formatter and analyzer. Linter and formatter is self-explanatory, the Analyzer essentially does import sorting.

and will also eventually support css/scss which would also allow us to drop stylelint in the future

Biome already supports CSS, though it's disabled by default and needs to be turned on:

{
  "css": {
    "formatter": {
      "enabled": true
    },
    "linter": {
      "enabled": true
    }
  }
}

This way the formatting works, but the lint rules are mostly in "Nursery" section, which means experimental, so needs to be enabled manually.

Last but not least both should support being an LSP, I'd say this is a must these days :)

Biome has an official language-server at nvim-lspconfig, they have a vscode plugin as well (though never used it). It works out of the box as long as biome.json file is present at project root.

The formatter works with vim.lsp.buf.format() call. My nvim config:

vim.api.nvim_create_autocmd('BufWritePre', {
  callback = function()
    vim.lsp.buf.format({
      filter = function(client)
        -- filtering because tsserver also has basic formatting capabilities
        -- which may get triggered with vim.ls.buf.format() method
        return client.name == 'biome'
      end
    })
  end
})

I prefer formatting in editor because, running biome format --write . on shell every time kinda defeats the purpose.

Biome formatting is much robust than ESLint style rules, because two people with same eslint config can format things drastically differently, not with biome. It's formatter is essentially Prettier but written in rust.

Lunarequest · 2024-07-23T16:47:28Z

as opinionated as black, I'd like to discuss that first for a little bit:

biome is super configurable almost everything from both the parser and formatter can be configured.

How does biome's rule set compares to eslint, oxlint, etc?

This page from the the upstream documentation should shed some light https://biomejs.dev/linter/rules-sources/. Almost all the rules either come from eslint or some eslint plugin

Lunarequest · 2024-07-23T16:49:50Z

this is a full run with lint, it has 26 errors.
log.txt

Lunarequest · 2024-07-23T17:06:15Z

Testing this PR by just picking the config file up, I have the idea that the ignores don't work as linting takes 2 seconds for me. Maybe this is configured wrong? https://biomejs.dev/reference/configuration/#filesignore

Testing with biome 1.8.3 on Cockpit itself, doesn't feel amazing.
~/Downloads/biome-linux-x64 lint pkg  1.74s user 0.08s system 571% cpu 0.319 total
But that's Cockpit, on files:
Checked 653 files in 245ms. No fixes applied.
~/Downloads/biome-linux-x64 lint  0.86s user 0.14s system 340% cpu 0.294 total
So it seems node_modules is taken into account:

Just linting src:
~/Downloads/biome-linux-x64 lint src  0.23s user 0.05s system 303% cpu 0.090 total
Checked 20 files in 31ms. No fixes applied.

yes, I noticed this. I believe that errors from these locations are ignored. but the files are still read. I'll look into if there is a more powerful way to make biome ignore files and not read those directories

allisonkarlitskaya · 2024-07-23T19:20:02Z

I'm officially neutral on this.

I'm no eslint fan, and particularly eslint 9 seems to have caused a lot of problems and caused the community to start asking themselves some big questions. I was wanting to play wait-and-see on that, but even if everything sorts itself out nicely there, I don't think eslint will ever be 'fast enough'.

On the other hand, I also feel like biome and oxlint aren't "there" yet...

My main requirements are good LSP support and a decent set of React rules: the sort of things that hint you when you mess up the dependencies to useEffect, for example.

Other than that, thanks for looking into this. As the rest of our test/static-code has gotten continuously faster over the past little while, eslint has been annoying me more and more...

subhoghoshX · 2024-07-24T08:06:19Z

biome.jsonc

@@ -0,0 +1,136 @@
+{
+	"$schema": "https://biomejs.dev/schemas/1.8.3/schema.json",


This is nice to have, but every time dependatbot updates the biome version I feel someone needs to come and manually update this.

From the docs we can also do:

{ "$schema": "./node_modules/@biomejs/biome/configuration_schema.json" }

yes, not sure if we want too

subhoghoshX · 2024-07-24T08:06:32Z

biome.jsonc

+				"useValidTypeof": "error"
+			}
+		},
+		"ignore": ["node_modules/*", "pkg/lib/*", "bots/*"]


We can also enable git integration:

{ "vcs": { "enabled": true, "clientKind": "git" } }

That way we don't need to do any "ignore" in linter or formatter.

node_modules are a submodule, wouldn't that be checked then?

we would any ways. bots has a bunch of broken symlinks that cause biome to spit out internal fs errors

node_modules are a submodule, wouldn't that be checked then?

I believe Biome ignores node_modules even without git integration. It's for the other files/folders in that are ignored in git.

we would any ways. bots has a bunch of broken symlinks that cause biome to spit out internal fs errors

Interesting. I think /bots is already in .gitignore.

subhoghoshX · 2024-07-24T08:06:40Z

biome.jsonc

+		"ignore": ["node_modules/*", "pkg/lib/*", "bots/*"]
+	},
+	"javascript": {
+		"globals": ["require", "module", "document", "navigator", "window"]


I think this is unnecessary.

this was imported from the eslint config

subhoghoshX · 2024-07-24T08:06:45Z

biome.jsonc

+	"javascript": {
+		"globals": ["require", "module", "document", "navigator", "window"]
+	},
+	"overrides": [{ "include": ["**/*.ts", "**/*.tsx"] }],


which files does it match?

this was also inherited from the eslint config

subhoghoshX · 2024-07-24T08:06:51Z

biome.jsonc

+		"globals": ["require", "module", "document", "navigator", "window"]
+	},
+	"overrides": [{ "include": ["**/*.ts", "**/*.tsx"] }],
+	"formatter": {


Please use spaces:

{ "formatter": { "indentStyle": "space" } }

And possibly add "indentWidth": 4 as well(?) I personally prefer 2 spaces, but cockpit has been traditionally using 4 spaces for some reason. I'm neutral on this.

subhoghoshX · 2024-07-24T08:06:57Z

biome.jsonc

+	},
+	"overrides": [{ "include": ["**/*.ts", "**/*.tsx"] }],
+	"formatter": {
+		"ignore": ["node_modules/*", "pkg/lib/*", "bots/*"]


We won't need this with the git integration.

we would see my comment on bots

subhoghoshX · 2024-07-24T09:04:57Z

I have a little doubt over how Biome formatter handles gettext. Like for a long text _("long text..."), it'll format it like below:

_(
"long text..."
)

which I doubt is not the same thing, based on what pitti explained to me once.

subhoghoshX · 2024-07-24T09:14:31Z

src/upload-button.tsx

-                    _("A file with the same name already exists in \"$0\". Replacing it will overwrite its content."),
-                    path.join('/')
+                    _(
+                        'A file with the same name already exists in "$0". Replacing it will overwrite its content.',


Ah found a concrete example of the gettext issue. I'm not sure about removing the \ from \"$0\" as well.

Yes, this is absolutely invalid. Translatable strings have to use double-quotes to get recognized by xgettext. test/static-code also enforces this, but either it's not running or (most probably) it doesn't see this due to the line break.

Lunarequest · 2024-07-24T09:15:36Z

I have a little doubt over how Biome formatter handles gettext. Like for a long text _("long text..."), it'll format it like below:
_(
"long text..."
)
which I doubt is not the same thing, based on what pitti explained to me once.

we might need to sprink a few ignores to prevent formatting if that turns out to be an issue. I'm not particularly familiar with gettext, so I am unsure if this will effect it. I know from a javascript perspective it is the same

martinpitt · 2024-07-24T09:15:45Z

@subhoghoshX It is the same thing as far as gettext runtime is concerned, but (1) xgettext may not recognize this with the line breaks (check generation of the .pot file), and (2) it looks absolutely horrible.

Lunarequest added 2 commits July 23, 2024 16:23

switch to biome from eslint

59e1874

The biome config was automatically generated from the eslintrc by the biome tool.

biome format

635a9b8

this is a formating pass done by biome to remove errors related to the formatter

jelly reviewed Jul 23, 2024

View reviewed changes

subhoghoshX reviewed Jul 24, 2024

View reviewed changes

Lunarequest added 2 commits July 24, 2024 14:25

update config

30e0361

run formatter

268b306

subhoghoshX reviewed Jul 24, 2024

View reviewed changes

skip line for xgetext

6958147

Lunarequest force-pushed the biome branch from ccfbd6a to 6958147 Compare July 24, 2024 11:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch to biome from eslint #672

Switch to biome from eslint #672

Lunarequest commented Jul 23, 2024

martinpitt commented Jul 23, 2024

jelly left a comment •

edited

Loading

jelly Jul 23, 2024 •

edited

Loading

jelly Jul 23, 2024

Lunarequest Jul 23, 2024

jelly Jul 23, 2024

jelly Jul 23, 2024

subhoghoshX commented Jul 23, 2024

Lunarequest commented Jul 23, 2024

Lunarequest commented Jul 23, 2024

Lunarequest commented Jul 23, 2024

allisonkarlitskaya commented Jul 23, 2024

subhoghoshX Jul 24, 2024

Lunarequest Jul 24, 2024

subhoghoshX Jul 24, 2024

jelly Jul 24, 2024

Lunarequest Jul 24, 2024

subhoghoshX Jul 24, 2024

subhoghoshX Jul 24, 2024

Lunarequest Jul 24, 2024

subhoghoshX Jul 24, 2024

Lunarequest Jul 24, 2024

subhoghoshX Jul 24, 2024

subhoghoshX Jul 24, 2024

Lunarequest Jul 24, 2024

subhoghoshX commented Jul 24, 2024

subhoghoshX Jul 24, 2024

martinpitt Jul 24, 2024

Lunarequest commented Jul 24, 2024

martinpitt commented Jul 24, 2024

		@@ -0,0 +1,136 @@
		{
		"$schema": "https://biomejs.dev/schemas/1.8.3/schema.json",

Switch to biome from eslint #672

Are you sure you want to change the base?

Switch to biome from eslint #672

Conversation

Lunarequest commented Jul 23, 2024

martinpitt commented Jul 23, 2024

jelly left a comment • edited Loading

Choose a reason for hiding this comment

jelly Jul 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

subhoghoshX commented Jul 23, 2024

Lunarequest commented Jul 23, 2024

Lunarequest commented Jul 23, 2024

Lunarequest commented Jul 23, 2024

allisonkarlitskaya commented Jul 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

subhoghoshX commented Jul 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lunarequest commented Jul 24, 2024

martinpitt commented Jul 24, 2024

jelly left a comment •

edited

Loading

jelly Jul 23, 2024 •

edited

Loading