Socket Mode Rewrite to Python(ish) Implementation #1781

filmaj · 2024-04-19T19:41:17Z

Socket Mode Rewrite

This PR ports, as much as possible, the Python implementation of the Socket Mode client to node.

I have been running a longevity test of an app, in production, on a busy workspace, using this PR since April 24th without issue (the app reconnects gracefully when Slack sends a disconnect message, which is what we expect).

Why?

The existing node socket-mode package uses finity, an old finite state machine configuration package. While it has served us well, it has been difficult to maintain, with tricky semantics. Additionally, the existing socket-mode implementation is almost a straight copy-paste of the rtm-api module, which is very old.

Breaking Changes

Removed two lifecycle events - though we can add them back in if we think they are useful / worth keeping for backwards compatibility:
- authenticated. I did not find it useful, it just represented that the client retrieved the WS URL to connect to via a successful response from the apps.connections.open HTTP API.
- unable_to_start_socket_mode. I felt like the error event was sufficient?
Some properties and methods that were previously public on SocketModeClient have been removed: connected, authenticated and isActive(), though isActive() could easily be added back in.

What's The Same?

Many of the developer-facing APIs have remained the same:

the EventEmitter-based design, where the client emits the events described in the README, remain identical. This should ease the upgrading path for any projects dependent on this package, including bolt-js.
I think SocketModeClient's standard public API also remains the same? start() and disconnect() for sure, but isActive() is now moved to SlackWebSocket, so that's a major change - though to be fair, we never documented these APIs.

Internal Design Changes

SocketModeClient aims to implement similar interfaces as Python's SocketModeClient class, as well as its BaseSocketModeClient.
- The Client is responsible for retrieving the WebSocket URL to connect to (by interacting with the apps.connections.open HTTP API) and managing reconnections. We can consider these as application-level concerns.
SlackWebSocket aims to implement similar interfaces as Python's Connection class. Luckily for the node implementation, many of the low-level details of the WebSocket protocol are encapsulated in the ws dependency.
- SlackWebSocket concerns itself with managing and maintaining a single WebSocket connection and nothing more; any WS-specific events relevant to application functioning (e.g. disconnects) are raised back to the Client for it to deal with.

IMO this leads to a nice separation of concerns.

Concerns / Questions / Request for Feedback

Moving to an object-oriented / class-based setup, actually unit testing the private methods in JavaScript is.. impossible? So I think the code coverage from unit tests will drop significantly as a result. Any suggestions on how to tackle this? Could just make methods public and prefix with an _ just to expose them for unit tests?
Seems like the documentation on https://slack.dev/node-slack-sdk/socket-mode is just a copy paste of the README (and even the comments in the readme markdown state as much). Can we instead link to the README from the slack.dev site? I feel like the chances of the package documentation being kept up to date are better by being in the README.

…t of the build woods.

- added some more jsdocs - if a logger is provided, also override the SlackWebSocket logger - fix emitting WS messages to the client - factor out connection termination into a `terminate()` method - do not reconnect on an explicit `disconnect()` call to the client - add and emit an authenticated state to match existing lifecycle events

…slint rules, using import for package json now that we accept importing json files.

…kage json to reflect that.

filmaj · 2024-04-25T16:30:47Z

docs/_packages/socket_mode.md

@@ -7,133 +7,11 @@ anchor_links_header: Usage

 # Slack Socket Mode

+For baseline package documentation, please see the project's [`README`](https://github.com/slackapi/node-slack-sdk/tree/main/packages/socket-mode#readme) or the [documentation on npm](https://www.npmjs.com/package/@slack/socket-mode).


Mainly axing content in here, as it used to be a straight copy/paste from the socket mode README. No need to duplicate, just link to the duplicated content and keep this area for additional documentation (if necessary).

Yeah, this sounds good

filmaj · 2024-04-25T16:31:48Z

packages/socket-mode/package.json

-  "main": "dist/index.js",
-  "types": "./dist/index.d.ts",
+  "main": "dist/src/index.js",
+  "types": "./dist/src/index.d.ts",


These changes are needed to enable the resolveJsonModule option in the tsconfig.json, as setting that option changes the output of the TypeScript compiler.

