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

[socket-mode] Pass an agent to WebSocket in a proper way #1162

Merged
merged 2 commits into from Jan 22, 2021

Conversation

g12i
Copy link
Contributor

@g12i g12i commented Jan 21, 2021

Summary

Fix passing an agent to WebSocket. this.clientOptions.agent was wrongly spread to WebSocket options, which effectively mean it wasn't used.

Requirements (place an x in each [ ])

@gitwave gitwave bot added the untriaged label Jan 21, 2021
@g12i g12i changed the title Pass agent in a proper way [socket-mode] Pass agent in a proper way Jan 21, 2021
@g12i g12i changed the title [socket-mode] Pass agent in a proper way [socket-mode] Pass an agent tp WebSocket in a proper way Jan 21, 2021
@g12i g12i changed the title [socket-mode] Pass an agent tp WebSocket in a proper way [socket-mode] Pass an agent to WebSocket in a proper way Jan 21, 2021
@mwbrooks mwbrooks added bug M-T: A confirmed bug report. Issues are confirmed when the reproduction steps are documented pkg:socket-mode applies to `@slack/socket-mode` semver:patch and removed untriaged labels Jan 21, 2021
@mwbrooks
Copy link
Member

Hey @g12i, thanks for catching this issue and sending a patch! I've added a couple reviewers, so someone will pick up it up shortly to confirm the bug and review the work.

If you have some time, I think we should also consider adding a test to prevent regression. :)

@g12i
Copy link
Contributor Author

g12i commented Jan 21, 2021

Hey @mwbrooks - trust me this is the first thing I wanted to do :)

PS C:\Users\wgrze\Projects\node-slack-sdk\packages\socket-mode> npm run test

> @slack/socket-mode@1.0.0 test C:\Users\wgrze\Projects\node-slack-sdk\packages\socket-mode
> npm run build && echo "Tests are not implemented." && exit 0


> @slack/socket-mode@1.0.0 build C:\Users\wgrze\Projects\node-slack-sdk\packages\socket-mode
> npm run build:clean && tsc


> @slack/socket-mode@1.0.0 build:clean C:\Users\wgrze\Projects\node-slack-sdk\packages\socket-mode
> shx rm -rf ./dist

"Tests are not implemented."

I see mocha in some packages but it's not defined in the root package.json. And I don't know the project well enough to build the test setup myself.

@mwbrooks
Copy link
Member

mwbrooks commented Jan 21, 2021

@g12i oops, my bad! SocketMode is hot off the press and don't have specs written for it yet. We've been looking into a reliable way to run non-integration tests and will most likely draw inspiration from the SocketMode tests for Bolt Python and Java.

Co-authored-by: Wojciech Grzebieniowski <grzebieniowski@gmail.com>
Copy link
Member

@stevengill stevengill left a comment

Choose a reason for hiding this comment

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

Thanks for catching this!

@stevengill stevengill merged commit b7186ed into slackapi:main Jan 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug M-T: A confirmed bug report. Issues are confirmed when the reproduction steps are documented pkg:socket-mode applies to `@slack/socket-mode` semver:patch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants