-
Notifications
You must be signed in to change notification settings - Fork 399
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
Fix #784 Add built-in AwsLambdaReceiver #785
Fix #784 Add built-in AwsLambdaReceiver #785
Conversation
Codecov Report
@@ Coverage Diff @@
## main #785 +/- ##
==========================================
+ Coverage 65.59% 66.04% +0.44%
==========================================
Files 12 13 +1
Lines 1093 1172 +79
Branches 323 342 +19
==========================================
+ Hits 717 774 +57
- Misses 317 333 +16
- Partials 59 65 +6
Continue to review full report at Codecov.
|
I can improve the code coverage. Any reviews / comments would be appreciated. |
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 nice!
I left a small nit to rename a type, but it's your choice on the naming convention.
🧠 I wonder curious, if it's possible to automatically apply the processBeforeResponse
setting in the AwsLambdaReceiver
? Or perhaps, we can set it to true
when it's undefined
? That way the developer can still disable it if they have a reason.
I am not really sure if there is a certain situation where a developer need to disable it for apps on FaaS. It does not work without exception (am I missing something?) The reason why this receiver is quite simple is that we do not have the flag. It's possible to have the flag but it will make the code unnecessarily complex. |
08104a1
to
8f00c79
Compare
After This morning, I realized that |
We may want to update https://github.com/slackapi/bolt-js/tree/main/examples/deploy-aws-lambda once we release this receiver. |
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 great Kaz! I'd say lets merge this in create an issue to write some documentation for it. We may want to hold off updating the official lambda deployment guide until we reach feature parity with ExpressReceiver
(like OAuth support).
Also, great tests
src/receivers/AwsLambdaReceiver.ts
Outdated
logLevel?: LogLevel; | ||
} | ||
|
||
export interface AwsLambdaReceiverInstallerOptions { |
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.
@seratch I don't see you using installPath
or redirectUriPath
. Did you add them because you wanted to implement OAuth but decided against it? For now, the recommendation is if you want OAuth, use ExpressReceiver
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.
Oh, thanks for pointing this out! Yes, I was going to implement OAuth flow too but, as I mentioned in the issue, @slack/oauth
module is not for this receiver at this point. I will remove this interface and add some comments why OAuth is not yet supported before merging this PR.
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.
Resolved this.
eb8e5a3
to
a383fee
Compare
Now this PR is ready to merge. |
5046859
to
f5a32d3
Compare
f5a32d3
to
733d28a
Compare
Thanks @seratch! I'll merge this PR now. I'll create a separate issue (unless we already have one) to add an example of AWSLambdaReceiver. We should not update our Regardless, it would be great to have examples of both methods - using the AwsLambdaReceiver and using the ExpressReceiver/Serverless express combo. |
Summary
This pull request fixes #784 - see the issue for details.
Requirements (place an
x
in each[ ]
)