-
Notifications
You must be signed in to change notification settings - Fork 682
feat(webhook): add opt-in retries to IncomingWebhook and WebhookTrigger #2641
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
Changes from all commits
7ce2c4a
89c7af2
0aab45b
2dcef0f
a0e3e1d
a0e845e
4f0f591
ef40e60
7c8e9ca
29ea38e
a4982fc
74f0dcd
700af6a
d6677bf
817cfd0
236d954
7d56341
cd50095
2724cd7
b8992d8
eab95b3
ceba283
eace3ba
b02a78e
e3626f9
7968168
38a310a
345ff5b
3d9a0af
a2868ad
ee7f29f
fce391b
dc3dc2d
4e0f4f5
3bed3dc
c0f5c7e
839751b
5d1a1ed
e6604b6
692179f
1684fa8
edd3ecb
5ddeb03
baea6ec
24c4a3b
3be520f
338455c
35d7096
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "@slack/webhook": minor | ||
| --- | ||
|
|
||
| feat: add opt-in retries to `IncomingWebhook` and `WebhookTrigger` |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,9 +2,11 @@ import type { Agent } from 'node:http'; | |
|
|
||
| import type { Block, KnownBlock, MessageAttachment } from '@slack/types'; // TODO: Block and KnownBlock will be merged into AnyBlock in upcoming types release | ||
| import axios, { type AxiosInstance, type AxiosResponse } from 'axios'; | ||
| import pRetry, { AbortError } from 'p-retry'; | ||
|
|
||
| import { httpErrorWithOriginal, requestErrorWithOriginal } from './errors'; | ||
| import { getUserAgent } from './instrument'; | ||
| import type { RetryOptions } from './retry-policies'; | ||
|
|
||
| /** | ||
| * A client for Slack's Incoming Webhooks | ||
|
|
@@ -25,6 +27,11 @@ export class IncomingWebhook { | |
| */ | ||
| private axios: AxiosInstance; | ||
|
|
||
| /** | ||
| * Retry policy applied to each send. Defaults to no retries. | ||
| */ | ||
| private retryConfig: RetryOptions; | ||
|
|
||
| public constructor( | ||
| url: string, | ||
| defaults: IncomingWebhookDefaultArguments = { | ||
|
|
@@ -37,6 +44,7 @@ export class IncomingWebhook { | |
|
|
||
| this.url = url; | ||
| this.defaults = defaults; | ||
| this.retryConfig = defaults.retryConfig ?? { retries: 0 }; | ||
|
|
||
| this.axios = axios.create({ | ||
| baseURL: url, | ||
|
|
@@ -50,7 +58,9 @@ export class IncomingWebhook { | |
| }, | ||
| }); | ||
|
|
||
| // Strip transport-only options so they do not leak into the posted payload. | ||
| this.defaults.agent = undefined; | ||
| this.defaults.retryConfig = undefined; | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -67,20 +77,25 @@ export class IncomingWebhook { | |
| payload = Object.assign(payload, message); | ||
| } | ||
|
|
||
| try { | ||
| const response = await this.axios.post(this.url, payload); | ||
| return this.buildResult(response); | ||
| // biome-ignore lint/suspicious/noExplicitAny: errors can be anything | ||
| } catch (error: any) { | ||
| // Wrap errors in this packages own error types (abstract the implementation details' types) | ||
| if (error.response !== undefined) { | ||
| throw httpErrorWithOriginal(error); | ||
| return pRetry(async () => { | ||
| try { | ||
| const response = await this.axios.post(this.url, payload); | ||
| return this.buildResult(response); | ||
| // biome-ignore lint/suspicious/noExplicitAny: errors can be anything | ||
| } catch (error: any) { | ||
| // Wrap errors in this packages own error types (abstract the implementation details' types) | ||
| if (error.response !== undefined) { | ||
| const status: number = error.response.status; | ||
| const wrapped = httpErrorWithOriginal(error); | ||
| throw status >= 500 ? wrapped : new AbortError(wrapped); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice 💯 thanks for simplifying this 🚀 |
||
| } | ||
| if (error.request !== undefined) { | ||
| // No response received (network/timeout): retryable. | ||
| throw requestErrorWithOriginal(error); | ||
| } | ||
| throw new AbortError(error); | ||
| } | ||
| if (error.request !== undefined) { | ||
| throw requestErrorWithOriginal(error); | ||
| } | ||
| throw error; | ||
| } | ||
| }, this.retryConfig); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -105,6 +120,7 @@ export interface IncomingWebhookDefaultArguments { | |
| text?: string; | ||
| link_names?: boolean; | ||
| agent?: Agent; | ||
| retryConfig?: RetryOptions; | ||
| timeout?: number; | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,7 @@ import nock from 'nock'; | |
| import type { CodedError, WebhookTriggerHTTPError } from './errors'; | ||
| import { ErrorCode } from './errors'; | ||
| import { addAppMetadata } from './instrument'; | ||
| import { rapidRetryPolicy } from './retry-policies'; | ||
| import { WebhookTrigger } from './WebhookTrigger'; | ||
|
|
||
| const url = 'https://hooks.slack.com/triggers/FAKETRIGGER'; | ||
|
|
@@ -154,5 +155,83 @@ describe('WebhookTrigger', () => { | |
| } | ||
| }); | ||
| }); | ||
|
|
||
| describe('retries', () => { | ||
| it('retries a 5xx then succeeds when a retry policy is set', async () => { | ||
| const scope = nock('https://hooks.slack.com') | ||
| .post(/triggers/) | ||
| .reply(503) | ||
| .post(/triggers/) | ||
| .reply(200, { ok: true }); | ||
| const trigger = new WebhookTrigger(url, { retryConfig: rapidRetryPolicy }); | ||
| const result = await trigger.send({ key: 'value' }); | ||
| assert.strictEqual(result.ok, true); | ||
| scope.done(); | ||
| }); | ||
|
|
||
| it('does not retry a 4xx even when a retry policy is set', async () => { | ||
| const scope = nock('https://hooks.slack.com') | ||
| .post(/triggers/) | ||
| .reply(400); | ||
| const trigger = new WebhookTrigger(url, { retryConfig: rapidRetryPolicy }); | ||
| try { | ||
| await trigger.send({ key: 'value' }); | ||
| assert.fail('expected rejection'); | ||
| } catch (error) { | ||
| assert.strictEqual((error as CodedError).code, ErrorCode.HTTPError); | ||
| } | ||
| // Only one interceptor is registered; a retry would leave it unmatched | ||
| // and scope.done() would throw. | ||
| scope.done(); | ||
|
Comment on lines
+172
to
+185
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Praise 💯 |
||
| }); | ||
|
|
||
| it('does not retry a 429 even when a retry policy is set', async () => { | ||
| const scope = nock('https://hooks.slack.com') | ||
| .post(/triggers/) | ||
| .reply(429); | ||
| const trigger = new WebhookTrigger(url, { retryConfig: rapidRetryPolicy }); | ||
| try { | ||
| await trigger.send({ key: 'value' }); | ||
| assert.fail('expected rejection'); | ||
| } catch (error) { | ||
| assert.strictEqual((error as CodedError).code, ErrorCode.HTTPError); | ||
| } | ||
| // Only one interceptor is registered; a retry would leave it unmatched | ||
| // and scope.done() would throw. | ||
| scope.done(); | ||
| }); | ||
|
|
||
| it('gives up with the HTTP error after exhausting retries', async () => { | ||
| const scope = nock('https://hooks.slack.com') | ||
| .post(/triggers/) | ||
| .reply(500) | ||
| .post(/triggers/) | ||
| .reply(500); | ||
| const trigger = new WebhookTrigger(url, { | ||
| retryConfig: { retries: 1, minTimeout: 0, maxTimeout: 1 }, | ||
| }); | ||
| try { | ||
| await trigger.send({ key: 'value' }); | ||
| assert.fail('expected rejection'); | ||
| } catch (error) { | ||
| assert.strictEqual((error as CodedError).code, ErrorCode.HTTPError); | ||
| } | ||
| scope.done(); | ||
| }); | ||
|
|
||
| it('does not retry by default (no retryConfig)', async () => { | ||
| const scope = nock('https://hooks.slack.com') | ||
| .post(/triggers/) | ||
| .reply(503); | ||
| const trigger = new WebhookTrigger(url); | ||
| try { | ||
| await trigger.send({ key: 'value' }); | ||
| assert.fail('expected rejection'); | ||
| } catch (error) { | ||
| assert.strictEqual((error as CodedError).code, ErrorCode.HTTPError); | ||
| } | ||
| scope.done(); | ||
| }); | ||
| }); | ||
| }); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📝 note: The default options passed to requests isn't ideal practice IMHO but we avoid regressions with this line.