Repository navigation
fix(events): use the WebSocket global and drop the ws dependency #4799
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
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 |
|---|---|---|
|
|
@@ -4,31 +4,18 @@ | |
|
|
||
| import { Event, ConversationCallbackType } from '../types/base'; | ||
|
|
||
| // Use native WebSocket in browser, ws library in Node.js. | ||
| // Every runtime this package supports supplies a standards-compatible | ||
| // WebSocket global: browsers, and Node.js since 22.4. | ||
| // | ||
| // IMPORTANT: this block must never throw. It runs whenever this file is | ||
| // imported, and this file is transitively imported by the package barrel | ||
| // (via RemoteConversation), so any throw here crashes consumers that | ||
| // merely `import { RemoteWorkspace } from "@openhands/typescript-client"` | ||
| // even when they have no intent to open a WebSocket. The "no implementation | ||
| // available" condition is deferred to connect() time, where it is surfaced | ||
| // through the existing onError callback channel. | ||
| let WebSocketImpl: any; | ||
|
|
||
| if (typeof window !== 'undefined' && window.WebSocket) { | ||
| // Browser environment | ||
| WebSocketImpl = window.WebSocket; | ||
| } else { | ||
| // Node.js environment | ||
| try { | ||
| // eslint-disable-next-line @typescript-eslint/no-require-imports | ||
| const ws = require('ws'); | ||
| WebSocketImpl = ws; | ||
| } catch { | ||
| // Leave WebSocketImpl undefined; connect() reports the error via onError. | ||
| WebSocketImpl = undefined; | ||
| } | ||
| } | ||
| // IMPORTANT: this must never throw. It runs whenever this file is imported, | ||
| // and this file is transitively imported by the package barrel (via | ||
| // RemoteConversation), so any throw here crashes consumers that merely | ||
| // `import { RemoteWorkspace } from "@openhands/typescript-client"` even when | ||
| // they have no intent to open a WebSocket. Reading a missing global yields | ||
| // undefined rather than throwing, and the "no implementation available" | ||
| // condition stays deferred to connect() time, where it is surfaced through | ||
| // the existing onError callback channel. | ||
| const WebSocketImpl: typeof WebSocket | undefined = globalThis.WebSocket; | ||
|
Collaborator
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. Dropping the |
||
|
|
||
| /** | ||
| * Error callback type for reporting non-fatal errors. | ||
|
|
@@ -51,7 +38,7 @@ | |
| private callback: ConversationCallbackType; | ||
| private apiKey?: string; | ||
| private onError?: ErrorCallbackType; | ||
| private ws?: any; // WebSocket instance (browser or Node.js) | ||
|
Check warning on line 41 in clients/typescript/src/events/websocket-client.ts
|
||
| private reconnectDelay = 1000; | ||
| private maxReconnectDelay = 30000; | ||
| private currentDelay = 1000; | ||
|
|
@@ -111,7 +98,7 @@ | |
| this.currentDelay = this.reconnectDelay; | ||
| }; | ||
|
|
||
| this.ws.onmessage = (event: { data: any }) => { | ||
|
Check warning on line 101 in clients/typescript/src/events/websocket-client.ts
|
||
| try { | ||
| const message = typeof event.data === 'string' ? event.data : event.data.toString(); | ||
| const eventData: Event = JSON.parse(message); | ||
|
|
||
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.
This rewritten suite is Jest-based (
jest.isolateModules/jest.resetModules), butmainhas migrated the TS client to Vitest (#5075):jest.config.cjsno longer exists andnpm testrunsvitest. After rebasing, these cases will not run as written — and this file is one of the two conflicted paths againstmain. Please port them to Vitest (vi.resetModules+ dynamicimport/vi.spyOn-basedrequireinterception, matching the currentmainversion) while resolving the conflict.