diff --git a/package-lock.json b/package-lock.json index 77f32ef..5ca7739 100644 --- a/package-lock.json +++ b/package-lock.json @@ -9,8 +9,7 @@ "version": "1.39.0", "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/package.json b/package.json index 6855e0f..10d5584 100644 --- a/package.json +++ b/package.json @@ -87,8 +87,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/src/__tests__/package-import.test.ts b/src/__tests__/package-import.test.ts index e01e18b..efa114e 100644 --- a/src/__tests__/package-import.test.ts +++ b/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/src/events/bash-websocket-client.ts b/src/events/bash-websocket-client.ts index 94aab77..5af7f83 100644 --- a/src/events/bash-websocket-client.ts +++ b/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/src/events/websocket-client.ts b/src/events/websocket-client.ts index 99be5ea..92692b4 100644 --- a/src/events/websocket-client.ts +++ b/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.