-
Notifications
You must be signed in to change notification settings - Fork 40
refactor: remove websocket-polyfill #531
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,8 @@ | ||
| /** @type {import('ts-jest').JestConfigWithTsJest} */ | ||
| module.exports = { | ||
| preset: 'ts-jest', | ||
| testEnvironment: 'node', | ||
| export default { | ||
| preset: "ts-jest", | ||
| testEnvironment: "node", | ||
| transform: { | ||
| "^.+\\.tsx?$": ["ts-jest", { useESM: true }], | ||
| }, | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| import "websocket-polyfill"; | ||
|
|
||
| import { NWCClient } from "./NWCClient"; | ||
|
|
||
| // this has no funds on it, I think ;-) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,7 @@ import { | |
| EventTemplate, | ||
| SimplePool, | ||
| } from "nostr-tools"; | ||
| import { normalizeURL } from "nostr-tools/utils"; | ||
| import { hexToBytes, bytesToHex } from "@noble/hashes/utils"; | ||
| import { | ||
| Nip47EncryptionType, | ||
|
|
@@ -55,7 +56,7 @@ import { | |
| Nip47CancelHoldInvoiceResponse, | ||
| Nip47NetworkError, | ||
| } from "./types"; | ||
| import { SubCloser } from "nostr-tools/lib/types/abstract-pool"; | ||
| import { SubCloser } from "nostr-tools/abstract-pool"; | ||
|
|
||
| export interface NWCOptions { | ||
| relayUrls: string[]; | ||
|
|
@@ -140,8 +141,8 @@ export class NWCClient { | |
| // this.subscribers = {}; | ||
|
|
||
| if (globalThis.WebSocket === undefined) { | ||
| console.error( | ||
| "WebSocket is undefined. Make sure to `import websocket-polyfill` for nodejs environments", | ||
| throw new Error( | ||
| "WebSocket is undefined. Make sure to polyfill it for nodejs environments", | ||
| ); | ||
| } | ||
| } | ||
|
|
@@ -198,8 +199,23 @@ export class NWCClient { | |
| return getEventHash(event); | ||
| } | ||
|
|
||
| close() { | ||
| return this.pool.close(this.relayUrls); | ||
|
|
||
| async close(): Promise<void> { | ||
|
Contributor
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. these changes are unrelated to this PR |
||
| 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); | ||
|
Comment on lines
+203
to
+218
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. 🧩 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:
💡 Result: In nostr-tools, the pool’s “relay close” API works like this:
Sources: nostr-tools README examples [1], 🏁 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:
💡 Result: In nostr-tools / Under the hood, Implication: Sources Citations:
Add timeout to WebSocket close promises and document reliance on private API. Two concerns with this
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 |
||
| } | ||
|
|
||
| async encrypt(pubkey: string, content: string) { | ||
|
|
||
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.
🧩 Analysis chain
🏁 Script executed:
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 viaextensionsToTreatAsEsm, otherwise Jest may still execute.tsas CJS semantics. [1][2]Minimal working pattern (no preset):
Key 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]TS_EXT_TO_TREAT_AS_ESM,ESM_TS_TRANSFORM_PATTERN) instead of hardcoding extensions/regex. [1].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
extensionsToTreatAsEsmto the Jest configuration.When using
ts-jestwithuseESM: true, you must also configureextensionsToTreatAsEsm: ['.ts']in the Jest config. Without it, Jest will still execute.tsfiles as CommonJS, creating a mismatch between ts-jest's ESM transformation and Jest's runtime expectations.Add to
jest.config.ts:🤖 Prompt for AI Agents