-
Notifications
You must be signed in to change notification settings - Fork 419
feat: add missing SocketMode options #2735
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
base: main
Are you sure you want to change the base?
Conversation
|
|
||
| /** App initialization options */ | ||
| export interface AppOptions { | ||
| signingSecret?: HTTPReceiverOptions['signingSecret']; |
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.
These all still exist, just made them alphabetical order bc it was driving me nuts 🫠 (but a sanity check is very welcome!).
| private attachFunctionToken: boolean; | ||
|
|
||
| public constructor({ | ||
| signingSecret = undefined, |
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.
Same here. All still exist, just alphabetized.
| clientId?: string; | ||
| clientPingTimeout?: number; | ||
| clientSecret?: string; | ||
| stateSecret?: InstallProviderOptions['stateSecret']; // required when using default stateStore |
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.
And again 😉 (alphabetized)
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2735 +/- ##
==========================================
+ Coverage 93.44% 93.46% +0.01%
==========================================
Files 37 37
Lines 7675 7693 +18
Branches 669 669
==========================================
+ Hits 7172 7190 +18
Misses 498 498
Partials 5 5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
mwbrooks
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.
🙇🏻 Nice, thanks for exposing these options through Bolt!
❓ I'll start to test the PR soon, but I was curious whether there's a reason why we don't organize the socket mode options into a separate object (there is probably a reason)? Something like:
/** Initialization */
const app = new App({
token: process.env.SLACK_BOT_TOKEN,
socketMode: true,
appToken: process.env.SLACK_APP_TOKEN,
logLevel: LogLevel.DEBUG,
// New SocketModeOptions
socketModeOptions: {
autoReconnectEnabled: true,
clientPingTimeout: 42,
pingPongLoggingEnabled: true,
serverPingTimeout: 15,
},
});|
Nice 💯 |
| receiver?: Receiver; | ||
| redirectUri?: HTTPReceiverOptions['redirectUri']; | ||
| scopes?: HTTPReceiverOptions['scopes']; | ||
| socketModeOptions?: Omit<SocketModeOptions, 'appToken'>; |
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 @WilliamBergamin Bundled!
The caveat here is that SocketModeOptions (from the socket-mode pkg) requires appToken; we derive that from the root level initialization here. I've leveraged Omit to appease the type expectation, but open to alternatives (though it seems preferable at first blush to avoid the creation of a near-identical type).
| installerOptions: this.installerOptions, | ||
| installationStore, | ||
| redirectUri, | ||
| ...socketModeOptions, |
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.
❓ installerOptions is made public on the instance. I'm not sure why, exactly, but we can do the same for socketModeOptions if we think there's a use case for exposing it.
|
📚 Quick question if the new options might be nice to reference from this page? |
Summary
Adds missing
SocketModeOptionstoAppinit. Resolves #2292.New options are made available as seen below:
To test:
@slack/bolt-jsto this branch in projectautoReconnectEnabled,serverPingTimeoutMS,clientPingTimeoutMS,pingPongLoggingEnabled).Requirements (place an
xin each[ ])