Node-slack-sdk: make webclient rate-limit retries configurable

[rodrigo4244]

Description

I read issue #305 that explained that giving a retryConfig object for WebClient it would be possible to disable the retry operation but it actually isn't (I explain properly on steps to reproduce).
On your docs is: https://slackapi.github.io/node-slack-sdk/web_api#changing-the-retry-configuration.

What type of issue is this? (place an x in one of the [ ])

• [ ] bug
• [x] enhancement (feature request)
• [ ] question
• [ ] documentation related
• [ ] testing related
• [ ] discussion

Requirements (place an x in each of the [ ])

• [x] I've read and understood the Contributing guidelines and have done my best effort to follow them.
• [x] I've read and agree to the Code of Conduct.
• [x] I've searched for any related issues and avoided creating a duplicate issue.

---

Bug Report

Filling out the following details about bugs will help us solve your issue sooner.

Reproducible in:

@slack/client version: 3.15.0

node version: 8.9.3

OS version(s): Ubuntu 16.04 LTS

Steps to reproduce:

1. Run the following script until you get Rate limited errors. (make sure you're using a token from a workspace which have a good amount of channels, otherwise it could take long)

const { WebClient } = require('@slack/client')

const SLACK_USER_TOKEN='<token here>'

const slack = new WebClient(SLACK_USER_TOKEN, { retryConfig: { retries: 0 }, maxRequestConcurrency: Infinity })

for (let i = 1; i <= 1000; i++) {
  slack.channels.list()
    .catch(e => console.log('this never gets called'))
}

Expected result:

I expected that @slack/client would throw an error so I can throttle API calls myself.

Actual result:

@slack/client is automatically retrying.

Attachments:

[gunar]

It seems handleRateLimitResponse is a special case and doesn't really use the library retry.

[aoberoi]

thanks for point that out @gunar. you're right, and the docs don't specify whether the retry configuration should apply for rate-limited requests. the key information is buried in the code comments:

https://github.com/slackapi/node-slack-sdk/blob/1dfe3ea63bf6bf803ba6031790bfd22c7bab41e0/lib/clients/transports/call-transport.js#L141-L145

@rodrigo4244 can i take this issue to mean that you have a feature request for turning rate-limit handling off in the WebClient?

[rodrigo4244]

Thanks @aoberoi! I already edited the title and the message to reflect that! Let me know as soon as this is up and running. Thanks!

[aoberoi]

@rodrigo4244 lets try to define what "working properly" might mean for you. do you simply want an on/off switch so you can say "do not retry any rate-limited requests"? or do you want the existing retryConfig to apply to rate-limited requests just the same as requests that fail for other reasons (network interruptions, minor service downtime, etc)? or do you want a fully independent rateLimitRetryConfig that applies differently to rate-limited requests (leaving retryConfig to only apply to the other failure reasons)?

[rodrigo4244]

@aoberoi I want rateLimitRetryConfig!
I just didn't want rate limited requests to be automatically retried and, instead, I want them to throw.

[gunar]

same here

[aoberoi]

thanks for the feedback @rodrigo4244 and @gunar! i'm happy to work on getting this feature into master on a v4 version of this package. if you'd rather see it landed on v3, please let me know so we can make another issue to track that separately.

[gunar]

@aerobi whatever is fastest. Thank you!!

[aoberoi]

Hey folks, I've refactored the code where retries occur in a substantial way for #596. This has opened the door for me to build a new option to address this need too. I'd like to confirm with you all whether you'd be satisfied by what I'm planning on adding.

proposal: a new rejectRateLimitedCalls boolean configuration option for the WebClient constructor. the default would be false, but when set to true, the client would return a rejected promise (or invoke your callback) with an Error. This error would be of interface (in the TypeScript sense) WebAPIRateLimitedError. The error would also have a retryAfter property, that contains the number of seconds to wait, according to the HTTP response from Slack.

Edit: Changed interface from WebAPIPlatformError to a new WebAPIRateLimitedError.