Skip to content

refactor: send webhooks with the @slack/webhook package#630

Merged
zimeg merged 49 commits into
mainfrom
webhook-trigger-sdk
Jul 10, 2026
Merged

refactor: send webhooks with the @slack/webhook package#630
zimeg merged 49 commits into
mainfrom
webhook-trigger-sdk

Conversation

@zimeg

@zimeg zimeg commented Jun 30, 2026

Copy link
Copy Markdown
Member

Summary

Converts both webhook techniques to the @slack/webhook SDK and removes axios / axios-retry as direct dependencies of the action:

  • incoming-webhookIncomingWebhook class
  • webhook-triggerWebhookTrigger class

Retry behavior (0 / 5 / 10 / RAPID) and HTTPS proxy support are preserved, so this is not a regression. Retries are handled inside the SDK: the selected policy is passed as the retryConfig option to the IncomingWebhook / WebhookTrigger client, using @slack/webhook's own retryPolicies (fiveRetriesInFiveMinutes, tenRetriesInAboutThirtyMinutes, rapidRetryPolicy), with fiveRetriesInFiveMinutes as the default. The action no longer runs its own p-retry loop.

src/config.js no longer owns an axios client or builds the webhook User-Agent — the SDKs set their own. The action still registers app metadata with @slack/web-api.

Notes

  • @slack/webhook is pinned to the published ^7.2.0 release (which exports WebhookTrigger).
  • A patch changeset is included, so this releases as 3.0.4.
  • No @slack/web-api version bump in this PR; client.js (the API-method technique) is unchanged and still uses @slack/web-api@7 (axios transitively).
  • dist/ is rebuilt as part of release, not committed here.

Requirements

🤖 Generated with Claude Code

zimeg and others added 6 commits June 30, 2026 04:02
Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
@zimeg zimeg added enhancement New feature or request javascript Pull requests that update Javascript code semver:minor tests labels Jun 30, 2026
@changeset-bot

changeset-bot Bot commented Jun 30, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 197f045

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@slack/slack-github-action Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Retries now live in the @slack/webhook package via retryConfig, matching
how the API-method technique passes retryConfig to @slack/web-api. The
action maps its 0/5/10/RAPID input to the SDK's exported retry policies.

Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
@zimeg

zimeg commented Jul 2, 2026

Copy link
Copy Markdown
Member Author

Retries now live upstream in @slack/webhook (node-slack-sdk branch eden/webhook-retries, off the WWFB eden/webhook-trigger WIP) rather than in the action.

  • Both IncomingWebhook and WebhookTrigger now accept a retryConfig?: RetryOptions option (mirroring @slack/web-api's WebClient), defaulting to no retries (opt-in).
  • The webhook package re-exports the same named policies (fiveRetriesInFiveMinutes, tenRetriesInAboutThirtyMinutes, rapidRetryPolicy).
  • Retry conditions match transient failures: 429 + 5xx retry, other 4xx abort, network errors retry.

This PR now just passes retryConfig through to the SDK constructors and maps 0/5/10/RAPID to the exported policies — no action-side p-retry wrapper. Pre-merge gate is unchanged: swap the file: link for a published @slack/webhook that includes these retry policies.

Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
zimeg and others added 7 commits July 6, 2026 21:49
The action already registered its app metadata with @slack/web-api via
addAppMetadata, but webhook requests went out without the action's
identifier. @slack/webhook now exports the same addAppMetadata API, so
register the package name and version with it too using the shared
package.json values. Adds a test covering the webhook registration.

Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
The SDK migration collapsed proxies() to pass any proxy URL straight to
HttpsProxyAgent, dropping the http:/https: protocol check that main had.
Restore the switch so an unsupported proxy protocol throws the same
'Unsupported URL protocol' error instead of leaking a raw agent failure.
Both http: and https: proxies map to a single HttpsProxyAgent; the old
axios-only 'proxy: false' branch is unneeded since @slack/webhook sets
proxy: false on its own axios instance.

Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
Collapse postIncomingWebhook/postWebhookTrigger into a single post():
construct each client through the config.webhook module namespace, send,
and write outputs inline within the type switch, sharing the options and
one try/catch. Drops the direct IncomingWebhook/WebhookTrigger imports
and the redundant webhook URL type cast.

Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
The SDK migration (074e58f) dropped two behaviors main had: the guard
that throws when no webhook is provided, and the skip that returns
undefined (no proxy) when the webhook destination is not HTTPS. Restore
both for parity — an HttpsProxyAgent only tunnels HTTPS destinations, so
a non-HTTPS webhook should bypass the proxy. Adds a test for the skip.

Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
Restore the @see link to #132 alongside #205 so both PRs that shaped the
proxy behavior are documented on proxies().

Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
Each case is the last action in post(), so return makes the intent
explicit and guards against fall-through if code is added later.

Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
Move the non-HTTPS webhook skip test back to its position after the
missing-webhook case and restore its original name, matching main's
proxies ordering.

Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
Place "sets up the proxy agent for the provided https proxy" ahead of
the returns-undefined case to match main's proxies ordering.

Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
Add a test that an http proxy also yields an HttpsProxyAgent (beside the
https case) and one asserting post() throws for an unknown webhook type,
bringing src/webhook.js to full branch coverage.

Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
Use a bare assert.fail() in the unknown webhook type test.

Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
zimeg and others added 5 commits July 7, 2026 13:26
Bump to @slack/webhook 7.2.0-rc.2 and consume retry policies via the
new retryPolicies namespace (webhook.retryPolicies.<policy>), mirroring
the @slack/web-api client. Updates src/webhook.js and the retries tests.

Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
Bump to @slack/webhook 7.2.0-rc.3, whose WebhookTriggerResult is
{ ok, error? } with no body field. Stringify the entire response for
the "response" output instead of the now-undefined response.body, and
drop the fabricated body from the trigger test mocks.

Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
Pin to the stable 7.2.0 now that it is published; same API surface as
7.2.0-rc.3 (retryPolicies export, { ok, error? } trigger result), so no
code changes are needed.

Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
Widen the @slack/webhook pin from an exact 7.2.0 to a ^7.2.0 caret
range so compatible patch and minor releases are picked up.

Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>

@zimeg zimeg left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🗣️ Comments on these changes but an RC is planned to confirm proxies deliver as expected next!

Comment thread test/config.spec.js
@@ -185,18 +186,30 @@
});

