Allow enabling websocket compression in NodeHttpServer and BunHttpServer - #6691
Conversation
…erver Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 51c31fb The changes in this PR will be included in the next version bump. This PR includes changesets to release 29 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughNodeHttpServer and BunHttpServer now accept optional websocket configuration, forward supported settings to their WebSocket implementations, and retain internal lifecycle handling. Node tests verify ChangesWebSocket options
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant WebSocketClient
participant NodeHttpServer
participant NodeWS.WebSocketServer
WebSocketClient->>NodeHttpServer: Connect to /ws with extension settings
NodeHttpServer->>NodeWS.WebSocketServer: Create server with websocket options and noServer
NodeWS.WebSocketServer-->>WebSocketClient: Negotiate permessage-deflate extension
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/platform-node/src/NodeHttpServer.ts`:
- Around line 65-85: Update the WebSocketServer creation flow in NodeHttpServer
to explicitly remove port and server from runtime options.websocket before
forwarding them, while retaining the locally managed noServer setting. Ensure
only non-conflicting WebSocket options are passed to NodeWS.Server.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b90fcd19-0fae-4d1b-a93b-dcbe4ee81a99
📒 Files selected for processing (4)
.changeset/http-server-websocket-options.mdpackages/platform-bun/src/BunHttpServer.tspackages/platform-node/src/NodeHttpServer.tspackages/platform-node/test/NodeHttpServer.test.ts
github-project-automation
Bot
moved this from Discussion Ongoing
to Waiting on Author
in PR Backlog
| */ | ||
| export type WebSocketOptions = Omit< | ||
| Bun.WebSocketHandler<WebSocketContext>, | ||
| "open" | "message" | "close" | "drain" | "ping" | "pong" | "data" |
There was a problem hiding this comment.
I think we should also drop "binaryType" here too, so it can't be set to arraybuffer.
There was a problem hiding this comment.
Done in 51c31fb — binaryType added to the omit. Good call: the server's message handler feeds Socket.fromWebSocket, which expects the default nodebuffer framing, so letting callers flip it to arraybuffer would break message decoding.
| * `websocket` is forwarded to the underlying `ws` `WebSocketServer`, minus the | ||
| * wiring options the server manages itself. Use it to enable | ||
| * `permessage-deflate` compression or tune payload limits, e.g. | ||
| * `NodeHttpServer.layer(() => createServer(), { port: 3000, websocket: { perMessageDeflate: true } })`. |
There was a problem hiding this comment.
Have this comment inline with the property or comment on all the property keys.
There was a problem hiding this comment.
Moved inline onto the websocket property in 51c31fb. Left the neighboring shutdown options as they were since they predate this PR.
Bundle Size Analysis
|
There was a problem hiding this comment.
Small change, otherwise LGTM. I'm griping with the fact that shovelling down a config to the constructor feels like its not taking advantage of effect by creating a service with a default layer for the config but this is small enough that I'll leave it to you or @tim-smart whether that makes sense here.
…naryType from Bun options Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The websocket options passthrough was upstreamed as Effect-TS/effect#6691 and ships in @effect/platform-{node,bun} >= 4.0.0-beta.103. The comments mark both patches as deletable on that upgrade, replaced by the layer option websocket: { perMessageDeflate } in apps/server/src/server.ts. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
3 participants
tl;dr - I want to add compression to websockets in T3 Code. It will help cut data transferred on T3 Code by as much as 80%
Happy to talk more about this. Figured the patch was simple enough to file a PR quick - no hard feelings if this gets closed :)
---- FROM HERE DOWN IS AI SLOP ----
Browsers offer
permessage-deflateon every WebSocket connection, but an Effect HTTP server has no way to accept it.NodeHttpServerhardcodesnew WebSocketServer({ noServer: true }), andBunHttpServeroverwrites anywebsocketkey in the serve options with its own handlers — so there is no way to enable compression or tune payload limits without patching the packages.For apps streaming JSON over websockets this leaves a lot on the table: measured on real production traffic (small ~500B-1KB frames with repeated keys and UUIDs, raw TCP byte counts), accepting
permessage-deflatewith context takeover cuts wire bytes by 4x.Solution
Both servers accept an optional
websocketfield in their existing options, forwarded to the underlying implementation. The wiring/lifecycle options the server manages (noServer,open/message/close, etc.) are excluded from the type so they cannot be overridden.Omitting the option keeps today's behavior exactly; clients that don't offer the extension keep uncompressed frames on their connection.
Includes a test asserting the server accepts
permessage-deflatewhen configured and offered, and that clients not offering it still connect uncompressed.tsc -bon both packages, oxlint, dprint, and jsdocs are clean; a changeset is included.🤖 Drafted with Claude Code (Fable 5), reviewed and filed by a human.
Summary by CodeRabbit
websocketoption to both Node.js and Bun HTTP servers.