Replace eslint with biome#439
Conversation
There was a problem hiding this comment.
I don't feel too strongly one way or the other for biome vs eslint; I'd slightly lean towards eslint due to familiarity, native JS and maturity, but it's not a show stopper.
Feedback:
- I ran biome locally and got some changes, which I'm not sure if we want or not given that it means if some other contributor runs
biome lint --writethen they will get quite a big diff which isn't committed and could cause noise in other PRs. - You've moved from spaces -> tabs, and single to double quotes. Both changes create significant diff noise, personally I'd keep the current formatting conventions where possible to reduce this noise - but not a deal breaker; we could also add a
.git-blame-ignore-revsfile that can hide these reformat changes from blames to keep disruption down due to formatting changes. - I'd reword some of the commits (a bit pedantic, sorry):
a)feature(dev): replace eslint with biome toolchain:chore(deps-dev): ...is typical for this kind of change
b)fix(core): biome format fix:stype: ..."fix" commits tend to be for fixing bugs, this is just internal code reorganisation so not worthy offixcommit IMO
c)fix: run biome formatterisdocs: add biome badge
d)fix run biome formatter fix:style:again, I'd probably fixup into the previous refactor commit
e)fix(tests): detect different line in emitted error after biome formatter fixes:test: ...
dhensby
left a comment
There was a problem hiding this comment.
- Should we split out the other dep bumps (type-is, mocha, sinon, vitepress) to their own commits or PR?
- Runnint
npm run lint:fixcreates a lot of changes (when I ran it locally anyway), which I don't think it should otherwise (as I said previously) other contributors will find themselves with a lot of noise when running this command.
Co-authored-by: Daniel Hensby <dhensby@users.noreply.github.com>
|
Thank you @dhensby I mostly did this because we are stuck at eslint 8.5 and can't move any further since eslint 9 requires ESM (see #279) and it is not the eslint we used to know because they splitted a lot of the rules from the core and it feels like a hot mess. For the spaces and single quotes I am sorry to have missed that, my goal was to configure biome to be on par with our current code style as much as possible. I will change them to spaces and single quotes ofc. |
…o feature/replace-eslint-with-biome
|
@dhensby I disabled some more rules to avoid lint fix creating noise. We can gradually enable / configure these rules in future PRs to get a coherent formatting that complies with many modern codebases out there. Regarding the commit messages I would opt for squash merge to avoid polluting the git history. |
dhensby
left a comment
There was a problem hiding this comment.
The arrowParentheses rule is also adding parentheses in places it wasn't. I'm actually a fan of them, so not so bothered, but depends if we want to try and keep consistency with what we have.
Co-authored-by: Daniel Hensby <dhensby@users.noreply.github.com>
…o feature/replace-eslint-with-biome
dhensby
left a comment
There was a problem hiding this comment.
First off — thank you for all the iterations here. 🙌 The formatting config now sits really close to our existing style, and the diff is far more reviewable than I'd feared. We're genuinely almost there.
Three small bits and then I'm happy — I've left inline suggestions for two of them. The third is just outside the diff so I couldn't attach a one-click suggestion:
pretest still calls eslint. Since eslint is now removed, npm test fails at the pretest hook before any test runs. (CI doesn't surface it because the coverage job runs test:coverage directly, and pre* hooks only fire for the exact test script.) Linting already has its own CI step plus the lint/lint:fix scripts, so let's just drop it from pretest:
- "pretest": "eslint lib test index.js",Thanks again for sticking with this and getting the config dialled in — really appreciate the effort. Tweak these three and I'm a yes. 👍
| */ | ||
| class Model { // eslint-disable-line no-unused-vars | ||
| class Model { | ||
| // eslint-disable-line no-unused-vars |
There was a problem hiding this comment.
This eslint-disable-line no-unused-vars is dead now that eslint is gone — Biome ignores it, and the reformat actually moved it off the class Model { line so it wasn't suppressing anything anyway. Safe to just remove it:
| // eslint-disable-line no-unused-vars |
| "enabled": true, | ||
| "indentStyle": "tab" |
There was a problem hiding this comment.
Tiny config nit: the top-level formatter sets tab here, while javascript.formatter below sets space. Since we format with spaces, let's keep the space and drop the tab — the javascript.formatter setting already governs all the included .js files:
| "enabled": true, | |
| "indentStyle": "tab" | |
| "enabled": true |
Summary
Replaces eslint with biome with remains compatible with commonjs and is also able to handle (future) esm (see #279)
Please do not get scared of the change files. While the linter only produced warnings, the formatter produced lots of errors, which were automatically fixable (similar to how prettier works)
Linked issue(s)
#403 #279
Involved parts of the project
Added tests?
OAuth2 standard
Reproduction
clone and run one of the following: