[v2] Convert receivers to use App#processEvent

[barlock]

Summary

This receivers to use App#processEvent instead of being an event emitter as per #353

Depends on #369

NOTE I didn't include any documentation updates as #369 got giant because of it. They're in a separate pr #381

Requirements (place an x in each [ ])

• I've read and understood the Contributing Guidelines and have done my best effort to follow them.
• I've read and agree to the Code of Conduct.

[codecov]

Codecov Report

Merging #380 into @slack/bolt@next will increase coverage by 0.08%.
The diff coverage is 83.82%.

@@                 Coverage Diff                  @@
##           @slack/bolt@next     #380      +/-   ##
====================================================
+ Coverage             85.51%   85.60%   +0.08%     
====================================================
  Files                     7        7              
  Lines                   511      514       +3     
  Branches                146      149       +3     
====================================================
+ Hits                    437      440       +3     
- Misses                   49       50       +1     
+ Partials                 25       24       -1     

Impacted Files	Coverage Δ
src/ExpressReceiver.ts	71.00% <70.00%> (-1.39%)	⬇️
src/errors.ts	93.93% <91.66%> (-6.07%)	⬇️
src/App.ts	90.25% <100.00%> (+0.30%)	⬆️
src/middleware/builtin.ts	82.20% <100.00%> (+0.08%)	⬆️

---

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 1428ec3...b25ad27. Read the comment docs.

[seratch]

If I'm not wrong, this is the same to #375 (comment)

[barlock]

It is, the prs are based on each other. Rebasing on your merge and this should go away. 👍

[seratch]

Related to #388, I have some idea to change this function before exposing it to 3rd parties.

In my opinion, throwing exceptions in the case of verification failures is not the best way to handle them. Also, I don't think this function should return a Promise as it's just a simple synchronous function. So, my suggestions are:

1. If a receiver needs to know information about the reason for verification failures, we can return it as returned code values (say, Success, InsufficientInputs, InvalidTimestamp, Expiration, InvalidSignature)
2. Most 3rd parties don't need such detailed information, returning a boolean value may be enough like this code

I would like to go with 1. if you don't see any issues with it. Either of applying the changes to in this PR or coming up with another PR for the change after merging this is fine to me!

[barlock]

Also, I don't think this function should return a Promise

Nice catch, you're right that was left over from some old version of the code. I removed it.

In my opinion, throwing exceptions in the case of verification failures is not the best way to handle them.

Yes, this is interesting. A few opinions:

1. It is a strange thing to throw an error for. If I'm really thinking about the purity of this change, I probably shouldn't have updated it as it's not super related to processEvent.
2. Coded responses aren't a thing in Javascript land? Actual question, I haven't seen anyone else doing it and I expect it would be surprising to folks using it. I have seen coded errors which is why this function made some sense to me.
3. The only necessity for the detailed information is in a response code (400 vs 401. This is in response to slack apis, honestly that code will likely never be tracked? It's a nicety to the ops folks on your end, if they don't care doesn't really matter.

My preference in this is to let you handle it in #388. I think I would probably:

1. Leave it as is (and you change what you want via Extract some functionality to make writing custom App/Receiver easier #388)
2. Switch to a pattern like your 2 and always return 400 (not sure if folks on your end pay attention there)

[seratch]

If I'm really thinking about the purity of this change, I probably shouldn't have updated it as it's not super related to processEvent.

This is so true.

1. Leave it as is (and you change what you want via Extract some functionality to make writing custom App/Receiver easier #388)

I agree with this. We all can decide how to change in another PR / discussion. Thanks for the response here 👍

[barlock]

Note, this pr is huge, I feel kinda bad about it.

There are really 3 refactors in it.

1. processEvent per Proposal: Chaining middleware using Promises #353
2. Converting coded errors and their helpers to be subclasses so they work in JS land as well as TS land
3. Pulling out the signature verification (discussion [v2] Convert receivers to use App#processEvent #380 (comment))

I've run out of time in the immediate future to make significant changes to anything except 1. I'm happy to remove 2 and 3 for easier reviews.

Otherwise, I feel good about all 3 and coverage is nice 🤷‍♂

[seratch]

First of all, I'd greatly appreciate @barlock's massive efforts and quality works (for not only this PR but the discussion and a series of PRs). Thank you very much again.

As for this PR, I don't have any concerns if we have all the three. This PR looks excellent to me.

I would like to hear other Bolt maintainers' thoughts!

[barlock]

It's ready!

[barlock]

Branches updated and ready to go again!

[stevengill]

In regards to receiver.ts, if the ultimate plan is to make it easy for folks to use these functions as they build their own receivers, it is best to move this file into its own module that ships independently of bolt.

We shouldn't be exporting receiver.ts in index.ts as it will encourage receiver developers to use bolt as a dependency. We would rather they use the module on its own (once it is broken out). This could lead to some weird dependency issues where a receiver a different version of bolt as the app developer.

I've left some comments for changes in regards to this so it won't require a breaking change to bolt later on when we do ship this new module.

Shipping the new module shouldn't be a blocker for boltV2 either this way

[aoberoi]

so, this is a pretty huge review. @stevengill and i walked through the PR together and came up with these comments.

thanks for the continued effort over many weeks on this one. it's truly EPICCC!

[barlock]

Related to this pr, but not caused by it, I discovered errors are being swallowed in event listeners here: https://github.com/slackapi/bolt/pull/380/files#diff-0286657a0789ea9446fa3de979ff3e09R559

Not sure why yet. I don't think it should hold this PR up as it's a bug in the last one. I'll see if I can identify the fix and either slip it in with a test case or follow up in a later pr.

[barlock]

@stevengill When I removed those tests you asked me to it dropped the coverage for the commit which apparently breaks codecov even though the pr as a whole raises it?

Can we remove that requirement or can I export some stuff so I can test it? You're not going to release v2 until you break those packages apart anyway?

[stevengill]

@barlock we were planning on releasing v2 first and then breaking out those packages. If we expose an export to them, it means we need to continue to provide that export even after we break those packages out (or do another major version bump). We don't want to provide that export as people might start depending on it as they make their own receivers.

I'm personally fine with the drop.