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
build: use biome as formatter #50672
base: main
Are you sure you want to change the base?
Conversation
3c46f50
to
6ec8286
Compare
Or as suggest 💡-> Deprecation of formatting rules blog post:
|
139c534
to
c3e0525
Compare
I don't see any discussion leading up to this choice. rome (and now biome) still seems to have a small userbase compared to prettier etc. Stylistic, which @bricss mentioned, will likely also see tremendous growth. Aside from benchmarks, I assume, what made you choose this particular tool? |
f589685
to
0b7f606
Compare
We currently do not use a formatter in Node. We have several incomplete formatting rules, and that's it. If you clone and run the formatter on the repository, you'll see that the recommended changes (currently there is 14,000 errors) are recommendations that shouldn't be here at the first place. For example,
To make things much more solid, here's screenshot of what Biome recommends. IMHO, I don't understand how Eslint indentation rule is implemented and how the previous code is allowed.
There is more to what Biome offers especially in the linter area where we could potentially tap in the future. But here's my take:
Some of the Biome folks can answer this in more depth... |
I'm also curious about what led to the choice of Biome. (I have never heard of it and am interested in what it brings to the table). Genuinely speaking, I'm curious about the maintenance part, popularity, overall usage, and performance. Tools like prettier are standard because they're known well. Prettier is opinionated so I wonder how our stance differs here. Now, I wonder how this conflicts with our remark-preset-lint-node (in other words, with our linting configuration for Markdown files?) I was never a fan of having ESLint taking care or formatting rules, and I'm happy that they're removing that. It's definitely a good move. Yet, I'm curious how the adoption of Biome would play on the core, and I'm hesitant to think that running the lint on the whole Node.js codebase with Biome would imply. I'm concerned about how this might make backporting, git-blame, and other git operations harder because the stylistic and formatting rules differ. Have we accounted for that? Anyhow, by no means I'm against this, I just have this feeling that we should be careful with whatever we adopt. Thanks for taking the initiative, Yagiz. |
For the choice I have a few ideas for evaluation:
|
Although, for 1 mentioned above, we can also roll our own version of https://github.com/llvm-mirror/clang/blob/master/tools/clang-format/git-clang-format to run it on top of any JS formatter. Just need to make sure that it can format thes file in a way for a wrapper like this to work. Or it would still be better if the formatter support something like this out of the box |
Thank you @anonrig, for considering Biome.
I am going to piggyback and share more information about Biome, so you can make an informative decision about what you think is best for the project:
Happy to answer any questions |
IMO, |
@bricss Can you elaborate on what do you mean by "poor selection of rules"? |
I would say that performance should probably be that last thing we consider for a formatter. We just should not format the entire code base in one go and should do it incrementally, git-clang-format style, to reduce conflicts in backports and to preserve git blame history. Normally PRs just don't involve that many JS files being changed - in the rare occasions that they do, they are probably importing third-party dependencies, for which the formatter should be turned off. |
Agreed here. Performance is a good thing, but what matters the most here (which I also agree) is having a consistent ruleset that supports as closely as the current styling. This is something I talked about yesterday with @anonrig as backporting, git history, git blame, and others could be strongly affected. Supporting also our "git-clang-format" style would be preferable. In the end, if this adoption results in huge diffs, that's far from what we need and want here. So the obvious question is, can Biome, or any other formatter support our stylistic rules? |
@kibertoad By comparison of those two rulesets Biome -> Lint Rules vs ESLint Stylistic -> Rules is should be clear ⚖️ |
@bricss you're comparing ESLint's formatting lint rules to Biome's style lint rules (such as |
@joyeecheung I believe Biome has an incremental option planned, where it would only format/lint the files/lines that were changed. Is that what you're looking for? |
Why don't we try this and then reevaluate if we actually need another tool? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm -1 on this as it stands -- it involves adding 90mb of pre-compiled arch-specific (and on that note only x64 and arm64 architectures are supported) binaries to the repository. This will be problematic to downstream rebuilders (e.g. Linux distros) who prefer to build everything (including tooling) from source code.
I think not having proper formatting isn't really that big of a concern. Formatters should just be a helper for us when we are too lazy to e.g. indent code in a less messy way. But once formatting makes git blame harder to use e.g. making it harder to investigate the origin of a bug, it loses its value. git-clang-format style formatting works okay for our C++ because it helps with the "too lazy to put right amount of spaces in my macros" while not being too intrusive - before we started using git-clang-format I think we rarely did the "4 spaces when you indent an unfinished argument list" thing (even though we claimed to mostly follow the Google C++ style). With the incremental formatting however I rarely felt any intrusiveness because clang-format only corrects this indentation when you are already changing the argument lists, so it's just a drive-by that you might've done yourself anyway. |
It sounds similar but looks like it still works on a per-file bases. For this code base we'd need a tool that only touch changed lines (i.e. git-clang-format style) because we have many JS files containing thousands of lines still. |
About the build issue, I don't think we have to check the formatter into the code base, no matter which one we are choosing. We don't do that for clang-format either, it's only installed if you run And since I've been talking about clang-format as a reference, it actually also supports JavaScript. So that's also an option (we don't even need any extra build config, and can integrate it easily into our GitHub actions which already runs git-clang-format for C++). I am not sure how to pass the arguments properly in our Makefile though, but surely it's possible to run it on diffs in a huge code base (it's used by Chromium and V8 to format e.g. JS tests, and Node.js itself is tiny compared to those two). Another issue is that the pre-built clang-format binaries we use are no longer maintained, but that can probably be addressed with a GitHub repo that builds and releases binaries with a workflow. |
I think it is an erroneous affirmation. Yes Biome doesn't offer many stylistic rules, however most of the rules are carefully designed and some of them are actually less opinionated than the equivalent ESLint rule. We are more open to change a rule than ESLint is. We try to avoid false positives and to match community convention. Yes rules are not so customizable, however, you can still disable a rule. |
This isn't actually true (or it's inaccurate). We do use a formatter for C++ (clang-format) which can actually format JavaScript as well, and supports the incremental formatting that we need. We just never used it on JavaScript because we've been delegating that to the |
That seems like a workflow problem and not specific to any tool. |
I don't think this is true. Please refer to #50672 (comment) |
Why is it not true? It's just moving one style to another. In terms of dealing with "linters yelling at me about whitespace issues", they aren't too different from each other as long as the linter doesn't yell at you. Which style is better is up to personal preference but the goal of a formatter isn't really about stylistic preferences, as #50672 (comment) said, it's about
|
Because according to eslint indent rule that example should not have been possible. Current Node.js eslint configuration allows both of the following options, and how does it serve it's goal?
and
A quick Google search showed me this description of a formatter:
We currently do not have a consistent coding style. Edit: I want to emphasize "consistent" here. |
I would argue, that most of the modern IDEs already have had built-in |
FYI that Node should not be bound by IDE-specific linters. |
FYI: Biome has VSCode and IntelliJ extensions - https://biomejs.dev/guides/integrate-in-editor/ |
True story, but it's not the answer to how to get developers to use formatters or linters 👮 before they submit changes 📮 |
Sorry for the out-of-order-ness of these replies. I accidentally posted them under an account I didn't intend to. These are all me and no one else:
It's actually rather frustrating. Most of my commits in a recent PR were fixing stuff the linter in CI complained about because there wasn't a tool "fixing" the problem on save.
Agreed. I dislike some of the rules |
In my opinion it's a project configuration issue. It's entirely possible to ship the project in such a way that editors will recognize the formatting rules of the project and apply them. The way this project is setup it requires extra effort. |
@jsumners I don't disagree with you, but that isn't what this issue is about. |
Okay, I think they are quite closely related, but 🤷♂️ |
'default-case-last': 'error', | ||
'dot-location': ['error', 'property'], | ||
'dot-notation': 'error', | ||
'eol-last': 'error', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'eol-last': 'error', |
you forgot to remove this formatting rule biome already matches with
You can just consider this a bug in the formatter, but that doesn't make it less of a formatter. Otherwise as long as a formatter has a bug, it is...no longer a formatter? If ESlint cannot catch the indent rule being violated, does that make it...not a linter? At the end of the day, I am happy with a formatter that can help me keep the linter quite, buggy or not. For example the screenshot in #50672 (comment) does not look that significant to me - if I find this in a code review, I would not bother to ask folks to "fix the indentation" as long as the linter is happy with it. And if that formatting gets in the way of backporting or git blame, then I'd rather not format the code at all and let it be. And about the consistent part, I think that will just be what happens with a code base this size. To really have consistent style, you need to run a formatter in the entire code base, which introduce friction in backporting and git blames. As long as we format our code incrementally, some old code would always be in some inconsistent style until they get touched, if they ever gets touched. I think a cleaner git history is definitely worth giving up on the consistency of styles. |
It's possible to mitigate this by using git-blame-ignore-revs. It's a bit of a pain with the revs though on GitHub, especially with "squash and merge", because you get the final commit after the merge, so you end up doing two PRs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The update-biome.sh
script would permanently add almost 100 MiB to the repository for every single update. That seems like something we absolutely should not do.
(Non-blocking part.) I think #50714 is a good short term solution to fix #50449. @targos is right in that this PR is orthogonal, and with @joyeecheung's suggestion, the git diff should be minimal.
Incremental formatting is tricky. It works reasonably well with |
I think that still isn't quite enough for this code base. For context we have LTS lines (we even have two, at least for now) that need constant backporting from the main branch. A massive whitespace change would make backporting much harder because we already don't have the hands to backport patches soon enough and in correct order to older releases. Unless we have enough hands to keep all the LTS branches up-to-date regarding pending backports (which I guess we don't), the "before formatting" and "after formatting" conflicts could get out of hand quickly. If we format the LTS lines, then all the pending-to-backport patches that landed before formatting happened on the main branch would be a nightmare (remember that we don't have the hands to backport them in the correct order). If we don't format the LTS lines, then all the patches landed after formatting would be a nightmare. It would be easy to miss minute functional changes among all these whitespace changes when resolving the conflicts. Even when we are looking into an alternative release schedule, the alternative would still have at least one LTS release that needs some form of backporting nonetheless (maybe less frequent but it's still necessary for LTS), and before the alternative release schedule comes into effect, we still have two existing LTS lines to maintain. To me if we want to switch to another JS formatter, we should switch to one that supports git-clang-format style incremental formatting (or something like #50714 that does not actually update the formatting, just the disable comments). If they don't support this, we should not consider them until they support it. If we want to go with the least effort route, we can just write a python wrapper for git-clang-format to work with JavaScript which it already supports and is used by Chromium/V8. |
Removing |
Since ESLint is deprecating formatter rules, I propose using Biome as the formatter.
This is pretty much in progress. I'm not sure if Biome supports all existing rules.
Fixes #50449
Todos
Feature parity
eol-last ✅
linebreak-style ❌ (planned but not yet implemented, however git can automatically convert these for you in the .gitattributes file, like this)
no-extra-parens ✅
biome acts like the
all
eslint option, node currently uses thefunctions
one which is a subset ofall
no-multi-spaces ✅
no-multiple-empty-lines ⚠ (biome only supports the max: 1 eslint option, node uses max: 2)
no-tabs ✅ (set the biome formatter setting indentStyle to "space", doesnt work within comments)
no-trailing-spaces ✅
no-whitespace-before-property ✅
object-curly-newline ⚠ (click to see explanation)
biome will keep the braces in the same line if the object is empty
if you are destructuring, it will try to keep it in the same line unless that would exceed the defined
lineWidth
if the object is not empty, it will keep the linebreak (or lack of linebreak) the user provided in most cases
object-curly-spacing ✅
one-var-declaration-per-line ✅
operator-linebreak ✅ (tries to keep on the same line unless it exceeds lineWidth, if not it matches the "after" option which node uses)
padding-line-between-statements ❌ (click to see explanation)
node uses the
{ blankLine: 'always', prev: 'function', next: 'function' }
config for the option, which (i guess) makes eslint always add a blank newline if two functions are declarated in a row. biome doesn't add a blank newline unless you already had added onequote-props ⚠ (node uses the "consistent" eslint option, biome matches the "as-needed" option)
quotes' avoidEscape feature ✅
rest-spread-spacing ✅
space-before-blocks ✅
space-before-function-paren ⚠ (works almost the same as node's config except biome matches the "always" eslint option for anonymous functions)
space-in-parens ✅
space-infix-ops ✅
space-unary-ops ✅
spaced-comment ❌ planned but not yet supported