feat: migrate from axios to fetch#2591
Conversation
Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
zimeg
left a comment
There was a problem hiding this comment.
@WilliamBergamin LGTM! This is a fantastic enhancement 🪄 🎩 ✨
I'm leaving a few quibbles and thoughts of small improvements but am so excited for this PR 🏆
- Spacing: I ask for a linebreak after imports
- Documentation: A few inline variables weren't so clear to me at a glance and comments might be nice?
- Slashes: Prefixed methods with "/" might guard against absurd case but brings peace of mind 😉
Let's count this toward #2359 too and I have an open question now about removing the @slack/rtm-api package and might be curious about deprecation as well - seeing the code change is motivating!
| this.messageHandler = (event: Event) => { | ||
| if (!(event instanceof MessageEvent)) { | ||
| this.logger.warn(`Expected MessageEvent but received ${event.constructor.name} (type: ${event.type})`); | ||
| return; | ||
| } | ||
| const isBinary = typeof event.data !== 'string'; | ||
| this.options.client.emit('ws_message', event.data, isBinary); | ||
| }; | ||
| this.websocket.addEventListener('message', this.messageHandler); |
There was a problem hiding this comment.
🪬 praise: Solid patterns of the event parameter as a single argument!
| // Note that ws' `autoPong` option is true by default, so no need to respond to ping. | ||
| // see https://github.com/websockets/ws/blob/2aa0405a5e96754b296fef6bd6ebdfb2f11967fc/doc/ws.md#new-websocketaddress-protocols-options | ||
| // Subscribe to undici diagnostics_channel for WebSocket ping/pong frame events. | ||
| // These channels fire for ALL undici WebSocket instances, so we filter by matching instance. |
There was a problem hiding this comment.
🧠 praise: Ouch! Thanks for adding this note! Am I understanding that messages cross between processes?
There was a problem hiding this comment.
Yess thanks for pointing this out, I want to see if I can improve this logic slightly but I don't think I can 🤔 currently if you have more then one socket connection then all the ping messages will be received by the pingHandler then it is responsible for filtering relevant ping messages
| this.websocket?.ping(pingMessage); | ||
| if (!this.websocket) { | ||
| this.logger.error('WebSocket not available, skipping ping.'); | ||
| return; | ||
| } | ||
| ping(this.websocket, Buffer.from(pingMessage)); |
There was a problem hiding this comment.
🌪️ thought: Fascinating change of what's attached to this websocket!
| // const webClient = new WebClient(process.env.SLACK_BOT_TOKEN, { | ||
| // logLevel: LogLevel.DEBUG, | ||
| // clientOptions, | ||
| // fetch: (url, options) => fetch(url, { ...options, dispatcher }) |
There was a problem hiding this comment.
💡 praise: Big fan of how this can be reused!
| */ | ||
| private dispatcher?: SocketModeDispatcher; | ||
|
|
||
| private connectionResponse?: AppsConnectionsOpenResponse; |
There was a problem hiding this comment.
📚 quibble: A kind request for @jsdoc for what this goes toward might give context to later debugging!
| if (dispatcher && this.webClientOptions.fetch === undefined) { | ||
| this.webClientOptions.fetch = (url, init) => | ||
| undiciFetch(url, { ...init, dispatcher: dispatcher as Dispatcher } as RequestInit); | ||
| } |
There was a problem hiding this comment.
🌟 praise: Quite a delightful pattern to find with shared unidici defaults!
| */ | ||
| export interface SocketModeDispatcher { | ||
| // biome-ignore lint/suspicious/noExplicitAny: structural compatibility with any undici Dispatcher version | ||
| dispatch(options: any, handler: any): boolean; |
There was a problem hiding this comment.
📚 question: The expected options and handler values aren't clear to me and I'm wondering if documenting these is still useful alongside the "any" option?
There was a problem hiding this comment.
Great question maybe adding more @jsdoc make more sense 💯
This is a structural type, its main goal is to prevent our project from directly exposing undici types, this provides users with more flexibility when definingDispatchers and gives use more leeway when the time comes to bump undici version
Co-authored-by: Eden Zimbelman <eden.zimbelman@salesforce.com>
Co-authored-by: Eden Zimbelman <eden.zimbelman@salesforce.com>
Co-authored-by: Eden Zimbelman <eden.zimbelman@salesforce.com>
Co-authored-by: Ale Mercado <104795114+srtaalej@users.noreply.github.com>
Summary
These changes migrates this project
axiosto the native Fetch API — a major step toward v8.What changed
@slack/web-api— Removedaxios,form-data,is-electron, andis-stream. HTTP requests now useglobalThis.fetch(or a user-suppliedfetchfunction). The public API surface is simpler: a singlefetchoption replacesagent,tls,requestInterceptor, andadapter.@slack/webhook— Same migration:axiosremoved, replaced with nativefetch.@slack/socket-mode— Replaced thewsWebSocket library withundici's spec-compliant WHATWGWebSocket. A newdispatcheroption enables proxy and custom TLS configuration.undiciis declared as a peer dependency.Breaking changes
agent(http/https Agent)fetchoption with a custom dispatchertls(pfx, cert, ca, etc.)fetchoption with custom TLS configrequestInterceptorfetchfunctionadapterfetchattachOriginalToWebAPIRequestErrorhttpAgent(socket-mode)dispatcheroption (undiciDispatcher)WebAPIHTTPError.headerstype changed fromIncomingHttpHeaderstoRecord<string, string>Proxy configuration examples
Web API — proxy via custom
fetchSocket Mode — proxy via
dispatcherSocket Mode — separate proxy for HTTP vs WebSocket
Webhook — proxy via custom
fetchCustom
fetch— override for testing or instrumentationTesting
web-api,webhook,socket-mode)prod-server-integration-tests/)bolt-js#test-bolt-with-sdk-v8bolt-js-starter-template) works as expected with these changesfetchoverride, and advanced transport scenarios validated via dedicated test project:WilliamBergamin/test-slack-node-sdk-8Requirements