refactor: remove websocket-polyfill#531
Conversation
The close() method in NWCClient was synchronous, which meant it didn't wait for WebSocket connections to actually close before returning. This caused Jest tests to hang because of open handles. I made close() async so it waits for the underlying WebSocket capabilities to finish closing. I also updated the dependencies and config to support the latest nostr-tools exports. Changes: - Made NWCClient.close() async - Updated imports to use specific paths from nostr-tools - Set moduleResolution to bundler in tsconfig - Updated jest.config.ts to export default
fix: NWCClient.close() hanging Jest tests
📝 WalkthroughWalkthroughRemoves the websocket-polyfill dependency from the codebase, updating documentation and example files to reflect this change. Updates import paths for nostr-tools modules, changes error handling to throw instead of logging when WebSocket is absent, updates Jest configuration to ES modules, and modifies tsconfig moduleResolution setting. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
- Removes websocket-polyfill dependency - Removes imports from tests and examples - Updates documentation to remove installation instructions - Updates error messages to be generic for missing WebSocket
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
| close() { | ||
| return this.pool.close(this.relayUrls); | ||
|
|
||
| async close(): Promise<void> { |
There was a problem hiding this comment.
these changes are unrelated to this PR
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@jest.config.ts`:
- Around line 2-7: The Jest config is missing extensionsToTreatAsEsm which
causes a mismatch when using ts-jest with useESM: true; update the exported
config object to include extensionsToTreatAsEsm: ['.ts'] alongside
preset/testEnvironment and the transform entry so Jest treats .ts files as ESM
(modify the object containing transform and the ts-jest { useESM: true } option
to add the extensionsToTreatAsEsm key).
In `@src/nwc/NWCClient.ts`:
- Around line 203-218: The close() method currently reaches into SimplePool
internals via (this.pool as any).relays to await relay.ws "close" events but
doesn’t guard against missing events; update close() to wrap each close promise
with a timeout (e.g., reject/resolve after configurable ms) so
Promise.all(closePromises) cannot hang indefinitely, and ensure the timeout
value is configurable/constant. Also add an inline comment above the use of
(this.pool as any).relays documenting that this is a deliberate workaround for
nostr-tools' synchronous pool.close() (since pool.close(relayUrls) only
initiates closes and does not await them) and mark reliance on the private API
as fragile so future maintainers know why it’s used. Ensure references: close(),
relayUrls, this.pool/(SimplePool).relays, closePromises, and WebSocket close
event are updated accordingly.
🧹 Nitpick comments (3)
package.json (1)
94-96: Consider updatingenginesto reflect actual minimum Node.js requirements.The
enginesfield still specifies"node": ">=14", but the SDK already requiresfetch(Node ≥ 18) and now expects a globalWebSocket(native in Node ≥ 22, or user-provided polyfill). This could mislead users on older Node versions. Consider bumping to>=18at minimum.README.md (1)
86-87: Minor text clarity suggestion.The current phrasing "We recommend
websocket-polyfillbut checking your node version is preferred" reads a bit ambiguously. Consider rephrasing for clarity:📝 Suggested wording
#### WebSocket Polyfill -If you are using Node.js < v22, you may need a WebSocket polyfill. We recommend `websocket-polyfill` but checking your node version is preferred. +If you are using Node.js < v22, you may need a WebSocket polyfill such as `websocket-polyfill`. Consider upgrading to Node.js v22+ where WebSocket is available natively.src/nwc/NWCWalletService.ts (1)
60-66: Constructor createsRelaybefore the WebSocket check — consider reordering.
new Relay(this.relayUrl)on line 60 is called before verifyingglobalThis.WebSocketon line 62. If theRelayconstructor internally referencesWebSocket(e.g., for validation or eager setup), this could throw a less informative error before your guard is reached. Swapping the order ensures the developer-friendly message is always shown first.Suggested reorder
constructor(options: NewNWCWalletServiceOptions) { this.relayUrl = options.relayUrl; + if (globalThis.WebSocket === undefined) { + throw new Error( + "WebSocket is undefined. Make sure to polyfill it for nodejs environments", + ); + } + this.relay = new Relay(this.relayUrl); - if (globalThis.WebSocket === undefined) { - throw new Error( - "WebSocket is undefined. Make sure to polyfill it for nodejs environments", - ); - } }
| export default { | ||
| preset: "ts-jest", | ||
| testEnvironment: "node", | ||
| transform: { | ||
| "^.+\\.tsx?$": ["ts-jest", { useESM: true }], | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check full jest config
echo "=== jest.config.ts ==="
cat jest.config.ts 2>/dev/null || echo "File not found"
echo ""
echo "=== package.json (test script and ts-jest version) ==="
cat package.json | grep -A5 '"test"' || echo "Not found"
echo ""
cat package.json | grep -A2 '"ts-jest"' || echo "Not found"
echo ""
echo "=== Searching for extensionsToTreatAsEsm ==="
rg -n "extensionsToTreatAsEsm" 2>/dev/null || echo "Not found"
echo ""
echo "=== Searching for experimental-vm-modules ==="
rg -n "experimental-vm-modules" 2>/dev/null || echo "Not found"Repository: getAlby/js-sdk
Length of output: 650
🌐 Web query:
ts-jest useESM configuration extensionsToTreatAsEsm requirements 2025
💡 Result:
When using ts-jest in ESM mode (useESM: true), you generally must also tell Jest to treat TypeScript files as native ESM via extensionsToTreatAsEsm, otherwise Jest may still execute .ts as CJS semantics. [1][2]
Minimal working pattern (no preset):
// jest.config.ts
import type { Config } from 'jest'
export default {
extensionsToTreatAsEsm: ['.ts'], // (often also '.tsx' if you use it)
transform: {
'^.+\\.tsx?$': ['ts-jest', { useESM: true }],
},
} satisfies ConfigKey requirements / notes:
extensionsToTreatAsEsmshould include.ts(and.tsxif applicable) when you want those files to run as ESM in Jest. [1][2]useESM: trueis the ts-jest switch that makes ts-jest emit/transform to ESM where possible; it’s separate from Jest’sextensionsToTreatAsEsmtoggle. [3][2]- If you use ts-jest’s ESM helpers, you can use their provided constants/patterns (e.g.,
TS_EXT_TO_TREAT_AS_ESM,ESM_TS_TRANSFORM_PATTERN) instead of hardcoding extensions/regex. [1] - For
.mts/.mjs-style setups, ts-jest documents additional requirements such as havingpackage.jsoninclude"type": "module", and sometimes a custom resolver for.mjs→.mtsresolution. [1]
Add extensionsToTreatAsEsm to the Jest configuration.
When using ts-jest with useESM: true, you must also configure extensionsToTreatAsEsm: ['.ts'] in the Jest config. Without it, Jest will still execute .ts files as CommonJS, creating a mismatch between ts-jest's ESM transformation and Jest's runtime expectations.
Add to jest.config.ts:
export default {
preset: "ts-jest",
testEnvironment: "node",
extensionsToTreatAsEsm: ['.ts'],
transform: {
"^.+\\.tsx?$": ["ts-jest", { useESM: true }],
},
};🤖 Prompt for AI Agents
In `@jest.config.ts` around lines 2 - 7, The Jest config is missing
extensionsToTreatAsEsm which causes a mismatch when using ts-jest with useESM:
true; update the exported config object to include extensionsToTreatAsEsm:
['.ts'] alongside preset/testEnvironment and the transform entry so Jest treats
.ts files as ESM (modify the object containing transform and the ts-jest {
useESM: true } option to add the extensionsToTreatAsEsm key).
| async close(): Promise<void> { | ||
| const closePromises: Promise<void>[] = []; | ||
| for (const url of this.relayUrls) { | ||
| const relayUrl = normalizeURL(url); | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| const relay = (this.pool as any).relays.get(relayUrl); | ||
| if (relay?.ws && relay.ws.readyState !== WebSocket.CLOSED) { | ||
| closePromises.push( | ||
| new Promise<void>((resolve) => { | ||
| relay.ws?.addEventListener("close", () => resolve(), { once: true }); | ||
| }), | ||
| ); | ||
| } | ||
| } | ||
| this.pool.close(this.relayUrls); | ||
| await Promise.all(closePromises); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if SimplePool is from nostr-tools and understand its type
fd "pool" --type ts | head -20Repository: getAlby/js-sdk
Length of output: 227
🏁 Script executed:
# Search for SimplePool type definitions and usage in the codebase
rg "SimplePool" --type ts -B 2 -A 2Repository: getAlby/js-sdk
Length of output: 1430
🏁 Script executed:
# Check imports to understand where SimplePool comes from
rg "import.*SimplePool\|from.*nostr" --type ts | head -20Repository: getAlby/js-sdk
Length of output: 40
🏁 Script executed:
# Look for any type definitions or interfaces for pool
ast-grep --pattern 'interface.*Pool {
$$$
}' | head -50Repository: getAlby/js-sdk
Length of output: 40
🌐 Web query:
nostr-tools SimplePool public API relay close
💡 Result:
In nostr-tools, the pool’s “relay close” API works like this:
-
Close (disconnect) specific relay connections managed by the pool:
pool.close(relayUrls)closes only the relays whose URLs you pass in, and removes them from the pool’s internal relay map. Example:pool.close(['wss://relay.example.com'])
(Implementation: normalize URL →
relay.close()→ delete from map.) [2] -
Close a Relay directly (if you created/obtained a Relay instance):
The README shows calling:relay.close()
to close that relay’s websocket connection. [1]
-
Close all relays in the pool:
pool.close([])closes none (empty list). If you want to disconnect everything, use:pool.destroy()
which iterates all connected relays, closes them, and resets the pool’s relay map. [2]
(The README snippet that callspool.close([])is just passing an empty list—i.e., it won’t disconnect relays.) [1]
Sources: nostr-tools README examples [1], AbstractSimplePool implementation (close, destroy) [2].
🏁 Script executed:
rg "pool\.close|relay\.close" --type=typescript -B 2 -A 5 | head -40Repository: getAlby/js-sdk
Length of output: 92
🏁 Script executed:
# Look for how close() is typically used elsewhere in codebase
rg "\.close\(" --type=typescript | grep -E "(pool|relay)" | head -20Repository: getAlby/js-sdk
Length of output: 92
🌐 Web query:
nostr-tools SimplePool close method async await behavior
💡 Result:
In nostr-tools / @nostr/tools, SimplePool.close(relays) is not async and returns void, so await pool.close(...) does not wait for anything. The implementation just normalizes each URL, calls the cached relay’s .close(), and removes it from the pool’s internal map. [1]
Under the hood, relay.close() is also synchronous: it marks the relay as intentionally closed, clears reconnect/ping timers, closes all subscriptions, flips connected to false, calls the relay’s onclose callback, and then initiates the WebSocket close (ws.close()) if the socket is open—without returning a Promise you can await. [2]
Implication: pool.close() only starts shutdown; it doesn’t provide an “await until sockets are fully closed” mechanism.
If you need an awaitable close in your code, you’ll have to build it yourself (e.g., track the WebSocket close event for each relay connection you created).
Sources
[1] AbstractSimplePool.close(relays): void implementation (JSR, abstract-pool.ts) (jsr.io)
[2] AbstractRelay.close(): void implementation (JSR, abstract-relay.ts) (jsr.io)
Citations:
- 1: https://jsr.io/%40nostr/tools/2.23.1/abstract-pool.ts
- 2: https://jsr.io/%40nostr/tools/2.19.4/abstract-relay.ts
Add timeout to WebSocket close promises and document reliance on private API.
Two concerns with this close() implementation:
-
Accessing private internals:
(this.pool as any).relaysaccessesSimplePool's internalMap. Whilenostr-toolsexposespool.close(relayUrls)as a public API, it is synchronous and does not wait for WebSocket close events—it only initiates them. Your manual event tracking is necessary to ensure connections fully close, but this fragile reliance on private API will break silently if the field is renamed or restructured. Document this as a workaround for nostr-tools' lack of awaitable close. -
No timeout on close promises: If a WebSocket
closeevent never fires (e.g., network stall),await Promise.all(closePromises)hangs indefinitely. Add a timeout to each close promise:
Suggested timeout wrapper
if (relay?.ws && relay.ws.readyState !== WebSocket.CLOSED) {
closePromises.push(
- new Promise<void>((resolve) => {
- relay.ws?.addEventListener("close", () => resolve(), { once: true });
- }),
+ new Promise<void>((resolve) => {
+ const timer = setTimeout(resolve, 5000);
+ relay.ws?.addEventListener("close", () => { clearTimeout(timer); resolve(); }, { once: true });
+ }),
);
}🤖 Prompt for AI Agents
In `@src/nwc/NWCClient.ts` around lines 203 - 218, The close() method currently
reaches into SimplePool internals via (this.pool as any).relays to await
relay.ws "close" events but doesn’t guard against missing events; update close()
to wrap each close promise with a timeout (e.g., reject/resolve after
configurable ms) so Promise.all(closePromises) cannot hang indefinitely, and
ensure the timeout value is configurable/constant. Also add an inline comment
above the use of (this.pool as any).relays documenting that this is a deliberate
workaround for nostr-tools' synchronous pool.close() (since
pool.close(relayUrls) only initiates closes and does not await them) and mark
reliance on the private API as fragile so future maintainers know why it’s used.
Ensure references: close(), relayUrls, this.pool/(SimplePool).relays,
closePromises, and WebSocket close event are updated accordingly.
2 participants
Closes #519
websocket-polyfill is only needed for NodeJS < 22, and it incorrectly overwrites WebSocket even if it already exists on the global object.
This PR:
Summary by CodeRabbit
Chores
Bug Fixes