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

Update SocketModeFunctions.ts #1553

Merged
merged 1 commit into from
Aug 16, 2022
Merged

Update SocketModeFunctions.ts #1553

merged 1 commit into from
Aug 16, 2022

Conversation

rileyeaton
Copy link
Contributor

Summary

This PR will fix an issue where the bolt app will crash due to disabling socket mode while running. When this is disabled, the app crashes due to an attempt to access a key of an undefined value, body.type. All that is needed to avoid this crash is to add optional chaining.

This is the error that is displayed (locally) when socket mode is disabled:

C:\Users\RileyEaton\Documents\gameon\node_modules\@slack\bolt\dist\receivers\SocketModeFunctions.js:17
        logger.error(`An unhandled error occurred while Bolt processed (type: ${event.body.type}, error: ${error})`);
                                                                                           ^

TypeError: Cannot read properties of undefined (reading 'type')
    at SocketModeReceiver.defaultProcessEventErrorHandler [as processEventErrorHandler] (C:\Users\RileyEaton\Documents\gameon\node_modules\@slack\bolt\dist\receivers\SocketModeFunctions.js:17:92)
    at SocketModeClient.<anonymous> (C:\Users\RileyEaton\Documents\gameon\node_modules\@slack\bolt\dist\receivers\SocketModeReceiver.js:129:50)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)

This is because the event object is:

{
  body: undefined,
  ack: [AsyncFunction: ack],
  retryNum: undefined,
  retryReason: undefined,
  customProperties: {}
}

It is important that this is fixed because it cannot be caught by any users of this library as it is an internal process.

Please correct me if this pull request should be to a different base branch as I am not familiar with the current branch structure.

Requirements

This will add necessary optional chaining when the body is undefined. This occurs when the app is running in socket mode and then socket mode is disabled from the app settings. Currently, when this happens the app will crash. The optional chaining here will prevent that.
@CLAassistant
Copy link

CLAassistant commented Aug 16, 2022

CLA assistant check
All committers have signed the CLA.

@rileyeaton
Copy link
Contributor Author

@seratch I can't add reviewers now that I have submitted this. The maintainers (such as yourself?) will need to add them. Thanks!

@WilliamBergamin WilliamBergamin added the bug M-T: confirmed bug report. Issues are confirmed when the reproduction steps are documented label Aug 16, 2022
@rileyeaton
Copy link
Contributor Author

The root of this issue stems from why the body property is undefined, however, this quick fix is necessary first to stop user's applications from crashing.

@rileyeaton rileyeaton mentioned this pull request Aug 16, 2022
10 tasks
Copy link
Contributor

@WilliamBergamin WilliamBergamin left a comment

Choose a reason for hiding this comment

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

Hi @rileyeaton thanks for catching this and contributing!

LGTM!

@codecov
Copy link

codecov bot commented Aug 16, 2022

Codecov Report

Merging #1553 (cb54385) into main (1e15a68) will decrease coverage by 0.06%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #1553      +/-   ##
==========================================
- Coverage   82.08%   82.01%   -0.07%     
==========================================
  Files          18       18              
  Lines        1496     1496              
  Branches      435      436       +1     
==========================================
- Hits         1228     1227       -1     
  Misses        172      172              
- Partials       96       97       +1     
Impacted Files Coverage Δ
src/receivers/SocketModeFunctions.ts 88.88% <0.00%> (-11.12%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

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.

4 participants