From d4e934ed8f65baee0ba451c086ebf6208835c14e Mon Sep 17 00:00:00 2001 From: george larson Date: Mon, 31 Aug 2026 13:34:45 -0400 Subject: [PATCH] fix(events): use the WebSocket global and drop the ws dependency MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The package is emitted as ESM, and Node 22 provides a standards-compatible globalThis.WebSocket. The conversation and bash event clients only checked window.WebSocket, then attempted require('ws'), which is unavailable in ESM — so starting either client in a Node ESM process reported "WebSocket implementation not available" even though the global exists. Every runtime this package supports supplies the global (browsers, and Node.js since 22.4), so read globalThis.WebSocket directly and drop the ws dependency. The "no implementation" condition stays deferred to connect() time and is still surfaced through the existing onError callback, so importing the package barrel never throws. Co-authored-by: openhands --- clients/typescript/package-lock.json | 24 +-- clients/typescript/package.json | 3 +- .../src/__tests__/package-import.test.ts | 145 +++++++++++------- .../src/events/bash-websocket-client.ts | 22 +-- .../typescript/src/events/websocket-client.ts | 35 ++--- 5 files changed, 105 insertions(+), 124 deletions(-) diff --git a/clients/typescript/package-lock.json b/clients/typescript/package-lock.json index ec1b4e12ef3..e14efdb5c73 100644 --- a/clients/typescript/package-lock.json +++ b/clients/typescript/package-lock.json @@ -9,8 +9,7 @@ "version": "1.44.1", "license": "MIT", "dependencies": { - "@openrouter/sdk": "^1.2.11", - "ws": "^8.20.0" + "@openrouter/sdk": "^1.2.11" }, "devDependencies": { "@babel/preset-env": "^7.29.5", @@ -7802,27 +7801,6 @@ "node": "^14.17.0 || ^16.13.0 || >=18.0.0" } }, - "node_modules/ws": { - "version": "8.21.2", - "resolved": "https://registry.npmjs.org/ws/-/ws-8.21.2.tgz", - "integrity": "sha512-54dMVAo4WIe6SKy3vBgN+9bJZqqQ8IMRevAkOLQALhi49qkkQDQfWdAZ8KQlXiEabw88ARXXdUrlvtbKQX+aKw==", - "license": "MIT", - "engines": { - "node": ">=10.0.0" - }, - "peerDependencies": { - "bufferutil": "^4.0.1", - "utf-8-validate": ">=5.0.2" - }, - "peerDependenciesMeta": { - "bufferutil": { - "optional": true - }, - "utf-8-validate": { - "optional": true - } - } - }, "node_modules/wsl-utils": { "version": "0.3.1", "resolved": "https://registry.npmjs.org/wsl-utils/-/wsl-utils-0.3.1.tgz", diff --git a/clients/typescript/package.json b/clients/typescript/package.json index 8f5dae7107a..e1d5e418718 100644 --- a/clients/typescript/package.json +++ b/clients/typescript/package.json @@ -86,8 +86,7 @@ "author": "OpenHands Team", "license": "MIT", "dependencies": { - "@openrouter/sdk": "^1.2.11", - "ws": "^8.20.0" + "@openrouter/sdk": "^1.2.11" }, "devDependencies": { "@babel/preset-env": "^7.29.5", diff --git a/clients/typescript/src/__tests__/package-import.test.ts b/clients/typescript/src/__tests__/package-import.test.ts index e01e18b911d..efa114ed86d 100644 --- a/clients/typescript/src/__tests__/package-import.test.ts +++ b/clients/typescript/src/__tests__/package-import.test.ts @@ -11,15 +11,15 @@ * * import { RemoteWorkspace } from "@openhands/typescript-client"; * - * These tests pin the new behaviour: the WebSocket modules never throw at - * module load, and the "no WebSocket implementation" condition is reported - * via the existing `onError` callback only when `start()` is called. + * The `ws` fallback is gone. Every runtime this package supports supplies a + * standards-compatible WebSocket global, so the clients read + * `globalThis.WebSocket` directly. * - * Implementation note: the default Jest runner is CommonJS, so plain - * `require('ws')` *succeeds* in tests and hides the bug. Each test below - * uses `jest.isolateModules` + `jest.doMock('ws', () => { throw ... })` so - * the module-load path is exercised against the same conditions a real - * Node.js ESM consumer hits. + * These tests pin two things: the WebSocket modules never throw at module + * load even when no implementation exists, and the "no WebSocket + * implementation" condition is reported via the existing `onError` callback + * only when `start()` is called. Deleting the global is the real condition a + * consumer would hit, rather than a simulation of it. */ const WEBSOCKET_MODULES = [ @@ -27,18 +27,24 @@ const WEBSOCKET_MODULES = [ '../events/bash-websocket-client', ] as const; -const mockWsAsUnavailable = (): void => { - jest.doMock('ws', () => { - throw new Error('ws is not available in this environment'); - }); +const originalWebSocket = globalThis.WebSocket; + +/** Remove the global before the module under test reads it at load time. */ +const removeWebSocketGlobal = (): void => { + globalThis.WebSocket = undefined as unknown as typeof WebSocket; }; -describe('package imports do not crash when `ws` is unavailable', () => { +afterEach(() => { + globalThis.WebSocket = originalWebSocket; + jest.resetModules(); +}); + +describe('package imports do not crash without a WebSocket implementation', () => { describe.each(WEBSOCKET_MODULES)('%s', (modulePath) => { it('does not throw at module load', () => { expect(() => { jest.isolateModules(() => { - mockWsAsUnavailable(); + removeWebSocketGlobal(); // eslint-disable-next-line @typescript-eslint/no-require-imports require(modulePath); }); @@ -46,13 +52,13 @@ describe('package imports do not crash when `ws` is unavailable', () => { }); }); - it('importing the package barrel does not throw when `ws` is unavailable', () => { + it('importing the package barrel does not throw', () => { // This is the exact failure agent-canvas hit: // import { RemoteWorkspace } from "@openhands/typescript-client"; // would crash because the barrel transitively loads the websocket modules. expect(() => { jest.isolateModules(() => { - mockWsAsUnavailable(); + removeWebSocketGlobal(); // eslint-disable-next-line @typescript-eslint/no-require-imports const pkg = require('../index'); // Touch a non-WebSocket export to make sure nothing is lazy in a way @@ -64,10 +70,10 @@ describe('package imports do not crash when `ws` is unavailable', () => { }).not.toThrow(); }); - it('constructing RemoteWorkspace does not require `ws`', () => { + it('constructing RemoteWorkspace does not need a WebSocket', () => { expect(() => { jest.isolateModules(() => { - mockWsAsUnavailable(); + removeWebSocketGlobal(); // eslint-disable-next-line @typescript-eslint/no-require-imports const { RemoteWorkspace } = require('../index'); new RemoteWorkspace({ @@ -79,54 +85,77 @@ describe('package imports do not crash when `ws` is unavailable', () => { }).not.toThrow(); }); - it('WebSocketCallbackClient.start() reports the missing implementation via onError instead of throwing', () => { - let captured: Error | undefined; + it.each([ + ['../events/websocket-client', 'WebSocketCallbackClient', { conversationId: 'conv-1' }], + ['../events/bash-websocket-client', 'BashWebSocketClient', {}], + ])( + '%s %s.start() reports the missing implementation via onError', + (modulePath, exportName, extra) => { + let captured: Error | undefined; - jest.isolateModules(() => { - mockWsAsUnavailable(); - // eslint-disable-next-line @typescript-eslint/no-require-imports - const { WebSocketCallbackClient } = require('../events/websocket-client'); - const client = new WebSocketCallbackClient({ - host: 'http://example.com', - conversationId: 'conv-1', - callback: () => {}, - onError: (err: Error) => { - captured = err; - }, + jest.isolateModules(() => { + removeWebSocketGlobal(); + // eslint-disable-next-line @typescript-eslint/no-require-imports + const module = require(modulePath); + const client = new module[exportName]({ + host: 'http://example.com', + callback: () => {}, + onError: (err: Error) => { + captured = err; + }, + ...(extra as Record), + }); + try { + client.start(); + } finally { + client.stop(); + } }); - try { - client.start(); - } finally { - client.stop(); - } - }); - expect(captured).toBeInstanceOf(Error); - expect(captured?.message).toMatch(/WebSocket implementation not available/i); - }); + expect(captured).toBeInstanceOf(Error); + expect(captured?.message).toMatch(/WebSocket implementation not available/i); + } + ); +}); - it('BashWebSocketClient.start() reports the missing implementation via onError instead of throwing', () => { - let captured: Error | undefined; +describe('the clients use globalThis.WebSocket when it exists', () => { + it.each([ + { + modulePath: '../events/websocket-client', + exportName: 'WebSocketCallbackClient', + options: { host: 'http://example.com', conversationId: 'conv-1', callback: () => {} }, + expectedUrl: 'ws://example.com/sockets/events/conv-1', + }, + { + modulePath: '../events/bash-websocket-client', + exportName: 'BashWebSocketClient', + options: { host: 'http://example.com', callback: () => {} }, + expectedUrl: 'ws://example.com/sockets/bash-events', + }, + ])('$exportName opens $expectedUrl', ({ modulePath, exportName, options, expectedUrl }) => { + const urls: string[] = []; + class FakeWebSocket { + onopen?: () => void; + onmessage?: (event: { data: unknown }) => void; + onclose?: () => void; + onerror?: () => void; + + constructor(url: string) { + urls.push(url); + } + + close(): void {} + } + globalThis.WebSocket = FakeWebSocket as unknown as typeof WebSocket; jest.isolateModules(() => { - mockWsAsUnavailable(); // eslint-disable-next-line @typescript-eslint/no-require-imports - const { BashWebSocketClient } = require('../events/bash-websocket-client'); - const client = new BashWebSocketClient({ - host: 'http://example.com', - callback: () => {}, - onError: (err: Error) => { - captured = err; - }, - }); - try { - client.start(); - } finally { - client.stop(); - } + const module = require(modulePath); + const client = new module[exportName](options); + client.start(); + client.stop(); }); - expect(captured).toBeInstanceOf(Error); - expect(captured?.message).toMatch(/WebSocket implementation not available/i); + expect(urls).toEqual([expectedUrl]); }); }); diff --git a/clients/typescript/src/events/bash-websocket-client.ts b/clients/typescript/src/events/bash-websocket-client.ts index 94aab776757..5af7f832cc1 100644 --- a/clients/typescript/src/events/bash-websocket-client.ts +++ b/clients/typescript/src/events/bash-websocket-client.ts @@ -5,23 +5,11 @@ import { BashEvent } from '../models/workspace'; import { ErrorCallbackType } from './websocket-client'; -// IMPORTANT: this block must never throw. See the matching note in -// `events/websocket-client.ts` — the "no WebSocket implementation" -// condition is deferred to connect() time so importing this module does -// not crash consumers that never use bash event streaming. -let WebSocketImpl: any; - -if (typeof window !== 'undefined' && window.WebSocket) { - WebSocketImpl = window.WebSocket; -} else { - try { - // eslint-disable-next-line @typescript-eslint/no-require-imports - const ws = require('ws'); - WebSocketImpl = ws; - } catch { - WebSocketImpl = undefined; - } -} +// See the matching note in `events/websocket-client.ts`: reading the global +// never throws, and the "no WebSocket implementation" condition stays +// deferred to connect() time so importing this module does not crash +// consumers that never use bash event streaming. +const WebSocketImpl: typeof WebSocket | undefined = globalThis.WebSocket; export interface BashWebSocketClientOptions { host: string; diff --git a/clients/typescript/src/events/websocket-client.ts b/clients/typescript/src/events/websocket-client.ts index 99be5ea0269..92692b4cabb 100644 --- a/clients/typescript/src/events/websocket-client.ts +++ b/clients/typescript/src/events/websocket-client.ts @@ -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; /** * Error callback type for reporting non-fatal errors.