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

Bind this for async rejection handling #175

Merged
merged 1 commit into from
May 6, 2019
Merged

Conversation

sethlu
Copy link
Contributor

@sethlu sethlu commented May 1, 2019

Summary

This small patch intends to resolve an this == undefined issue from using onGlobalError for handling Promise rejection errors. The the App instance is lost by only passing the function. The following error can be observed:

[DEBUG]  WebClient:0 initialized
[DEBUG]  WebClient:0 apiCall() start
[DEBUG]  WebClient:0 will perform http request
⚡️ Bolt app is running!
[DEBUG]  WebClient:0 http response received
[DEBUG]  WebClient:0 apiCall() start
[DEBUG]  WebClient:0 will perform http request
[DEBUG]  WebClient:0 http response received
[DEBUG]   Conversation context not loaded: Conversation not found
[DEBUG]  WebClient:0 apiCall() start
[DEBUG]  WebClient:0 will perform http request
[DEBUG]  WebClient:0 http response received
(node:20128) UnhandledPromiseRejectionWarning: TypeError: Cannot read property 'errorHandler' of undefined
    at onGlobalError (/home/sethlu/gamescrafters-slack-bot/node_modules/@slack/bolt/dist/App.js:231:14)
    at processTicksAndRejections (internal/process/task_queues.js:89:5)
(node:20128) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 2)
(node:20128) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

And with the following code... Using a slash command in a DM or using the slash command /test without any text should raise an error.

const { App, LogLevel } = require('@slack/bolt');

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

app.command('/test', async ({ command, ack, respond }) => {
    // Acknowledge command request
    ack();

    say(`${command.text}`);
});

(async () => {
    // Start the app
    await app.start(process.env.PORT || 3000);

    console.log('⚡️ Bolt app is running!');
})();

A similar solution is consulted from:

https://github.com/slackapi/bolt/blob/d3428aee9715c131c1ef54aa695456e08a0ce8ed/src/App.ts#L175

Requirements

@CLAassistant
Copy link

CLAassistant commented May 1, 2019

CLA assistant check
All committers have signed the CLA.

@aoberoi
Copy link
Contributor

aoberoi commented May 1, 2019

@sethlu good find! i had actually encountered this problem myself and was doing some work to write a test for it. i think your fix with my tests would combine nicely 😄

if you'll allow me, i'll pull your branch into my own PR, review, and merge them together. does that sound good?

@sethlu
Copy link
Contributor Author

sethlu commented May 2, 2019

@aoberoi Yea 😸 feel free to merge this and make the tests more comprehensive 👍

@colestrode
Copy link

I found a similar issue with the say factory builder, PR incoming 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants