-
Notifications
You must be signed in to change notification settings - Fork 394
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
passed clientOptions and authorizationUrl to @slack/oauth #586
Conversation
This needs slackapi/node-slack-sdk#1081 to be merged and released first |
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.
LGTM, single question for own understanding
@@ -104,6 +106,8 @@ export default class ExpressReceiver implements Receiver { | |||
installationStore, | |||
stateStore: installerOptions.stateStore, | |||
authVersion: installerOptions.authVersion!, |
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.
Why is installerOptions.authVersion
the only one to get the non-null assertion? ie, stateStore
also defaults
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.
lol the short answer is that the editor yelled at me and i wanted it to stop.
The real answer is that the type of installerOptions.authVersion
can only be 'v1' | 'v2'
and in this line, it could potentially be undefined
. Even if it is undefined
here, once it makes it to @slack/oauth
, it will get the default value of v2
assigned if that is the case.
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 good to me as long as tests pass after we merge slackapi/node-slack-sdk#1081
@@ -166,6 +166,8 @@ export default class App { | |||
|
|||
private axios: AxiosInstance; | |||
|
|||
private installerOptions: ExpressReceiverOptions['installerOptions']; |
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.
since installerOptions
is just passed through to the ExpressReceiver
, can we avoid storing it as a property in App
?
just picked up on the code smell of a property's type being defined in terms of another class's property.
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 reason why I store it as a property is because I have to add clientOptions
to installerOptions
. You can see that at https://github.com/slackapi/bolt-js/pull/586/files#diff-0286657a0789ea9446fa3de979ff3e09R226-R229.
Codecov Report
@@ Coverage Diff @@
## main #586 +/- ##
==========================================
+ Coverage 83.24% 83.27% +0.02%
==========================================
Files 7 7
Lines 597 598 +1
Branches 193 193
==========================================
+ Hits 497 498 +1
Misses 67 67
Partials 33 33
Continue to review full report at Codecov.
|
slackapi/bolt-js#586 > Exported interfaces from @slack/oauth and everything from @slack/types.
Summary
passed
clientOptions
andauthorizationUrl
to@slack/oauth
.Exported interfaces from
@slack/oauth
and everything from@slack/types
.Added
npm run watch
command that usesnodemon
to rebuild the source on every change.Issue: #585
Requirements (place an
x
in each[ ]
)