fix(App): Can't catch async errors from singleAuthorization.

[Awalgawe]

Summary

In singleAuthorization, errors from client.auth.test can't be catched.

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 #837 (18d079c) into main (78e605b) will increase coverage by 0.11%.
The diff coverage is 66.66%.

❗ Current head 18d079c differs from pull request most recent head bb5eac8. Consider uploading reports for the commit bb5eac8 to get more accurate results

@@            Coverage Diff             @@
##             main     #837      +/-   ##
==========================================
+ Coverage   66.05%   66.16%   +0.11%     
==========================================
  Files          13       13              
  Lines        1193     1200       +7     
  Branches      351      354       +3     
==========================================
+ Hits          788      794       +6     
- Misses        336      338       +2     
+ Partials       69       68       -1     

Impacted Files	Coverage Δ
src/App.ts	82.76% <66.66%> (-0.23%)	⬇️
src/receivers/HTTPReceiver.ts	17.75% <0.00%> (+0.59%)	⬆️

---

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 78e605b...bb5eac8. Read the comment docs.

[stevengill]

Hey @Awalgawe,

Thanks for sending the PR! Could you provide a reproduction usecase for the issue?

[Awalgawe]

Hi @stevengill It arrives when you use a bot token & accidently delete your bot ! ^^'

[Awalgawe]

we had something like:

try {
  new App({
    token: 'myToken',
    signingSecret: 'MySecret',
  });
} catch (error) {
  // nop 
}

[seratch]

I will check the behavior later but can you share error details illustrating the behavior changes and/or write some tests verifying if it works as expected?

[seratch]

The test cases you've added do not fail with the current implementation.

[seratch]

@Awalgawe Before discussing the details of your solution, your test code does not fail even with the current implementation. The added tests seem to be unrelated with the following situation you've mentioned:

try {
  new App({
    token: 'myToken',
    signingSecret: 'MySecret',
  });
} catch (error) {
  // nop 
}

Your test code is trying to verify some patterns with App#processEvent method. For the exceptions in the method execution, you can use app.error listeners. I'm thinking that the changes in this PR may not be necessary for your use case.

[Awalgawe]

@seratch Before my fix, the async error was throwed from the sync context of the App constructor: the previous version of the code create a promise in singleAuthorization but the await was declared in the App.authorize (which is the return value of the singleAuthorization in my case). The unhandled error come from that the fact that the promise can be rejected before being awaited. I made the promise creation lazy to handle this problem. With this fix, the promise is created (and the client.auth.test sent) only when the App.authorize is awaited. So, to test the client.auth.test failure, I need to test a function using App.authorize. This is why I add a test on App.processEvent to catch the App.authorize failure, which is the client.auth.test.

[seratch]

@Awalgawe Thanks for sharing more details!

At this moment, early failure of auth.test in App constructor (if the given token is not valid) is an intended behavior. Thus, if you are thinking to change the timing of the first auth.test API call to the first event processing, I call it a breaking change. We are unable to merge the change as we have been saying that App initialization verifies the token if you pass it.

Just for reference, We are planning to add a flag to customize the timing of first API call. #718. We have been receiving requests like "I don't want to have the auth.test calls in App constructor.". The new option is for the use cases. If it also works for you and you're interested in working on the ticket instead, I'm happy to work together on it.

[Awalgawe]

At this moment, early failure of auth.test in App constructor (if the given token is not valid) is an intended behavior.

Oh ok, I didn't expect that after reading the code.

I call it a breaking change

On this part, we have numerous ways to make it not breaking. I'll try something.

[Awalgawe]

@seratch any comments on new commits ?

[seratch]

@Awalgawe Oh, you've changed the direction of this pull request and you're trying to implement tokenVerificationEnabled option. That's great! However, as I didn't hear from you, I've implemented the feature at #863 a few days ago. If you have feedback on tokenVerificationEnabled feature, please feel free to leave comments at #863 . I appreciate your time and effort on this pull request.

[seratch]

As #863 can be a solution for your use case, let us close this pull request now. Thanks again for taking the time to make this pull request!