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 #860 Enable developers to customize the built-in receivers more #1183

Merged

Conversation

seratch
Copy link
Member

@seratch seratch commented Oct 29, 2021

Summary

This pull request adds a few customizable handlers to the default Receiver -- HTTPReceiver.

To: reviewers
I'm happy to elaborate more on details! but please refer to the comments in the diff first to learn the reason why these handlers can be useful.

Notes on other built-in receivers:

  • SocketModeReceiver: This is a quite different one -- most of the changes are not applicable to this receiver
  • ExpressReceiver: We can add some of these changes to this receiver but I personally would like to hold off adding the changes until we receive requests for the feature parity

Fixes #860

Requirements (place an x in each [ ])

@seratch seratch added the enhancement M-T: A feature request for new functionality label Oct 29, 2021
@seratch seratch added this to the 3.9.0 milestone Oct 29, 2021
@seratch seratch self-assigned this Oct 29, 2021
@seratch seratch force-pushed the issue-860-http-receiver-customization branch from d6028aa to a2da515 Compare October 29, 2021 04:07
@seratch
Copy link
Member Author

seratch commented Oct 29, 2021

I've set the milestone to v3.9 but if other maintainers are fine, I'm happy to move this to v3.8 (the next release)

@codecov
Copy link

codecov bot commented Oct 29, 2021

Codecov Report

Merging #1183 (9c2d8a7) into main (1b450f6) will increase coverage by 0.79%.
The diff coverage is 60.97%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1183      +/-   ##
==========================================
+ Coverage   71.42%   72.22%   +0.79%     
==========================================
  Files          17       17              
  Lines        1414     1433      +19     
  Branches      424      430       +6     
==========================================
+ Hits         1010     1035      +25     
+ Misses        332      322      -10     
- Partials       72       76       +4     
Impacted Files Coverage Δ
src/receivers/HTTPReceiver.ts 44.82% <60.97%> (+7.73%) ⬆️

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 1b450f6...9c2d8a7. Read the comment docs.

Copy link
Contributor

@filmaj filmaj left a 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!

What sorts of documentation updates should go with this? Looking at the docs on Handling Errors, there is only mention of app.error in there - would that be a good place to add this functionality to? Another option would possibly be the Customizing Receiver docs? What do you think?

src/receivers/HTTPReceiver.ts Show resolved Hide resolved
@seratch
Copy link
Member Author

seratch commented Oct 29, 2021

What sorts of documentation updates should go with this? Looking at the docs on Handling Errors, there is only mention of app.error in there - would that be a good place to add this functionality to? Another option would possibly be the Customizing Receiver docs? What do you think?

Yes, this is a really good point. We should update the error handling section. Also, we can update the reference page as well.

@filmaj
Copy link
Contributor

filmaj commented Oct 29, 2021

Yes, this is a really good point. We should update the error handling section. Also, we can update the reference page as well.

OK, sounds good. I am happy to write those up if you wish.

@seratch
Copy link
Member Author

seratch commented Oct 29, 2021

OK, sounds good. I am happy to write those up if you wish.

Nice! Let's start with creating a task issue for it 👍

@filmaj
Copy link
Contributor

filmaj commented Oct 29, 2021

Sounds good, created #1187 - if you'd like for me to work on it, please just assign me.

@seratch
Copy link
Member Author

seratch commented Oct 29, 2021

I've set the milestone to v3.9 but if other maintainers are fine, I'm happy to move this to v3.8 (the next release)

I'm thinking that we can have this in v3.8 next week. Before merging this, I wait for others' feedback / reviews by the end of next Modany, Pacific Time. Feel free to write in if you have any!

@seratch seratch modified the milestones: 3.9.0, 3.8.0 Oct 29, 2021
Copy link
Member

@stevengill stevengill left a comment

Choose a reason for hiding this comment

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

Looks great Kaz!!

@seratch seratch force-pushed the issue-860-http-receiver-customization branch from 42c4077 to 9c2d8a7 Compare October 30, 2021 01:06
@seratch seratch merged commit bd056fd into slackapi:main Nov 2, 2021
@seratch seratch deleted the issue-860-http-receiver-customization branch November 2, 2021 02:45
filmaj pushed a commit that referenced this pull request Apr 18, 2022
… handlers. Added these to the reference docs too. Related to #1183, fixes #1187.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement M-T: A feature request for new functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable developers to customize the built-in receivers more
3 participants