fix(ws): expose crossws on app.fetch response#1306
fix(ws): expose crossws on app.fetch response#1306ahmetguness wants to merge 3 commits intoh3js:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughUpdated type system to introduce Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/response.ts (1)
15-19: Consider updating internal cast for consistency.The return type is now
H3Response | Promise<H3Response>, but line 19 still casts toPromise<Response>. While this works due to structural typing (the recursivetoResponsecall returns the correct type), updating the cast would improve code consistency.Proposed fix
if (typeof (val as PromiseLike<unknown>)?.then === "function") { return ((val as Promise<unknown>).catch?.((error) => error) || Promise.resolve(val)).then( (resolvedVal) => toResponse(resolvedVal, event, config), - ) as Promise<Response>; + ) as Promise<H3Response>; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/response.ts` around lines 15 - 19, The return type cast at the end of the async branch in toResponse is inconsistent: it currently casts to Promise<Response> even though the function signature and recursive toResponse call use H3Response; update the cast to Promise<H3Response> (or remove the cast entirely) so the expression returns the correct Promise<H3Response> type consistently with toResponse and the function signature.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/index.ts`:
- Around line 194-195: There are two identical `// WebSocket` comments
duplicated in src/index.ts; remove one so only a single `// WebSocket` comment
remains above the WebSocket-related code (e.g., the WebSocket server/handler
block) to avoid redundancy — locate the duplicate `// WebSocket` markers and
delete the extra one.
---
Nitpick comments:
In `@src/response.ts`:
- Around line 15-19: The return type cast at the end of the async branch in
toResponse is inconsistent: it currently casts to Promise<Response> even though
the function signature and recursive toResponse call use H3Response; update the
cast to Promise<H3Response> (or remove the cast entirely) so the expression
returns the correct Promise<H3Response> type consistently with toResponse and
the function signature.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
docs/5.migration/0.index.mdsrc/h3.tssrc/index.tssrc/response.tssrc/types/h3.tssrc/utils/ws.tstest/unit/types.test-d.tstest/ws.test.ts
… + correct H3Response cast)
When using
defineWebSocketHandler, the returnedResponseincludes acrosswsproperty.However,
app.fetch()is typed as returningResponse, sores.crosswsis not visible in TypeScript without casting.H3Responsetype (Responsewith optionalcrossws)fetch/toResponseto returnH3Response(res as any).crosswsfrom testsAll checks pass locally (
lint,typecheck,vitest).Closes #1258
Summary by CodeRabbit
New Features
Tests