types: expose .crossws on defineWebSocketHandler return type#1435
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a ChangesWebSocket Handler Typing
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![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 `@src/utils/ws.ts`:
- Around line 13-21: The WebSocket response type and `defineWebSocketHandler()`
integration currently assume `crossws` is a synchronous Partial<WebSocketHooks>,
but async hook factories can return a Promise and end up attaching the wrong
runtime shape. Update the `WebSocketResponse`/`crossws` flow so the hooks
produced by `defineWebSocketHandler()` are awaited before assignment, or
otherwise restrict the accepted hook factory overloads to sync-only behavior;
make sure the change is reflected where `crossws` is attached and consumed by
adapters.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 99f5b9bf-f76e-4619-9bb1-a4bacb6bcc7c
📒 Files selected for processing (11)
src/adapters.tssrc/event.tssrc/index.tssrc/utils/ws.tstest/app.test.tstest/bench/bundle.test.tstest/event.test.tstest/unit/event.test.tstest/unit/request.test.tstest/unit/types.test-d.tstest/ws.test.ts
pi0
changed the title
.crossws on defineWebSocketHandler return type
pi0
approved these changes
Jul 4, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/utils/ws.ts (1)
96-115: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated response-construction logic.
The
Object.assign(new Response("WebSocket upgrade is required.", { status: 426 }), { crossws: ... })pattern is duplicated between the async-resolution branch and the sync fallback, differing only in thecrosswsvalue used.♻️ Suggested consolidation
+ const toUpgradeResponse = (resolvedCrossws: Partial<WebSocketHooks>) => + Object.assign( + new Response("WebSocket upgrade is required.", { status: 426 }), + { crossws: resolvedCrossws }, + ); + if (crossws instanceof Promise) { - return crossws.then((resolvedCrossws) => - Object.assign( - new Response("WebSocket upgrade is required.", { - status: 426, - }), - { - crossws: resolvedCrossws, - }, - ), - ); + return crossws.then(toUpgradeResponse); } - return Object.assign( - new Response("WebSocket upgrade is required.", { - status: 426, - }), - { - crossws, - }, - ); + return toUpgradeResponse(crossws);🤖 Prompt for 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. In `@src/utils/ws.ts` around lines 96 - 115, The response-construction logic in the websocket upgrade handler is duplicated between the `crossws.then(...)` branch and the sync fallback. Refactor the repeated `Object.assign(new Response("WebSocket upgrade is required.", { status: 426 }), { crossws: ... })` pattern into a shared helper or single construction path in `src/utils/ws.ts`, and have both branches reuse it by passing the resolved `crossws` value.
🤖 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 `@src/utils/ws.ts`:
- Around line 20-24: The WebSocket response typing in
WebSocketResponse/EventHandler currently only models a synchronous Response,
which forces casts for async hooks factories. Update the types around
defineWebSocketHandler so async hooks factories are represented as
Promise<WebSocketResponse> (or a union that preserves awaited responses), and
ensure crossws remains available after awaiting the response without requiring
casts.
---
Nitpick comments:
In `@src/utils/ws.ts`:
- Around line 96-115: The response-construction logic in the websocket upgrade
handler is duplicated between the `crossws.then(...)` branch and the sync
fallback. Refactor the repeated `Object.assign(new Response("WebSocket upgrade
is required.", { status: 426 }), { crossws: ... })` pattern into a shared helper
or single construction path in `src/utils/ws.ts`, and have both branches reuse
it by passing the resolved `crossws` value.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d06649bb-cf16-4e49-bc9c-32f93151d016
📒 Files selected for processing (2)
src/utils/ws.tstest/ws.test.ts
Contributor
Author
|
The async-factory await concern was fixed in 892bfe0 (the factory result is awaited before crossws is attached, covered by the awaits-an-async-hooks-factory test). The follow-up review's return-type point is also addressed now: defineWebSocketHandler's factory overload returns WebSocketResponse | Promise, with a type test that fails on the sync-only type. Resolving as addressed. |
`defineWebSocketHandler()` attaches `crossws` hooks to the 426 Response at
runtime via `Object.assign`, but its return type was a plain `EventHandler`
whose response resolved to `unknown`. Consumers (and the crossws adapter
pattern of reading `res.crossws`) had to cast with `as any` to reach the
field, and `app.fetch`-style callers saw no `crossws` at all.
Add a scoped `WebSocketResponse = Response & { crossws?: Partial<WebSocketHooks> }`
type and typed overloads so the returned handler's response exposes `crossws`
without a cast. The http-fallback overload preserves the fallback handler's
return type in the union. This is a type-only change; runtime behavior is
unchanged.
Fixes the type mismatch from h3js#1258 with a scoped augmentation rather than a
global response type, per maintainer guidance on h3js#1306.
Closes h3js#1258
defineWebSocketHandler() accepted an async hooks(event) factory but never awaited it, so res.crossws ended up as an unresolved Promise instead of the hooks object. Await it before attaching, and keep the sync path untouched.
defineWebSocketHandler only declared a sync WebSocketResponse return type, so an async hooks factory still forced a cast to await the response and read crossws. Split the overloads by hooks shape so a factory (sync or async) returns WebSocketResponse | Promise<WebSocketResponse>, while plain object hooks keep the sync-only type. Also extract the duplicated 426 response construction into one toUpgradeResponse helper.
The async factory test awaited res and checked crossws on the awaited value, but Awaited<T> is a no-op on non-Promise types and crossws is optional on WebSocketResponse, so this passed even with a sync-only overload. Assert on res itself against the union type instead.
pi0x
force-pushed
the
fix/websocket-handler-crossws-type-1258
branch
from
July 5, 2026 17:21
0353f07 to
3731217
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
2 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.
defineWebSocketHandler()attachescrosswshooks to the returned Response at runtime, but the declared return type resolved tounknown, so readingres.crosswsneeded a cast (#1258).This adds a scoped
WebSocketResponsetype and typed overloads (hooks-only and http-fallback) socrosswsis visible. Type-only change, runtime behaviour is unchanged. Kept scoped rather than introducing a global response type, per the maintainer note on #1306.Tests: a runtime check that
res.crosswsequals the hooks without a cast, plusexpectTypeOfassertions. Full suite passes, tsc and lint clean.Closes #1258
Summary by CodeRabbit
WebSocketResponsefrom the package entrypoint.defineWebSocketHandlertyping so WebSocket responses includecrosswsin a type-safe way.crosswsis now attached only after hooks resolve, ensuring correct values on426 Upgrade Required.crosswspresence and async hook behavior.defineWebSocketHandlerbehavioral details from the docs.