filmaj · 2024-04-25T16:33:18Z

packages/socket-mode/src/logger.ts

-  const instanceId = instanceCount;
-  instanceCount += 1;
+export default {
+  getLogger: function getLogger(name: string, level: LogLevel, existingLogger?: Logger): Logger {


This restructuring of exports-under-a-single-exported-object allows for easier mocking without the need for a tool like rewire or proxyquire.

WilliamBergamin

This is awesome 💯 I'm a fan of the OOP approach, this version is definitely easier to understand

I think respecting object encapsulation instead of making things public with _ prefix will yield more maintainable code
Faking/Mocking a socket mode server to test the the intended behavior of SlackWebSocket seems like a viable long term testing solution since this API is stable

WilliamBergamin · 2024-04-29T15:27:22Z

packages/socket-mode/src/SlackWebSocket.ts

+      this.logger.debug('isActive(): websocket not instantiated!');
+      return false;
+    }
+    this.logger.debug(`isActive(): websocket ready state is ${WS_READY_STATES[this.websocket.readyState]}`);


Curious to know why this array pattern is used instead of some form of Enum?

No specific reason! I can change it to an enum if you'd prefer that - lemme know

I prefer the enum
it should make the code self explanatory, the comment on the following will no longer be needed

Hmmm looking at this, the reason I had the array is that the websocket ready state returns an integer. Unclear how I would map from an integer to the enum? Enums seem to map variable names to integers.

Ahh I see, adding the Enum means you need to search for the state based on the integer

In that case sticking to the array pattern seems valid

packages/socket-mode/src/SlackWebSocket.spec.ts

lint-configs/.eslintrc.js

…etection, fixed an issue with running unit vs. integration tests locally, added integration tests for ping and pong events and stale connection detection/reconnection.

seratch

Thanks for this great work; I don't see any issues so far

seratch · 2024-04-30T00:58:34Z

docs/_packages/socket_mode.md

@@ -7,133 +7,11 @@ anchor_links_header: Usage

 # Slack Socket Mode

+For baseline package documentation, please see the project's [`README`](https://github.com/slackapi/node-slack-sdk/tree/main/packages/socket-mode#readme) or the [documentation on npm](https://www.npmjs.com/package/@slack/socket-mode).


Yeah, this sounds good

seratch · 2024-04-30T01:00:26Z

packages/socket-mode/README.md

 The package exports a `SocketModeClient` class. Your app will create an instance of the class for each workspace it communicates with. Creating an instance requires an [**app-level token**][app-token] from Slack. Apps connect to the **Socket Mode** API using an [**app-level token**][app-token], which starts with `xapp`.

-Note: **Socket Mode** requires the `connections:write` scope. Navigate to your [app configuration](https://api.slack.com/apps) and go to the **OAuth and Permissions** section to add the scope.


Good catch; this had been completely incorrect

…fter the client starts, and not after the timeout period.

filmaj · 2024-04-30T12:52:52Z

Thanks for the reviews! I did see some CI failures with the integration tests, and I realized some of the sequencing of events / websocket behaviour in the test was not ideal and probably brittle on a weaker machines (like GitHub Actions). Running them once on my local machine tended to pass them, but if I ran the tests like this instead:

while npm run test:integration; do echo "OK"; done

.. after about 10 or so runs I could trigger a failure.

I made a change in the tests to make them more resilient (any event-based waiting promises should be created after start() completes, not after any timeouts; ensures the event loop has opportunity to execute a few iterations to ensure events get picked up properly by any waiters) and now they pass on both local and CI at a much higher rate.

Also confirming that the longevity test I had set up has been operating flawlessly for a week.

Merging this! Hooray! I will likely publish a 2.0 release of socket-mode soon!

Filip Maj added 6 commits April 19, 2024 15:35

partway thru rewrite

a85a45a

more rewriting

0018690

further rewrite progress; SlackWebSocket wrapper class is at least ou…

ee578cf

…t of the build woods.

dont need basesocketmode

fc9c584

lint fixes in slackwebsocket

b875c07

rewrite kinda done?

7fcd13c

filmaj added semver:major draft pkg:socket-mode applies to `@slack/socket-mode` labels Apr 19, 2024

filmaj self-assigned this Apr 19, 2024

Filip Maj added 6 commits April 19, 2024 17:10

moar jsdocs, fixing unit tests, using typescript for tests, tweakin e…

982602f

…slint rules, using import for package json now that we accept importing json files.

run the rewrite under a temporary 2.0.0 rc version

5100f0d

resolvejsonmodule tsconfig changes the output of tsc build; tweak pac…

2e166ad

…kage json to reflect that.

2.0rc2

972630e

Tweaking debug messages a bit

0a144cb

filmaj removed the draft label Apr 25, 2024

README updates

3e932ed

filmaj requested a review from a team April 25, 2024 16:29

filmaj commented Apr 25, 2024

View reviewed changes

filmaj marked this pull request as ready for review April 25, 2024 16:34

filmaj changed the title ~~WIP Socket Mode Rewrite to Python(ish) Implementation~~ Socket Mode Rewrite to Python(ish) Implementation Apr 25, 2024

WilliamBergamin reviewed Apr 29, 2024

View reviewed changes

a few more comments related to ping/pong websocket stale connection d…

066bc5a

…etection, fixed an issue with running unit vs. integration tests locally, added integration tests for ping and pong events and stale connection detection/reconnection.

filmaj requested a review from WilliamBergamin April 29, 2024 19:29

missed removing this only from the tests

5cb3e57

seratch approved these changes Apr 30, 2024

View reviewed changes

Turn off debug logging in integration tests

b8f3a30

Filip Maj added 3 commits April 30, 2024 08:10

force disconnect client after each test, in case test hits timeouts

143f453

debugging a test in CI

5f2be02

create the reconnection waiter in the integration tests immediately a…

7b13b09

…fter the client starts, and not after the timeout period.

filmaj added this to the [email protected] milestone Apr 30, 2024

filmaj merged commit 476e6a9 into main Apr 30, 2024
17 checks passed

filmaj deleted the socket-mode-rewrite branch April 30, 2024 12:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Socket Mode Rewrite to Python(ish) Implementation #1781

Socket Mode Rewrite to Python(ish) Implementation #1781

filmaj commented Apr 19, 2024 •

edited

Loading

filmaj Apr 25, 2024

seratch Apr 30, 2024

filmaj Apr 25, 2024

filmaj Apr 25, 2024 •

edited

Loading

WilliamBergamin left a comment

WilliamBergamin Apr 29, 2024

filmaj Apr 29, 2024

WilliamBergamin Apr 29, 2024

filmaj Apr 29, 2024

WilliamBergamin Apr 29, 2024

seratch left a comment

seratch Apr 30, 2024

seratch Apr 30, 2024

filmaj commented Apr 30, 2024

		@@ -7,133 +7,11 @@ anchor_links_header: Usage

		# Slack Socket Mode

		For baseline package documentation, please see the project's [`README`](https://github.com/slackapi/node-slack-sdk/tree/main/packages/socket-mode#readme) or the [documentation on npm](https://www.npmjs.com/package/@slack/socket-mode).

		The package exports a `SocketModeClient` class. Your app will create an instance of the class for each workspace it communicates with. Creating an instance requires an [app-level token][app-token] from Slack. Apps connect to the Socket Mode API using an [app-level token][app-token], which starts with `xapp`.

		Note: Socket Mode requires the `connections:write` scope. Navigate to your [app configuration](https://api.slack.com/apps) and go to the OAuth and Permissions section to add the scope.

Socket Mode Rewrite to Python(ish) Implementation #1781

Socket Mode Rewrite to Python(ish) Implementation #1781

Conversation

filmaj commented Apr 19, 2024 • edited Loading

Socket Mode Rewrite

Why?

Breaking Changes

What's The Same?

Internal Design Changes

Concerns / Questions / Request for Feedback

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filmaj Apr 25, 2024 • edited Loading

Choose a reason for hiding this comment

WilliamBergamin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seratch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filmaj commented Apr 30, 2024

filmaj commented Apr 19, 2024 •

edited

Loading

filmaj Apr 25, 2024 •

edited

Loading