chore: provide proxied configurations in update to "@slack/web-api@v8" and "@slack/webhook@v8" packages#619
chore: provide proxied configurations in update to "@slack/web-api@v8" and "@slack/webhook@v8" packages#619zimeg wants to merge 19 commits into
Conversation
Migrate from axios/https-proxy-agent to native fetch with undici ProxyAgent, supporting the @slack/web-api v8 release candidate (slackapi/node-slack-sdk#2603). Breaking changes addressed: - Replace `agent` option with `fetch` option for WebClient proxy support - Replace axios with native fetch + undici ProxyAgent for webhook requests - Replace axios-retry with built-in retry logic for webhook requests - Update error handling to use v8 Error subclasses Dependencies removed: axios, axios-retry, https-proxy-agent Dependencies added: undici (for ProxyAgent) Dependencies upgraded: @slack/web-api ^7 → ^8.0.0-rc.1, @slack/logger ^4 → ^5.0.0-rc.1 Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
🦋 Changeset detectedLatest commit: e589900 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
Cast ProxyAgent to `any` to avoid type mismatch between undici's Dispatcher and undici-types' Dispatcher in Node's fetch signature. Initialize userAgent property to satisfy strict property checks. Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #619 +/- ##
==========================================
- Coverage 99.86% 99.86% -0.01%
==========================================
Files 7 8 +1
Lines 730 725 -5
==========================================
- Hits 729 724 -5
Misses 1 1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Tests must explicitly configure mocks.fetch for their specific scenario rather than relying on a hidden default response. Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
zimeg
left a comment
There was a problem hiding this comment.
🪄 A comment about webhook implementations while we explore changes-
Replace the manual fetch-with-retry implementation with IncomingWebhook from @slack/webhook v8 RC. The SDK handles HTTP internally while we inject a custom fetch wrapper for User-Agent and proxy support. Note: @slack/webhook v8 does not export addAppMetadata, so there is no public way to register custom app metadata in the SDK's User-Agent. We supplement by prepending our action identity in the custom fetch wrapper. Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
Use @slack/webhook IncomingWebhook only for the incoming-webhook type. Webhook triggers use a direct fetch since the SDK has no class for them and the response shape differs (JSON vs plain text). Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
# Conflicts: # package-lock.json
# Conflicts: # package-lock.json # package.json
WilliamBergamin
left a comment
There was a problem hiding this comment.
Nice work 💯 I'm a fan of how this is taking shape 🥇
Only true feedback I have is around the proxies.js file it really seems like it could be some sort of builder or factory pattern 🤔
| switch (new URL(proxy).protocol) { | ||
| case "https:": | ||
| case "http:": | ||
| return new ProxyAgent(proxy); |
There was a problem hiding this comment.
I'm a fan of this dispatcher approach 💯
| return undefined; | ||
| } | ||
| return (url, init) => globalThis.fetch(url, { ...init, dispatcher }); | ||
| } |
There was a problem hiding this comment.
This function seems like it wants to be some sort of factory method pattern but it isn't because it returns undefined 🤔
What do you think about renaming and changing it to something like
export function buildFetch(config, destination) {
const dispatcher = proxies(config, destination);
if (!dispatcher) {
return (url, init) => globalThis.fetch(url, { ...init });
}
return (url, init) => globalThis.fetch(url, { ...init, dispatcher });
}We could also fully embrace the factory pattern and name this file FetchFactory or FetchBuilder but this might not align with the rest of the project 🤔
There was a problem hiding this comment.
@WilliamBergamin I poked into updating each case with a meaningful return but think keeping undefined is easiest in fallbacks to the SDK implementation for now!
These are curious patterns shared - and new to me - but for now I want to hold off on extending this part of the implementation 🎁
| * @returns {import("axios").AxiosRequestConfig | undefined} | ||
| * @see {@link https://github.com/slackapi/slack-github-action/pull/205} | ||
| */ | ||
| proxies(config) { |
There was a problem hiding this comment.
Praise for drying things up 🙏
Replace the hand-written fetch signature with typeof globalThis.fetch so the return type stays sourced from the lib types. The precise type surfaces a ProxyAgent/Dispatcher mismatch from the duplicate undici type packages, so cast the dispatcher field to keep proxying working. Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
@types/node pulls undici-types transitively at a version that trails the undici we depend on directly, so their Dispatcher types are nominally distinct and passing a ProxyAgent to globalThis.fetch failed to type check. Override undici-types to the matching 7.28 line — its .d.ts is byte-identical to the runtime undici's bundled types — so the dispatcher field type checks without the any cast. Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
Bump the declared undici range to match the version already resolved in the lockfile and the undici-types override, so the dependency range, the lockfile, and the override all point at the same 7.28 line. Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
|
[DRAFT — do not submit yet] Why the
Ruled out the alternatives:
Maintenance note: the override is a literal |
Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
Summary
This pull request upgrades to the
@slack/web-apiand@slack/webhookv8 release candidates (slackapi/node-slack-sdk#2603), maintaining full proxy support by migrating from axios/https-proxy-agent to nativefetchwith the undiciProxyAgent.Changes:
@slack/web-apiv8WebClient, using itsfetchoption instead of the removedagentoption@slack/webhookv8IncomingWebhookandWebhookTriggerclients, using theirfetchoption andretryConfigretry policiesproxiedFetchfactory (src/proxies.js) that returns a customfetchbacked by an undiciProxyAgentdispatcher — injected into both the API and webhook clients so proxying stays opt-in and testable without any global dispatcher stateaddAppMetadataexports (rc.2 restoresaddAppMetadata,WebhookTrigger, andretryPolicies), so the SDK sets theUser-AgentinternallyErrorsubclasses (WebAPIPlatformError,WebAPIHTTPError, etc.)Dependencies removed:
axios,axios-retry,https-proxy-agentDependencies added:
undici(forProxyAgent)Dependencies upgraded:
@slack/web-api^7 → ^8.0.0-rc.2,@slack/webhook^7 → ^8.0.0-rc.2,@slack/logger^4 → ^5.0.0-rc.1The
proxyinput andHTTPS_PROXYenvironment variable continue to work unchanged for end users.Requirements