it("adds metadata to webhook with package name and version", async () => {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

👁️‍🗨️ note: This now matches the patterns of @slack/web-api tests for app metadata above!

Comment thread test/config.spec.js
Comment on lines 228 to 241

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🔭 note: Our responses our now returned to match how @slack/webhook responses appear!

Comment thread test/webhook.spec.js
const [url, payload, options] = mocks.axios.post.getCall(0).args;
assert.equal(url, "https://hooks.slack.com");
assert.deepEqual(payload, { drinks: "coffee" });
assert.deepEqual(options, {});

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🧪 note: We don't have a simple scheme to test options are passed to the SDK without revising our approach to tests altogether toward a mock HTTP server. I'm willing to follow up with improvements but don't plan to explore this immediate unless it's considered a blocker?

Comment thread test/webhook.spec.js
const webhook = new Webhook();
const result = webhook.retries();
assert.equal(result.retries, 5);
const result = new Webhook().retries();

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

📝 note: An inlined result is created here to avoid name collisions with the webhook import!

Comment thread test/webhook.spec.js
});

it("returns the failures from a webhook trigger", async () => {
mocks.core.getBooleanInput.withArgs("errors").returns(true);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

⚠️ note: This was missed in earlier iterations of the test so asserts for errors that followed weren't exercised.

Comment thread src/webhook.js
url,
options,
).send(config.content.values);
config.core.setOutput("ok", true);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🧪 todo: Will confirm this throws for 404 instead of falls through!

Comment thread src/webhook.js
},
);
config.core.setOutput("ok", response.status === 200);
config.core.setOutput("response", JSON.stringify(response.data));

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🧪 todo: Will confirm response shapes continue to match after this change!

Comment thread package.json
Comment on lines -49 to -50
"axios": "^1.18.1",
"axios-retry": "^4.5.0",

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🫡 praise: Thank you axios and axios-retry for sending with confidence!

zimeg and others added 2 commits July 9, 2026 12:52

@WilliamBergamin WilliamBergamin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is looking good 💯

I only have green light feedback 🟢

Comment thread test/index.spec.js
Comment on lines +51 to +54
this.webhook = {
incoming: this.sandbox.stub(webhook.IncomingWebhook.prototype, "send"),
trigger: this.sandbox.stub(webhook.WebhookTrigger.prototype, "send"),
};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Praise 📜

Comment thread test/webhook.spec.js
assert.deepEqual(mocks.webhook.trigger.getCall(0).firstArg, {
drinks: "coffee",
});
assert.ok(mocks.core.setFailed.called);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Love this 🚀

@zimeg

zimeg commented Jul 10, 2026

Copy link
Copy Markdown
Member Author

@WilliamBergamin Much appreciated! Thanks for all of the reviews toward this! Let's now merge these changes 🚢 💨

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dependencies Pull requests that update a dependency file enhancement New feature or request semver:patch tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants