Skip to content
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 #1038 by making the AwsLambdaReceiver example TS compatible #1039

Merged
merged 1 commit into from
Aug 6, 2021

Conversation

seratch
Copy link
Member

@seratch seratch commented Aug 5, 2021

Summary

This pull request fixes #1038

Requirements (place an x in each [ ])

@seratch seratch added the bug M-T: confirmed bug report. Issues are confirmed when the reproduction steps are documented label Aug 5, 2021
@seratch seratch added this to the 3.6.0 milestone Aug 5, 2021
@seratch seratch requested a review from srajiang August 5, 2021 22:31
@codecov
Copy link

codecov bot commented Aug 5, 2021

Codecov Report

Merging #1039 (afebfcc) into main (08b50ca) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1039   +/-   ##
=======================================
  Coverage   68.89%   68.89%           
=======================================
  Files          13       13           
  Lines        1212     1212           
  Branches      357      357           
=======================================
  Hits          835      835           
  Misses        304      304           
  Partials       73       73           
Impacted Files Coverage Δ
src/App.ts 82.91% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 08b50ca...afebfcc. Read the comment docs.

Copy link
Member

@srajiang srajiang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah looks great and solves this user's issue very smoothly!

I do have a question re: when the user is instantiating their receiver separately then passing it as an option in the App constructor:

const awsLambdaReceiver = new AwsLambdaReceiver({
  signingSecret: process.env.SLACK_SIGNING_SECRET ?? ''
});

const app = new App({
  token: process.env.SLACK_BOT_TOKEN,
  processBeforeResponse: true,
  logLevel: LogLevel.DEBUG
});

In this case, would our error from #1038 still persist?

@seratch
Copy link
Member Author

seratch commented Aug 6, 2021

@srajiang

when the user is instantiating their receiver separately then passing it as an option in the App constructor:

This is the only way to use AwsLambdaReceiver so far.

In this case, would our error from #1038 still persist?

If you use app.start() method, yes. But our recommendation in TypeScript at this point is to use AwsLambdaReceiver#start() method instead.

Fixing App#start() method to be more robust may take a bit long. If we often see similar confusion around the AWS Lambda receiver, having a more specific method like AwsLambdaReceiver#toHandler() and encouraging developers not to use start() method may be an option.

Copy link
Member

@srajiang srajiang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@seratch
Alright, that makes sense! LGTM.

@srajiang srajiang merged commit f712a3b into slackapi:main Aug 6, 2021
@seratch seratch deleted the issue-1038-aws-lambda-example-bug branch August 6, 2021 22:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug M-T: confirmed bug report. Issues are confirmed when the reproduction steps are documented
Projects
None yet
Development

Successfully merging this pull request may close these issues.

App start() does not account for AWSLambdaReceiver return type
2 participants