8000 Socket mode using node v20 by WilliamBergamin · Pull Request #1918 · slackapi/bolt-js · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Socket mode using node v20 #1918

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Aug 4, 2023
Merged

Socket mode using node v20 #1918

merged 3 commits into from
Aug 4, 2023

Conversation

WilliamBergamin
Copy link
Contributor

Summary

This PR aims to solve #1917

In Node v19.5.0: A successful websocket.send returns a null value, instead of undefined, since this project supports Node v20 this use case must be handled properly.

Luckily slackapi/node-slack-sdk#1546 in the SocketModeClient resolves this issue, bumping @slack/socket-mode: ^1.3.0 to ^1.3.2 should fix the issue

testing

  1. follow the README instructions and install the bolt-js-starter-template
  2. Clone this repo and switch to the socket-mode-using-node-v20 branch
  3. Make sure you are using Node v19.5.0 or higher
  4. In your bolt-js-starter-template project
    a. run npm uninstall @slack/bolt
    b. run npm install /path/to/your/clone/bolt-js
  5. run npm start
  6. In slack, execute the /sample-command of the template app

Requirements

@WilliamBergamin WilliamBergamin added semver:patch dependencies Pull requests that update a dependency file labels Aug 2, 2023
@WilliamBergamin WilliamBergamin requested a review from filmaj August 2, 2023 16:25
@WilliamBergamin WilliamBergamin self-assigned this Aug 2, 2023
@codecov
Copy link
codecov bot commented Aug 2, 2023 •
edited
Loading

Codecov Report

Merging #1918 (aae0934) into main (ba5b893) will not change coverage.
The diff coverage is n/a.

❗ Current head aae0934 differs from pull request most recent head 6600e75. Consider uploading reports for the commit 6600e75 to get more accurate results

@@           Coverage Diff           @@
##             main    #1918   +/-   ##
=======================================
  Coverage   82.18%   82.18%           
=======================================
  Files          18       18           
  Lines        1521     1521           
  Branches      436      436           
=======================================
  Hits         1250     1250           
  Misses        175      175           
  Partials       96       96

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

@filmaj
Copy link
Contributor
filmaj commented Aug 2, 2023

Any ideas on what we could do to add a test and check for this? A test that would fail on main on node20 but pass in this branch?

@WilliamBergamin
Copy link
Contributor Author

I'm not sure how to simply test this and prevent errors like this from coming up again 😟

it seems like we may need E2E tests to solve this, with Node v20 all the unit tests passed and the template app started with no issues, its only when a request from the SlackAPI came along that this error occurred

In theory we had already solved this problem, maybe we could force dependabot to keep up with the latests patch versions of our dependencies

@filmaj
Copy link
Contributor
filmaj commented Aug 4, 2023

This does fix running in node v20 locally. Going to merge this and try to cut a release today, and we can think of how to prevent regressions on this issue moving forward in the background.

@filmaj filmaj merged commit 4626339 into main Aug 4, 2023
@filmaj filmaj deleted the socket-mode-using-node-v20 branch August 4, 2023 13:38
@filmaj filmaj added this to the 3.13.3 milestone Aug 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@filmaj filmaj filmaj approved these changes

Assignees

@WilliamBergamin WilliamBergamin

Labels
dependencies Pull requests that update a dependency file semver:patch
Projects
None yet
Milestone
3.13.3
Development

Successfully merging this pull request may close these issues.
2 participants
@WilliamBergamin @filmaj
0