Skip to content

Allow use of builtin Node WebSocket when available#26682

Open
sbc100 wants to merge 1 commit intoemscripten-core:mainfrom
sbc100:WebSocket_node
Open

Allow use of builtin Node WebSocket when available#26682
sbc100 wants to merge 1 commit intoemscripten-core:mainfrom
sbc100:WebSocket_node

Conversation

@sbc100
Copy link
Copy Markdown
Collaborator

@sbc100 sbc100 commented Apr 14, 2026

Node v21 and up have native WebSocket support. This change makes the import of the external ws module conditional so it will only run on older versions of node.

See #26676

@sbc100 sbc100 requested a review from kripken April 14, 2026 19:05
@sbc100
Copy link
Copy Markdown
Collaborator Author

sbc100 commented Apr 14, 2026

@jeroen

@sbc100 sbc100 changed the title Allow use of builtin node WebSocket when available. NFS Allow use of builtin node WebSocket when available Apr 14, 2026
@jeroen
Copy link
Copy Markdown

jeroen commented Apr 14, 2026

Actually Claude suggests we also need to update the event handlers, see the second half of changes in libsockfs.js here: https://github.com/jeroen/emscripten/pull/1/changes

Because peer.socket.on('open', handleOpen) only applies to the ws implementation we no longer condition that on if (ENVIRONMENT_IS_NODE) but instead on if (typeof peer.socket.on === 'function') .

@sbc100
Copy link
Copy Markdown
Collaborator Author

sbc100 commented Apr 14, 2026

Actually Claude suggests we also need to update the event handlers, see the second half of changes in libsockfs.js here: https://github.com/jeroen/emscripten/pull/1/changes

Because peer.socket.on('open', handleOpen) only applies to the ws implementation we no longer condition that on if (ENVIRONMENT_IS_NODE) but instead on if (typeof peer.socket.on === 'function') .

Good point. I decided to cleanup that code a bit first: #26683

@sbc100 sbc100 force-pushed the WebSocket_node branch 2 times, most recently from bca8eae to 7acdc56 Compare April 20, 2026 00:06
Node v21 and up have native WebSocket support.  This change makes the
import of the external `ws` module conditional so it will only run on
older versions of node.

See emscripten-core#26676
@sbc100
Copy link
Copy Markdown
Collaborator Author

sbc100 commented Apr 20, 2026

Done. PTAL

@sbc100 sbc100 changed the title Allow use of builtin node WebSocket when available Allow use of builtin Node WebSocket when available Apr 20, 2026
@sbc100 sbc100 requested a review from brendandahl April 20, 2026 20:41
@sbc100
Copy link
Copy Markdown
Collaborator Author

sbc100 commented Apr 20, 2026

Looks like we cannot switch just yet because node is sending the subprocols headers in the format of foo, bar but websockify doesn't handle the space after the comma just yet. See novnc/websockify#632

@jeroen
Copy link
Copy Markdown

jeroen commented Apr 21, 2026

@sbc100 should we maybe fix this in nodejs to behave the same as in browsers? There are many different websockify implementations and it may take a long time for this fix to end up on server distributions.

@sbc100
Copy link
Copy Markdown
Collaborator Author

sbc100 commented Apr 21, 2026

Are you suggesting an upstream node fix or some kind of monkey patching at runtime?

Also, are you sure of the browser behavour? I didn't confirm yet

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants