fix(events): never throw at module load, defer error to connect()#157
Merged
Conversation
Importing anything from `@openhands/typescript-client` transitively loads
`events/websocket-client.ts` (via `RemoteConversation`), and the same
file ran a `require('ws')` fallback at module-load time that threw
'WebSocket implementation not available. Install ws package for Node.js
environments.' whenever the require failed.
In a Node.js ESM consumer `require` is undefined, so the fallback always
threw. That meant agent-canvas could not even do:
import { RemoteWorkspace } from "@openhands/typescript-client";
without crashing, despite never opening a WebSocket. This is the
specific failure mode reported by the agent-canvas branch.
This change swaps the module-load throw for setting `WebSocketImpl =
undefined`. The 'no WebSocket implementation' condition is now reported
the first time `connect()` is called, through the existing `onError`
callback channel — the same way every other WebSocket connection error
is surfaced. Consumers that never open a WebSocket pay nothing.
The same fix is applied to `bash-websocket-client.ts`, which had the
same bug copied into it.
Adds `src/__tests__/package-import.test.ts` with regression coverage:
* importing each websocket module does not throw when `ws` is mocked
to throw on require;
* importing the package barrel does not throw and exposes
`RemoteWorkspace`, `Agent` and `RemoteConversation`;
* constructing `RemoteWorkspace` with no `ws` available does not
throw;
* `WebSocketCallbackClient.start()` and `BashWebSocketClient.start()`
surface the missing-implementation error via `onError` instead of
throwing.
The tests use `jest.isolateModules` + `jest.doMock('ws')` because Jest's
default runner is CommonJS, so a plain `require('ws')` succeeds in
tests — which is why this regression slipped past CI in the first place.
Co-authored-by: openhands <openhands@all-hands.dev>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Importing anything from
@openhands/typescript-clienttransitively loadsevents/websocket-client.ts(viaRemoteConversationre-exported from the barrel). That file runs arequire('ws')fallback at module-load time and throwswhenever the require fails. In a Node.js ESM consumer
requireis undefined, so the fallback always throws — meaning a consumer like agent-canvas can't even dowithout crashing, despite never opening a WebSocket. The same buggy block was copied into
bash-websocket-client.ts.Fix
Stop throwing at module load. Set
WebSocketImpl = undefinedin the catch instead, and surface the missing-implementation error through the existingonErrorcallback the first timeconnect()is actually called — exactly the same channel every other WebSocket connection error already flows through. Consumers that never open a WebSocket pay nothing.This is the minimum surgical change needed to make the package importable; the
if (typeof window …) else require('ws')runtime detection itself is untouched.private connect(): void { try { + if (!WebSocketImpl) { + throw new Error( + 'WebSocket implementation not available. Install the `ws` package, ' + + 'or run in an environment with a global WebSocket constructor.' + ); + } // …The existing
try { … } catch (error) { this.reportError(...); if (this.shouldReconnect) this.scheduleReconnect(); }wrapper already handles this — it just was unreachable before because the module-load throw fired first.Regression test
New
src/__tests__/package-import.test.tscovers:wsis unavailable.require('../index')does not throw and exposesRemoteWorkspace,Agent,RemoteConversation— the exact import shape agent-canvas uses.RemoteWorkspacewith nowsavailable does not throw.WebSocketCallbackClient.start()andBashWebSocketClient.start()surface the missing-implementation error viaonErrorinstead of throwing.All tests use
jest.isolateModules+jest.doMock('ws', () => { throw … }). This is critical: Jest's default runner is CommonJS, so a plainrequire('ws')succeeds in tests — which is exactly why this regression slipped past CI for months even though every real Node.js ESM consumer crashed on import.Verified the tests fail on the pre-fix code (all 6 throw with the legacy
"Install ws package..."message) and pass after the fix. Full suite: 193/193.Why not also switch to
globalThis.WebSocket?Modern Node 22+, Bun and edge runtimes all expose
globalThis.WebSocketwith no need for thewspackage, and a future PR could pick that up via aglobalThis.WebSocketlookup. That's a behaviour change worth shipping separately; this PR is intentionally limited to "don't crash on import".This PR was created by an AI agent (OpenHands) on behalf of @rbren.