-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
fix(start-server-core): use public h3-v2 api event.res #6162
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
fix(start-server-core): use public h3-v2 api event.res #6162
Conversation
WalkthroughThis PR refactors response handling in start-server-core by replacing internal h3 response symbol access with the public event.res property and removing optional chaining operators when accessing response status properties, assuming getResponse() consistently returns a defined Response object. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
|
View your CI Pipeline Execution ↗ for commit 1230cb0
☁️ Nx Cloud last updated this comment at |
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)
packages/start-server-core/src/server-functions-handler.ts (1)
221-231: Consider consistent fallback defaults for response status.The removal of optional chaining on
responseis correct sincegetResponse()now returns a defined object. However, there's an inconsistency in howresponse.statusis handled:
- Success paths (lines 224, 255, 265): Use
response.statusdirectly without fallback- Error path (line 305): Uses
response.status ?? 500with explicit fallbackWhile the Response constructor accepts
undefinedstatus and defaults to 200, consider adding explicit?? 200to success paths for consistency and clarity. This would make the intended behavior more explicit and align with the error path's pattern.🔎 Optional refactor for consistent defaults
return new Response( nonStreamingBody ? JSON.stringify(nonStreamingBody) : undefined, { - status: response.status, + status: response.status ?? 200, statusText: response.statusText, headers: {Apply similar changes at lines 255 and 265 for consistency.
Also applies to: 254-261, 264-267, 303-311
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/start-server-core/src/request-response.ts(1 hunks)packages/start-server-core/src/server-functions-handler.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript strict mode with extensive type safety for all code
Files:
packages/start-server-core/src/server-functions-handler.tspackages/start-server-core/src/request-response.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Implement ESLint rules for router best practices using the ESLint plugin router
Files:
packages/start-server-core/src/server-functions-handler.tspackages/start-server-core/src/request-response.ts
🔇 Additional comments (1)
packages/start-server-core/src/request-response.ts (1)
340-343: Confirmed:event.resis the documented public API in h3-v2.The change correctly uses h3-v2's public API instead of internal symbols. The
getResponse()function safely returnsevent.res, which is documented and stable.
Summary by CodeRabbit
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.