fix: resolve writeEarlyHints hang with empty hints on Node.js#1400
fix: resolve writeEarlyHints hang with empty hints on Node.js#1400leno23 wants to merge 1 commit into
Conversation
Closes h3js#1383 Co-authored-by: Cursor <cursoragent@cursor.com>
📝 WalkthroughWalkthroughThe PR fixes a bug where ChangesEarly Hints Processing
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
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.
🧹 Nitpick comments (2)
test/utils.test.ts (1)
473-484: ⚡ Quick winAdd a Node case for capitalized
Linkhints.This only locks in the
{}hang fix. The other half of the regression was{ Link: ... }on the native Node path, and the web-only fallback test below does not exercise that normalization. Please add a Node-targeted case that awaitswriteEarlyHints(event, { Link: ... })and asserts the handler still completes.🤖 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 `@test/utils.test.ts` around lines 473 - 484, Add a Node-targeted test that covers the capitalized header shape by creating a test (using the same pattern as the existing "resolves immediately when hints are empty on Node.js" block) which runs only when t.target === "node", registers a handler that awaits writeEarlyHints(event, { Link: '<...>' }) (use a valid Link value like "</style.css>; rel=preload; as=style"), then returns "ok", and finally fetches the route and asserts the response body is "ok"; reference writeEarlyHints and the existing test name to locate where to add this case so the native Node normalization of { Link: ... } is exercised.src/utils/response.ts (1)
136-149: ⚡ Quick winMove
getEarlyHintLinksto an internal location.This helper is internal-only, but it's now sitting in the middle of
src/utils/response.ts. Please move it to the end of the file or intosrc/utils/internal/so the file keeps the expected layout.As per coding guidelines,
src/**/*.ts:Place internal helpers at the end of files or in utils/internal/ subdirectory.🤖 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/response.ts` around lines 136 - 149, The getEarlyHintLinks helper is internal-only and should be relocated out of the main body: move the getEarlyHintLinks function to the end of this file (or into the utils/internal/ module) so internal helpers stay separate; after moving, update any local references or imports to point to the new location (or keep it file-local and unexported) and run type-checks to ensure no exported API changed; keep the function name getEarlyHintLinks unchanged and ensure its behavior and signatures remain identical.
🤖 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.
Nitpick comments:
In `@src/utils/response.ts`:
- Around line 136-149: The getEarlyHintLinks helper is internal-only and should
be relocated out of the main body: move the getEarlyHintLinks function to the
end of this file (or into the utils/internal/ module) so internal helpers stay
separate; after moving, update any local references or imports to point to the
new location (or keep it file-local and unexported) and run type-checks to
ensure no exported API changed; keep the function name getEarlyHintLinks
unchanged and ensure its behavior and signatures remain identical.
In `@test/utils.test.ts`:
- Around line 473-484: Add a Node-targeted test that covers the capitalized
header shape by creating a test (using the same pattern as the existing
"resolves immediately when hints are empty on Node.js" block) which runs only
when t.target === "node", registers a handler that awaits writeEarlyHints(event,
{ Link: '<...>' }) (use a valid Link value like "</style.css>; rel=preload;
as=style"), then returns "ok", and finally fetches the route and asserts the
response body is "ok"; reference writeEarlyHints and the existing test name to
locate where to add this case so the native Node normalization of { Link: ... }
is exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9f09122b-0266-41a3-97ed-5ecf51a152ee
📒 Files selected for processing (2)
src/utils/response.tstest/utils.test.ts
Summary
writeEarlyHints()hanging indefinitely on Node.js when called with empty hints ({})Linkheaders to Node's expected lowercaselinkkey before calling nativewriteEarlyHintsFixes #1383
Root cause
Node.js
ServerResponse.writeEarlyHints()only invokes its callback when validlinkhints are provided. Passing{}or{ Link: ... }(capital L) never triggers the callback, causingawait writeEarlyHints(event, {})to hang forever.Test plan
await writeEarlyHints(event, {})no longer hangs)pnpm vitest run test/utils.test.ts -t writeEarlyHintspassesSummary by CodeRabbit
Bug Fixes
Tests