-
-
Notifications
You must be signed in to change notification settings - Fork 448
fix: add WebSocketConfig type for constructor options #1642
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: main
Are you sure you want to change the base?
Conversation
WalkthroughAdds a new exported Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (3)src/ws/bun.ts (1)
src/universal/server.ts (2)
src/types.ts (2)
🔇 Additional comments (6)
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 |
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/ws/bun.ts (1)
319-372: Consider refactoring to eliminate type duplication.The new
WebSocketConfigtype duplicates the configuration properties already defined inWebSocketHandler(lines 491-549). This creates a maintenance risk where updating one type without the other could lead to inconsistencies.🔎 Refactor suggestion to eliminate duplication
Consider having
WebSocketHandlerextendWebSocketConfigto avoid duplicating the configuration properties:+/** + * WebSocket configuration options (without handler methods). + * Use this type when you only need to configure WebSocket behavior + * without defining message handlers. + */ +export type WebSocketConfig = { + /** + * Sets the maximum size of messages in bytes. + * @default 16 MB (1024 * 1024 * 16) + */ + maxPayloadLength?: number + + /** + * Sets the maximum number of bytes that can be buffered on a single connection. + * @default 16 MB (1024 * 1024 * 16) + */ + backpressureLimit?: number + + /** + * Sets if the connection should be closed if `backpressureLimit` is reached. + * @default false + */ + closeOnBackpressureLimit?: boolean + + /** + * Sets the number of seconds to wait before timing out a connection + * due to no messages or pings. + * @default 120 (2 minutes) + */ + idleTimeout?: number + + /** + * Should `ws.publish()` also send a message to `ws` (itself), if it is subscribed? + * @default false + */ + publishToSelf?: boolean + + /** + * Should the server automatically send and respond to pings to clients? + * @default true + */ + sendPings?: boolean + + /** + * Sets the compression level for messages, for clients that support it. + * @default false (disabled) + */ + perMessageDeflate?: + | boolean + | { + compress?: WebSocketCompressor | boolean + decompress?: WebSocketCompressor | boolean + } +} -export interface WebSocketHandler<in out T = undefined> { +export interface WebSocketHandler<in out T = undefined> extends WebSocketConfig { /** * Called when the server receives an incoming message. */ message( ws: ServerWebSocket<T>, message: string | Buffer ): void | Promise<void> /** * Called when a connection is opened. */ open?(ws: ServerWebSocket<T>): void | Promise<void> // ... other handler methods ... - /** - * Sets the maximum size of messages in bytes. - * - * Default is 16 MB, or `1024 * 1024 * 16` in bytes. - */ - maxPayloadLength?: number - - // ... remove other duplicated config properties ... }This ensures the configuration properties are defined in one place and automatically inherited by
WebSocketHandler.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/types.tssrc/universal/server.tssrc/ws/bun.tssrc/ws/index.ts
🧰 Additional context used
🧬 Code graph analysis (3)
src/ws/bun.ts (1)
src/ws/index.ts (1)
WebSocketConfig(11-11)
src/universal/server.ts (2)
src/ws/bun.ts (1)
WebSocketConfig(324-372)src/ws/index.ts (1)
WebSocketConfig(11-11)
src/types.ts (2)
src/ws/bun.ts (1)
WebSocketConfig(324-372)src/ws/index.ts (1)
WebSocketConfig(11-11)
🔇 Additional comments (5)
src/ws/index.ts (1)
7-11: LGTM! Clean public API exposure.The import and re-export of
WebSocketConfigcorrectly makes the type available for public consumption. This allows users to reference the type when configuring WebSocket options.src/universal/server.ts (2)
3-3: LGTM! Correct import.The import of
WebSocketConfigfrom the internal module is appropriate for typing the newwebsocketproperty.
125-130: LGTM! Well-documented property addition.The optional
websocketproperty correctly uses the newWebSocketConfigtype and includes helpful documentation. This enables users to pass WebSocket configuration through the constructor as intended.src/types.ts (2)
35-35: LGTM! Import updated correctly.The import now includes
WebSocketConfigalongside the existingWebSocketHandler, enabling the type update inElysiaConfig.
174-174: LGTM! Cleaner type definition.The change from
Omit<WebSocketHandler<any>, 'open' | 'close' | 'message' | 'drain'>toWebSocketConfigis a significant improvement. The new type is more explicit and semantically correct—configuration should contain only configuration properties, not handler methods.Note: The previous type inadvertently included the
ping?andpong?handler methods (they weren't in the omit list), which was likely unintended. The newWebSocketConfigcorrectly excludes all handler methods, focusing purely on configuration options.
Adds a new WebSocketConfig type that includes only WebSocket configuration options (idleTimeout, maxPayloadLength, etc.) without requiring handler methods like 'message'. This fixes the issue where users couldn't pass WebSocket config to the Elysia constructor without TypeScript errors, since WebSocketHandler requires the 'message' handler. Fixes elysiajs#1517
96ac9ac to
52e2522
Compare
Summary
WebSocketConfigtype that includes only WebSocket configuration options (idleTimeout, maxPayloadLength, etc.)ElysiaConfig.websocketto useWebSocketConfiginstead ofOmit<WebSocketHandler, ...>WebSocketConfigfromws/index.tsProblem
Users couldn't pass WebSocket config to the Elysia constructor without TypeScript errors:
This happened because
WebSocketHandlerrequires themessagehandler method.Solution
Created a separate
WebSocketConfigtype that only includes configuration options without handler methods. This allows users to configure WebSocket behavior at the app level without needing@ts-expect-error.Test plan
bun run build)Fixes #1517
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.