fix(websockets): send WsException errors to native WebSocket clients#16366
Open
veeceey wants to merge 4 commits intonestjs:v12.0.0from
Open
fix(websockets): send WsException errors to native WebSocket clients#16366veeceey wants to merge 4 commits intonestjs:v12.0.0from
veeceey wants to merge 4 commits intonestjs:v12.0.0from
Conversation
Pull Request Test Coverage Report for Build e10f6819-b1b4-4f6a-81e5-857fe08aabe0Details
💛 - Coveralls |
Member
|
Could you rebase to v12.0.0? |
15 tasks
Author
|
@kamilmysliwiec Absolutely, I'll rebase onto v12.0.0 and push the updated branch. Will get that done shortly! |
veeceey
force-pushed
the
fix/issue-9056-ws-exception-filter
branch
from
f760dae to
35a2306
Compare
Member
|
Author
|
Thanks for flagging that @kamilmysliwiec — looks like the build is failing after the rebase. I'll look into the CI failure and push a fix. Apologies for the breakage. |
Author
|
All good now - CI is passing. The build-and-test job completed successfully. |
The BaseWsExceptionFilter used client.emit() which only works with
Socket.IO clients. Native WebSocket clients (via @nestjs/platform-ws)
don't have an emit method, so exceptions were silently swallowed.
Added an emitMessage() method that detects the client type and uses
client.send() with JSON-serialized { event, data } payloads for native
WS clients, matching the message format the WS adapter already uses.
Also removed the !client.emit guard in WsExceptionsHandler that
prevented exception handling from reaching native WS clients at all.
Closes nestjs#9056
The emitMessage() method checked for client.emit first, but native WebSocket clients from the ws package inherit from EventEmitter and also have an emit method that only dispatches events locally. This caused WsException errors to be emitted as local events instead of being sent over the wire, resulting in the e2e test timing out. Added an isNativeWebSocket() check that looks for a numeric readyState property (per the WebSocket spec) to reliably distinguish native WS clients from Socket.IO sockets. Also fixed prettier formatting on the emitMessage generic signature, and updated test mocks to include readyState on the native WS client stub.
Replace sinon.stub() and Chai-style assertions with Vitest's vi.fn() and expect() to fix CI failures in the test environment.
The ws-error-gateway integration test was still using chai assertions and CJS-style imports which are incompatible with the v12.0.0 branch that has migrated to Vitest and ESM. Replaced chai's expect with Vitest globals, changed to default WebSocket import, added .js extension to local import, and removed a stray console.log from the unit test.
veeceey
force-pushed
the
fix/issue-9056-ws-exception-filter
branch
from
8cd7c66 to
104ca52
Compare
veeceey
force-pushed
the
fix/issue-9056-ws-exception-filter
branch
from
104ca52 to
4ee5330
Compare
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
3 participants
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.
Summary
BaseWsExceptionFiltercalledclient.emit('exception', payload)to send errors, but native WebSocket clients (via@nestjs/platform-ws) don't have anemitmethod — only Socket.IO clients do. This meant that when usingWsAdapter, anyWsExceptionthrown in a@SubscribeMessagehandler was silently swallowed and never reached the client.On top of that,
WsExceptionsHandler.handle()had a!client.emitguard that short-circuited the entire exception handling pipeline for non-Socket.IO clients.Changes
emitMessage()method toBaseWsExceptionFilterthat checks whether the client hasemit(Socket.IO) orsend(native WS) and dispatches accordinglyJSON.stringify({ event: 'exception', data: payload }), consistent with the{ event, data }message format the WS adapter already uses!client.emitguard inWsExceptionsHandlerto!client, so exceptions are properly handled regardless of adapter type{ emit: Function }to{ emit?: Function; send?: Function }to accept both client typesTest plan
ws-error-gateway.spec.ts) that spins up aWsAdapter-based gateway, sends a message that triggers aWsException, and asserts the error response is received by the clientCloses #9056