rtm-api: add support for custom webClient by ishangarg0 · Pull Request #1696 · slackapi/node-slack-sdk

ishangarg0 · 2023-11-23T08:01:10Z

Summary

Adding optional parameter to get more flexibility with webClient construction

Requirements (place an `x` in each `[ ]`)

I've read and understood the Contributing Guidelines and have done my best effort to follow them.
I've read and agree to the Code of Conduct.

- Update constructor to optionally allow for custom webClient

This reverts commit 9a842ee.

salesforce-cla · 2023-11-23T08:01:15Z

Thanks for the contribution! Before we can merge this, we need @ishangarg0 to sign the Salesforce Inc. Contributor License Agreement.

mwbrooks

Thanks for the contribution @ishangarg0 🙇🏻

The addition of webClient looks good to me and since rtm-api doesn't have any tests, I don't think we need to add a test.

I'm going to loop in another maintainer to give the PR a second pair of 👀 eyes, in case there was a historical reason to not support custom webClients!

Thanks for the signing the CLA btw 😄

filmaj

How does this new webClient option interact with the slackApiUrl that can be passed into the constructor, which sets the URL that the web client will interact with? If both webClient and slackApiUrl are set, what should this package do?

Assuming we can resolve that question, we should also expand the documentation/reference to describe this. In particular:

the reference documentation in the file docs/_reference/rtm-api.md
the main package description in the file docs/_packages/rtm_api.md, which maps to what is seen on https://slack.dev/node-slack-sdk/rtm-api

ishangarg0 · 2023-11-24T17:49:42Z

The webClient option will take precedence over any of the other arguments passed into the constructor. If the webClient option is not set, then the constructor will use the passed in value for slackApiUrl.

I'll update the documentation if everything looks good.

filmaj · 2023-11-24T17:50:36Z

The webClient option will take precedence over any of the other arguments passed into the constructor. If the webClient option is not set, then the constructor will use the passed in value for slackApiUrl.

Sounds good to me, let's just update the docs to reflect this fact. Thank you!

- updated docs to reflect new custom webClient parameter for RTMClient constructor

ishangarg0 · 2023-11-27T09:04:03Z

Updated the docs, let me know if there are any details missing. Thanks!

filmaj

LGTM, left some minor formatting / linking / phrasing suggestions. Please ping me once updated and I can review once more and merge if ready.

Thanks a lot for doing this! <3

filmaj · 2023-11-27T14:05:00Z


+### Custom WebClient
+
+In some cases, you might want flexibility with the WebClient construction beyond the provided RTMClient options


Suggested change

In some cases, you might want flexibility with the WebClient construction beyond the provided RTMClient options

In some cases, you might want to customize the underlying component making HTTP requests to the Slack API, the [`WebClient`](reference/web-api#webclient), beyond the provided [`RTMClientOptions`](reference/rtm-api#rtmclientoptions). Note that overriding the [`WebClient`](reference/web-api#webclient) instance takes precedence over any other [`RTMClientOptions`](reference/rtm-api#rtmclientoptions) specified.

filmaj · 2023-11-27T14:08:52Z

+const webClient = new WebClient(token, options);
+
+// Initialize a client using the configuration
+const rtm = new RTMClient(token, {webClient});


Suggested change

const rtm = new RTMClient(token, {webClient});

const rtm = new RTMClient(token, { webClient });

filmaj · 2023-11-27T14:09:22Z

 </tr>
+<tr>
+<td align="center">webClient</td>
+<td align="center"><code><a href="https://github.com/slackapi/node-slack-sdk/blob/main/docs/_reference/web-api.md#webclient" title="">WebClient</a></code></td>


Suggested change

<td align="center"><code><a href="https://github.com/slackapi/node-slack-sdk/blob/main/docs/_reference/web-api.md#webclient" title="">WebClient</a></code></td>

<td align="center"><code><a href="web-api#webclient" title="">WebClient</a></code></td>

Link to hosted docs instead of github

filmaj · 2023-11-27T14:11:23Z

+<td align="center">webClient</td>
+<td align="center"><code><a href="https://github.com/slackapi/node-slack-sdk/blob/main/docs/_reference/web-api.md#webclient" title="">WebClient</a></code></td>
+<td align="center">✗</td>
+<td align="center">An optional parameter to provide a customized <a href="https://github.com/slackapi/node-slack-sdk/blob/main/docs/_reference/web-api.md#webclient">WebClient</a>. Any desired options for the custom client must be set in this parameter (<code>webClient</code>) as they will take precedence over other arguments passed into <code>RTMClient</code>.</td>


Suggested change

<td align="center">An optional parameter to provide a customized <a href="https://github.com/slackapi/node-slack-sdk/blob/main/docs/_reference/web-api.md#webclient">WebClient</a>. Any desired options for the custom client must be set in this parameter (<code>webClient</code>) as they will take precedence over other arguments passed into <code>RTMClient</code>.</td>

<td align="center">An optional parameter to provide a customized <a href="web-api#webclient">WebClient</a>. Any desired options for the custom client must be set in this parameter (<code>webClient</code>) as they will take precedence over other arguments passed into <code>RTMClient</code>.</td>

Link to hosted docs instead of github

- minor formatting, linking, and phrasing updates to rtm-api docs

ishangarg0 · 2023-11-27T18:18:24Z

@filmaj Thanks for the suggestions, I pushed the changes let me know how it looks!

filmaj

Looks great, thank you! Gonna let the tests run and then will merge and release 😄

filmaj · 2023-11-27T18:45:02Z

This is now live on [email protected] on npm and on GitHub. Thanks again for your contribution, @ishangarg0 !

ishangarg0 added 3 commits November 22, 2023 23:22

Patch RTMClient.ts

2c3af95

- Update constructor to optionally allow for custom webClient

Add custom headers to websocket

9a842ee

Revert "Add custom headers to websocket"

7537d8a

This reverts commit 9a842ee.

salesforce-cla Bot added the cla:missing label Nov 23, 2023

salesforce-cla Bot added cla:signed and removed cla:missing labels Nov 23, 2023

mwbrooks added semver:minor enhancement M-T: A feature request for new functionality labels Nov 23, 2023

mwbrooks added this to the [email protected] milestone Nov 23, 2023

mwbrooks reviewed Nov 23, 2023

View reviewed changes

filmaj suggested changes Nov 24, 2023

View reviewed changes

update rtm-api docs

fab93d4

- updated docs to reflect new custom webClient parameter for RTMClient constructor

filmaj suggested changes Nov 27, 2023

View reviewed changes

update rtm-api docs

b7dd265

- minor formatting, linking, and phrasing updates to rtm-api docs

filmaj approved these changes Nov 27, 2023

View reviewed changes

filmaj merged commit 62e4570 into slackapi:main Nov 27, 2023

ishangarg0 deleted the rtm-api-patch branch November 27, 2023 18:27


		### Custom WebClient

		In some cases, you might want flexibility with the WebClient construction beyond the provided RTMClient options

	const rtm = new RTMClient(token, {webClient});
	const rtm = new RTMClient(token, { webClient });

	<td align="center"><code><a href="https://github.com/slackapi/node-slack-sdk/blob/main/docs/_reference/web-api.md#webclient" title="">WebClient</a></code></td>
	<td align="center"><code><a href="web-api#webclient" title="">WebClient</a></code></td>

Conversation

ishangarg0 commented Nov 23, 2023

Summary

Requirements (place an x in each [ ])

Uh oh!

salesforce-cla Bot commented Nov 23, 2023

Uh oh!

mwbrooks left a comment

Choose a reason for hiding this comment

Uh oh!

filmaj left a comment

Choose a reason for hiding this comment

Uh oh!

ishangarg0 commented Nov 24, 2023

Uh oh!

filmaj commented Nov 24, 2023

Uh oh!

ishangarg0 commented Nov 27, 2023

Uh oh!

filmaj left a comment

Choose a reason for hiding this comment

Uh oh!

filmaj Nov 27, 2023

Choose a reason for hiding this comment

Uh oh!

filmaj Nov 27, 2023

Choose a reason for hiding this comment

Uh oh!

filmaj Nov 27, 2023

Choose a reason for hiding this comment

Uh oh!

filmaj Nov 27, 2023

Choose a reason for hiding this comment

Uh oh!

ishangarg0 commented Nov 27, 2023

Uh oh!

filmaj left a comment

Choose a reason for hiding this comment

Uh oh!

filmaj commented Nov 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Requirements (place an `x` in each `[ ]`)