-
Notifications
You must be signed in to change notification settings - Fork 676
chore(deps-dev): bump @biomejs/biome to v2 for all packages #2281
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
Bumps [@biomejs/biome](https://github.com/biomejs/biome/tree/HEAD/packages/@biomejs/biome) from 1.9.4 to 2.0.5. - [Release notes](https://github.com/biomejs/biome/releases) - [Changelog](https://github.com/biomejs/biome/blob/main/packages/@biomejs/biome/CHANGELOG.md) - [Commits](https://github.com/biomejs/biome/commits/@biomejs/[email protected]/packages/@biomejs/biome) --- updated-dependencies: - dependency-name: "@biomejs/biome" dependency-version: 2.0.5 dependency-type: direct:development update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]>
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #2281 +/- ##
==========================================
- Coverage 92.71% 92.71% -0.01%
==========================================
Files 38 38
Lines 10609 10607 -2
Branches 683 683
==========================================
- Hits 9836 9834 -2
Misses 761 761
Partials 12 12
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
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.
Some comments to help the kind reviewers understand why these changes were made 🙇🏻
@@ -1,9 +1,18 @@ | |||
{ | |||
"$schema": "https://biomejs.dev/schemas/1.8.3/schema.json", | |||
"$schema": "https://biomejs.dev/schemas/2.0.5/schema.json", |
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.
note: Upgraded to schema v2 by following the v3 Upgrade Gudie and running:
$ npx @biomejs/biome migrate --write
"style": { | ||
"noParameterAssign": "error", | ||
"useAsConstAssertion": "error", | ||
"useDefaultParameterLast": "error", | ||
"useEnumInitializers": "error", | ||
"useSelfClosingElements": "error", | ||
"useSingleVarDeclarator": "error", | ||
"noUnusedTemplateLiteral": "error", | ||
"useNumberNamespace": "error", | ||
"noInferrableTypes": "error", | ||
"noUselessElse": "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.
note: These are the defaults that the migration command added. I believe they are meant to represent the default, recommended style rules.
@@ -1,4 +1,4 @@ | |||
{ | |||
"$schema": "https://biomejs.dev/schemas/1.8.3/schema.json", | |||
"$schema": "https://biomejs.dev/schemas/2.0.5/schema.json", |
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.
note: Each package extends the base biome.json
. This is why we need to have 1 PR that upgrades all packages to Biome v2 - because the schema is not compatible with Biome 1.x.
@@ -51,7 +55,7 @@ | |||
"semver": "^7.5.4" | |||
}, | |||
"devDependencies": { | |||
"@biomejs/biome": "^1.8.3", | |||
"@biomejs/biome": "^2.0.5", |
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.
note: Upgrading the biome npm package to v2.
"keywords": [ | ||
"cli", | ||
"hooks", | ||
"slack" | ||
], |
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.
note: biome linter rule requires multiple element arrays to be on newlines.
} from './RTMClient'; | ||
|
||
export { Logger, LogLevel } from './logger'; | ||
// Utilities |
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.
note: Added a // Utilities
chunk to keep the RTMClient
export at the top of the file.
|
||
// const { WebClient } = require('@slack/web-api'); | ||
// const webClient = new WebClient(process.env.SLACK_BOT_TOKEN, { | ||
// logLevel, | ||
// }); |
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.
note: Biome didn't like the unused variable, but it's important for the example. I've commented it out, similar to how we've commented out the serverPingTimeout
above. The alternative is an _webClient
prefix, but that could be misunderstood as a recommended naming pattern.
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.
@mwbrooks Thanks for catching this with comments! I think it's useful to keep with these files too, for fast reference 🙏 ✨
At a glance I didn't realize this was an unused variable also, so I am glad for this linter setting 👾
{ | ||
"includes": ["src/index.ts"], | ||
"assist": { | ||
"actions": { | ||
"source": { | ||
"organizeImports": "off" | ||
} | ||
} | ||
} |
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.
src/index.ts
requires the imports to be a specific, logical order but Biome requires the imports to be alphabetical. Right now, the Biome v2 Supressions (e.g. // biome-ignore lint/suspicious/noPrototypeBuiltins: reason why
) don't work for the assist
category. So, my workaround was to disable the organizeImports
for that specific file. 🤷🏻
// biome-ignore lint/suspicious/noPrototypeBuiltins: TODO use hasOwn instead of hasOwnProperty | ||
if (!Object.prototype.hasOwnProperty.call(toComplete, compareString)) { |
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.
note: Biome wants .hasOwn
instead of .hasOwnProperty
. I didn't feel comfortable making this change here because it uses a .call(...)
and I wasn't sure about test coverage.
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.
@mwbrooks Thanks for saving this in a TODO
- it would be tough to catch in this PR if it caused regressions of past Node versions 😓
@@ -1,5 +1,6 @@ | |||
import assert from 'node:assert'; | |||
import { WebClient } from '../../../dist/index.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.
note: Biome requires a newline between imports and logic.
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.
|
||
// const { WebClient } = require('@slack/web-api'); | ||
// const webClient = new WebClient(process.env.SLACK_BOT_TOKEN, { | ||
// logLevel, | ||
// }); |
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.
@mwbrooks Thanks for catching this with comments! I think it's useful to keep with these files too, for fast reference 🙏 ✨
At a glance I didn't realize this was an unused variable also, so I am glad for this linter setting 👾
export * from './message-attachments'; | ||
export * from './message-metadata'; | ||
export * from './views'; |
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 *
export worries me sometimes but I believe we're safe against name collisions in the build step.
// biome-ignore lint/suspicious/noPrototypeBuiltins: TODO use hasOwn instead of hasOwnProperty | ||
if (!Object.prototype.hasOwnProperty.call(toComplete, compareString)) { |
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.
@mwbrooks Thanks for saving this in a TODO
- it would be tough to catch in this PR if it caused regressions of past Node versions 😓
@@ -1,4 +1,4 @@ | |||
import { ConsoleLogger, type LogLevel, type Logger } from '@slack/logger'; | |||
import { ConsoleLogger, type Logger, type LogLevel } from '@slack/logger'; |
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.
📣 Rambling but I thought capital letters appeared before lowercase! 😉
packages/oauth/src/index.ts
Outdated
export * from './state-stores'; | ||
|
||
// Utilities |
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.
Half joking, but "errors" and "logger" wouldn't be not descriptive 😉
@@ -38,7 +38,7 @@ export class StateStoreChaiTestRunner { | |||
it('should detect old state values', async () => { | |||
const { stateStore } = this; | |||
const installUrlOptions = { scopes: ['channels:read'] }; | |||
const fifteenMinutesLater = new Date(new Date().getTime() + 15 * 60 * 1000); | |||
const fifteenMinutesLater = new Date(Date.now() + 15 * 60 * 1000); |
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.
🧠 TIL this was the same thing! Thank you for making this change.
@@ -105,7 +106,7 @@ describe('IncomingWebhook', () => { | |||
try { | |||
await webhook.send({ channel: 'different' }); | |||
assert.fail('expected rejection'); | |||
} catch (error) { | |||
} catch (_err) { |
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.
📣 @mwbrooks err
and _err
are good choices!
Summary
This pull request upgrades
@biomejs/biome
from1.8.3
to2.0.5
using the Biome v2 Migration Guide.The following packages are upgraded:
@slack/socket-mode
@slack/cli-hooks
@slack/cli-test
@slack/logger
@slack/oauth
@slack/rtm-api
@slack/types
@slack/web-api
@slack/webhook
Open PRs that are related to this migration:
Reviewers
Biome v2 appears to have made changes to the
organizeImports
assist, which is used to sort imports and exports. Most of the changes in this PR are from new rules. Most of the time, alphabetically sorted makes sense. But there are a few situations where it's better to sort by logic (e.g. import the main client then it's types and callbacks).I'm not advocating for the changes in this PR. I simply migrated to Biome v2. If these changes don't jive with you, then I can explore disabling
organizeImports
to see if it feels better for us.Requirements (place an
x
in each[ ]
)