-
Notifications
You must be signed in to change notification settings - Fork 50
ci: moving from eslint to biome #1283
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
Conversation
| import { exec } from 'node:child_process'; | ||
|
|
||
| import { describe, it, expect } from 'vitest'; | ||
| import { describe, expect, it } from 'vitest'; |
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.
Import sorting like this is an out of the box thing.
| describe('existing changelogs', () => { | ||
| let simpleDoc; | ||
| let anotherDoc; | ||
| let simpleDoc: PageObject; |
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.
It flagged that these didn't have an explicit type. I don't know where to put page.types.js but I didn't want to copy-paste this PageObject interface in a handful of files.
|
|
||
| declare module 'vitest' { | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| // biome-ignore lint/suspicious/noExplicitAny: This is the type for a custom matcher. |
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 love that it requires you give a reason why you're ignoring a rule.
| expect(yamlOutput).toMatchSnapshot(); | ||
| expect(fs.writeFileSync).toHaveBeenCalledWith(getGHAFileName(fileName), expect.any(String)); | ||
| expect(console.info).toHaveBeenCalledTimes(1); | ||
| expect(consoleInfoSpy).toHaveBeenCalledTimes(1); |
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.
Moved this over to the spy because it was already available and used in other tests, and also Biome was complaining about console.info being used and we had suppressed that at the top of file with ESLint.
biome.jsonc
Outdated
| // test result artifacts | ||
| "!coverage/**", | ||
|
|
||
| // release build artifacts | ||
| "!dist/**", | ||
| "!dist-gha/**", |
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 thought with vcs.useIgnoreFile: true we could get rid of these and have it automatically exclude files in our .gitignore file but it didn't work.
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.
this feels like a pretty rough oversight and doesn't inspire confidence tbh 😔 but glad the workaround is easy enough at least. mind adding a comment explaining this?
|
|
||
| validationHookResults.successes.forEach(success => { | ||
| if (success.result && success.result.pages.length) { | ||
| if (success.result?.pages.length) { |
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.
It recommended, but thankfully didn't autofix, that we move this to optional chaining.
| // biome-ignore lint/performance/noAccumulatingSpread: @todo can this be improved? | ||
| .reduce((prev, next) => Object.assign(prev, next)); |
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.
We might be able to work around this rule but it's probably fine?
| .map((p: string) => Object.keys(parsedPreparedSpec.paths?.[p] || {})) | ||
| .flat() |
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.
Blown away that it detected and autofixed this to a .flatMap().
| function replaceRef(schemaName: string, propertyName: string, refSchemaName: string) { | ||
| const schema = schemas[schemaName]; | ||
| if ('properties' in schema) { | ||
| schema.properties![propertyName] = { $ref: `#/components/schemas/${refSchemaName}` }; |
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.
| @@ -0,0 +1,3 @@ | |||
| { | |||
| "recommendations": ["biomejs.biome"] | |||
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.
So far this extension seems to work as well as the ESLint one.
| @@ -1,8 +1,8 @@ | |||
| import type { Mappings } from './readmeAPIFetch.js'; | |||
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.
Always bugged me that our ESLint import grouping config never grouped type imports like non-type imports. Really neat that Biome just does it all.
| if (options.Command?.flags?.key) { | ||
| this.debug('current command has --key flag'); | ||
| if (isTest()) { | ||
| // eslint-disable-next-line no-param-reassign |
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.
Not having a no-param-reassign rule might be an issue but with TS we've grown to be pretty good at not writing code that gets flagged with it.
|
|
||
| return { pathInput: newWorkingDir, zipped: true as const, unzippedDir: newWorkingDir }; | ||
| } catch (e) { | ||
| } catch (_e) { |
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.
Biome flags when we don't use variables but since we can't do } catch () { it says to prefix an unused variable with an underscore.
| import type { PageRequestSchema, PageResponseSchema } from './lib/types/index.js'; | ||
|
|
||
| export type { | ||
| APIKeyRepresentation, |
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 full :sickos: mode over here at these imports being autofixed to be properly alphabetized now.
| prefix: chore(deps) | ||
| prefix-development: chore(deps-dev) | ||
| ignore: | ||
| # Blocked on this until we can bump our shared ESLint config to support ESLint 9 |
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.
🥹
| // set the correct targets for GitHub Actions | ||
| current.oclif.commands.target = newTarget; | ||
| Object.values(current.oclif.hooks).forEach(hook => { | ||
| // eslint-disable-next-line no-param-reassign |
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.
it looks like this rule exists and is set to a warning by default: https://biomejs.dev/linter/rules/no-parameter-assign/
any idea why it's not being fired?
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.
Looks like it's not a part of the recommended ruleset.
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.
Looks like it's part of the project domain but enabling linter.domains.project: all doesn't seem to enable it. Not sure why but I've manually turned it on and addressed a couple issues.
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.
very glad that this file that we've spent so long in over the last few months has like zero issues lol 😮💨
kanadgupta
left a comment
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 slightly bummed about some of the rules we're losing (the eslint ecosystem is very difficult to top in this regard) but i think the tradeoffs seem worth it — eslint has been giving us grief for ages and biome's speed + out-of-the-box capabilities (the fact that it plays much better with TS 🫰🏽) + simpler configuration are all very promising.
this is a very big workflow change and i think we should make sure the team is fully on board before we move forward with other repos (and let's also make sure the custom rules support is adequate for our needs) but i'm in favor of this! let's also think about a shared config file that we publish from our standards repo at some point soon.
thanks for kicking off this effort ❤️🔥
|
|
||
| import { getNodeVersion, getMajorPkgVersion } from '../dist/lib/getPkg.js'; | ||
| // biome-ignore lint/correctness/useImportExtensions: This file exists but Biome wants to use the `.d.ts` file instead. | ||
| import { getMajorPkgVersion, getNodeVersion } from '../dist/lib/getPkg.js'; |
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.
weird — you'd think biome would be smart enough to know this is a .js file 🧐
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.
yea it might be that it prefers TS over JS and sees d.ts there. will see if i can open up an issue with them
| "enabled": true, | ||
| "domains": { | ||
| "project": "all", | ||
| "test": "all", |
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.
do we need this? it looks like they auto apply it if it detects vitest in our deps: https://biomejs.dev/linter/domains/#test-dependencies
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.
ugh nvm i just played around locally and looks like we need this 😔
| // biome-ignore lint/style/noParameterAssign: This is safe to rewrite our incoming questions for `prompts`. | ||
| questions = questions.map(question => ({ onRender, ...question })); | ||
| } else { | ||
| // eslint-disable-next-line no-param-reassign |
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 can't tell if this is biome being incomplete in its analysis or if it's actually a galaxy brain approach by not flagging this lol
| */ | ||
| function sanitizeHeaders(headers: Headers) { | ||
| const raw = Array.from(headers.entries()).reduce<Record<string, string>>((prev, current) => { | ||
| // eslint-disable-next-line no-param-reassign |
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 don't understand when it does and doesn't flag this rule 😮💨
| }, | ||
| "editor.defaultFormatter": "esbenp.prettier-vscode", | ||
| // explicitly disable ESLint to avoid conflicts with Biome | ||
| "eslint.enable": false, |
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 added the changes in this file b/c i imagine there might be weird side effects when vscode is running eslint + biome simultaneously and i figured it's safer to be explicit here. we should probably do the same with prettier (i.e., explicitly disable the prettier extension and set the default formatter to biome) if/when we migrate off of that too.
biome.jsonc
Outdated
| // test result artifacts | ||
| "!coverage/**", | ||
|
|
||
| // release build artifacts | ||
| "!dist/**", | ||
| "!dist-gha/**", |
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.
this feels like a pretty rough oversight and doesn't inspire confidence tbh 😔 but glad the workaround is easy enough at least. mind adding a comment explaining this?
|
🎉 This PR is included in version 10.5.0-next.1 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 10.5.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |

🧰 Changes
I've been staring down the barrel of upgrading our shared ESLint config to support ESLint 9 over in readmeio/standards#993 and... I honestly would rather live my life instead. I've been really interested in https://biomejs.dev/ as of late and after playing around with it for not even 10 minutes I am sold.
They're still working on supporting Markdown and YAML so we can't fully ditch Prettier too but as it is Biome has got the juice.
Footnotes
https://github.com/readmeio/standards/blob/bc29338858c55da37631addb36582ec5feac11b0/packages/eslint-config/index.js#L25-L36 ↩