fix: use web standard event apis for twilio websocket #127
Conversation
the twilio websocket extension used the node `ws` specific apis for attaching event handlers, which made it throw in a cloudflare environment. this patch adds a fix to the types, and uses `.addEventListener` on the websocket instead of `.on`. this also adds a fix to load the right shim for workerd when using `@openai/agents-realtime`
🦋 Changeset detectedLatest commit: cc04a29 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
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 |
| @@ -7,7 +7,13 @@ import { | |||
| RealtimeSessionConfig, | |||
There was a problem hiding this comment.
My IDE autoformatted this file, but the changes are minimal, pointed out below
| import type { | ||
| WebSocket as NodeWebSocket, | ||
| MessageEvent as NodeMessageEvent, | ||
| ErrorEvent as NodeErrorEvent, | ||
| } from 'ws'; | ||
|
|
||
| import type { ErrorEvent } from 'undici-types'; |
There was a problem hiding this comment.
import some types
| @@ -48,7 +54,7 @@ export type TwilioRealtimeTransportLayerOptions = | |||
| * ``` | |||
| */ | |||
| export class TwilioRealtimeTransportLayer extends OpenAIRealtimeWebSocket { | |||
| #twilioWebSocket: WebSocket; | |||
| #twilioWebSocket: WebSocket | NodeWebSocket; | |||
There was a problem hiding this comment.
change this type here to accept ws or a standard WebSocket
| 'Invalid mark name received:', | ||
| data.mark.name, | ||
| ); | ||
| this.#twilioWebSocket.addEventListener( |
There was a problem hiding this comment.
change this to use .addEventListener
| 'message', | ||
| (message: MessageEvent | NodeMessageEvent) => { | ||
| try { | ||
| const data = JSON.parse(message.data.toString()); |
There was a problem hiding this comment.
use message.data here
| ); | ||
| }, | ||
| ); | ||
| this.#twilioWebSocket.addEventListener('close', () => { |
There was a problem hiding this comment.
change this to .addEventListener
| this.close(); | ||
| } | ||
| }); | ||
| this.#twilioWebSocket.addEventListener( |
There was a problem hiding this comment.
change this to addEventListener
| @@ -18,7 +24,7 @@ export type TwilioRealtimeTransportLayerOptions = | |||
| * The websocket that is receiving messages from Twilio's Media Streams API. Typically the | |||
| * connection gets passed into your request handler when running your WebSocket server. | |||
| */ | |||
| twilioWebSocket: WebSocket; | |||
| twilioWebSocket: WebSocket | NodeWebSocket; | |||
There was a problem hiding this comment.
accept the websocket type
|
whoops, missed one failing test, should be fixed now |
|
actually, don't land this just yet, I think there might be some quirks to handle |
| @@ -149,7 +150,8 @@ export class OpenAIRealtimeWebSocket | |||
| ); | |||
| } | |||
|
|
|||
| const websocketArguments = isBrowserEnvironment() | |||
| // browsers and workerd should use the protocols argument, node should use the headers argument | |||
There was a problem hiding this comment.
workerd's WebSocket constructor is "standards" based, so can't pass headers
| export function isBrowserEnvironment(): boolean { | ||
| return false; | ||
| } | ||
| export const useWebSocketProtocols = true; |
There was a problem hiding this comment.
open to some other name for this, but this seemed clear
|
ok, now it should be good to review and/or land. |
the twilio websocket extension used the node
wsspecific apis for attaching event handlers, which made it throw in a cloudflare environment. this patch adds a fix to the types, and uses.addEventListeneron the websocket instead of.on. this also adds a fix to load the right shim for workerd when using@openai/agents-realtime