Skip to content

Conversation

@productdevbook
Copy link
Member

@productdevbook productdevbook commented Jul 7, 2025

Summary

cc: #1088
cc: #1138

  • Added defineWebSocketRoute utility function to simplify WebSocket route definitions
  • Added WebSocketRouteDefinition interface for type-safe WebSocket route configuration
  • Exported new utilities from main index.ts
  • WebSocket routes always use GET method (as per WebSocket upgrade spec)

Description

This PR introduces a new defineWebSocketRoute function that provides a consistent API pattern with defineRoute for defining WebSocket endpoints. This makes it easier to create WebSocket routes as plugins that can be registered with app.register().

Interface Definition

export interface WebSocketRouteDefinition {
  /** Route pattern, e.g. '/api/ws' */
  route: string;
  
  /** WebSocket hooks */
  websocket: Partial<WSHooks>;
  
  // TODO: Support middleware when implemented in defineWebSocketHandler
  // TODO: Support metadata when implemented in WebSocket routing
}

Key design decisions:

  • WebSocket routes always use GET method (standard for WebSocket upgrades)
  • No method parameter - WebSocket protocol requires GET
  • middleware and meta support deferred until implemented in underlying defineWebSocketHandler

Example usage:

import { defineWebSocketRoute } from "h3";

export const chatRoute = defineWebSocketRoute({
  route: "/api/chat",
  websocket: {
    open: (peer) => {
      console.log("WebSocket connected:", peer.id);
      peer.send("Welcome!");
    },
    message: (peer, message) => {
      console.log("Incoming message:", message);
      peer.send(`Echo: ${message}`);
    },
    close: (peer) => {
      console.log("WebSocket closed:", peer.id);
    },
  }
});

// Register with app
app.register(chatRoute);

Compatibility with existing APIs

This is fully compatible with the existing defineWebSocketHandler:

// New way (as plugin)
app.register(defineWebSocketRoute({ route: "/ws", websocket: hooks }));

// Traditional way (still works)
app.on("GET", "/ws", defineWebSocketHandler(hooks));

Test plan

  • Test WebSocket route registration with app.register()
  • Verify WebSocket upgrade works correctly
  • Test with different route patterns (including params like /ws/:id)
  • Ensure backwards compatibility with existing WebSocket handler methods
  • Verify GET method is always used
  • All tests passing after merge with main

Review feedback addressed

  • Removed method parameter - WebSocket routes always use GET (@pi0)
  • Removed middleware and meta properties - not yet supported (@pi0)
  • Added TODO comments for future feature support (@productdevbook)
  • Types kept co-located in same file (@pi0)
  • Property name stays websocket not handler (@pi0)

@productdevbook productdevbook requested a review from pi0 as a code owner July 7, 2025 08:43
@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Jul 7, 2025

Deploying h3dev with  Cloudflare Pages  Cloudflare Pages

Latest commit: fd6f13d
Status: ✅  Deploy successful!
Preview URL: https://71a3c637.h3dev.pages.dev
Branch Preview URL: https://feat-definewebsocketroute.h3dev.pages.dev

View logs

@productdevbook productdevbook changed the title feat: add defineWebSocketRoute utility feat: experimental defineWebSocketRoute Jul 7, 2025
@productdevbook productdevbook mentioned this pull request Jul 7, 2025
1 task
@codecov
Copy link

codecov bot commented Jul 7, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

📢 Thoughts on this report? Let us know!

@productdevbook productdevbook marked this pull request as draft July 7, 2025 09:40
/**
* Additional route metadata
*/
meta?: Record<string, unknown>;
Copy link
Member

@pi0 pi0 Jul 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Middleware and meta are not supported. Types should be removed (at least untill they are supported by defineWebSocket

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should take the comment line and leave a TODO ?

/**
* HTTP method for the route (typically 'GET' for WebSocket upgrades)
*/
method?: HTTPMethod;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WebSocket handlers should not accept a method.

productdevbook and others added 3 commits November 3, 2025 18:04
- Remove `method` parameter from WebSocketRouteDefinition (WebSocket routes always use GET)
- Remove `middleware` and `meta` properties (not yet supported by defineWebSocketHandler)
- Add TODO comments for future middleware and metadata support
- Update tests to reflect changes
- Remove custom method test case

Addresses review comments from @pi0 and @productdevbook in PR #1149

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
Merged latest changes from main branch to sync with upstream.

Resolved conflicts:
- src/index.ts: Added WebSocketRouteDefinition export alongside RouteDefinition
- test/unit/package.test.ts: Added defineWebSocketRoute to exports list
- Replace deprecated _routes with ~routes notation
- Replace app.fetch() with app.request() for consistency with main branch
- All route tests now passing after merge

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
@productdevbook productdevbook marked this pull request as ready for review November 3, 2025 15:08
@productdevbook
Copy link
Member Author

Hi team! I've addressed all the review feedback:

Changes made:

  1. Removed method property (@pi0's feedback)

    • WebSocket routes now always use GET method
    • Removed the optional method parameter from WebSocketRouteDefinition
  2. Removed middleware and meta properties (@pi0's feedback)

    • These are not yet supported by defineWebSocketHandler
    • Added TODO comments for future implementation (@productdevbook's suggestion)
  3. Kept types in the same file (@pi0's preference)

    • WebSocketRouteDefinition stays co-located in src/utils/route.ts
  4. Kept websocket property name (@pi0's decision)

    • Not changing to handler as they have different semantics
  5. Merged latest from main

    • Updated tests to use ~routes internal API
    • Updated to use app.request() instead of app.fetch()
    • All tests passing ✅

Commits:

  • 4369b94 - Address PR review feedback
  • 81eec94 - Merge main branch
  • 2ca3e4f - Update tests for API changes

The branch is ready for re-review! 🚀

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.

4 participants