Skip to content

Refactor HEAD method handling in compose.ts#1579

Merged
SaltyAom merged 1 commit intoelysiajs:mainfrom
gosvoh:main
Nov 29, 2025
Merged

Refactor HEAD method handling in compose.ts#1579
SaltyAom merged 1 commit intoelysiajs:mainfrom
gosvoh:main

Conversation

@gosvoh
Copy link
Copy Markdown
Contributor

@gosvoh gosvoh commented Nov 28, 2025

Fixes #1569

The Bug

When a HEAD request is made to a route that uses async parameter destructuring (e.g., get("/:id", async ({ params: { id } }) => { ... })), the server crashes with an error: ❌ undefined is not an object (evaluating '_res.headers.set').

The root cause was that the code handling HEAD requests did not properly account for handlers returning a Promise. This led to an attempt to access the .headers property on a Promise object (which is undefined) instead of on the resolved Response.

The Solution

The fix ensures the result of the route handler is properly resolved using Promise.resolve() before proceeding. It also includes safeguards to ensure the headers object exists on the response before trying to modify it.

Summary by CodeRabbit

  • Bug Fixes

    • Improved handling of HEAD requests to ensure proper content-length headers are reliably set.
  • Refactor

    • Restructured response handling logic for web-standard requests to enhance code clarity and maintainability.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Nov 28, 2025

Walkthrough

The HEAD request handling in composeGeneralHandler was refactored to safely manage the response object and content-length header. The code restructured the response handling into a Promise chain with an explicit guard ensuring headers exist before attempting to set the content-length property, preventing undefined reference errors.

Changes

Cohort / File(s) Change Summary
HEAD Request Response Handling
src/compose.ts
Refactored HEAD request path to wrap response handling in Promise.resolve().then() chain, added explicit header existence guard before setting content-length, and converted multi-line concatenation to explicit semicolon-terminated style with improved error resilience.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Verify the Promise chain correctly awaits and processes the response object
  • Confirm the headers existence guard (_res.headers ||= new Headers()) prevents the undefined reference error from issue #1569
  • Validate that getResponseLength(_res) functions correctly with the new header handling flow
  • Check that the final Response construction maintains correct status, statusText, and headers

Possibly related PRs

  • fix head request handler again #1524: Modifies the same HEAD handling logic in composeGeneralHandler to implement Promise-based response flow with content-length computation and header management.
  • fix head request #1522: Directly related change to the same HEAD response handling code in src/compose.ts, rewriting response awaiting and content-length header setting with improved Promise chaining.

Poem

🐰 A header lost in the void so dark,
Now caught before it leaves a mark,
With guards we place and checks we make,
No undefined shall cause us to break!
Promise chains mend what once was torn,
HEAD requests greet a safer morn.

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Refactor HEAD method handling in compose.ts' accurately describes the main change—refactoring the HEAD request handling logic in the compose.ts file.
Linked Issues check ✅ Passed The PR addresses issue #1569 by fixing the bug where HEAD requests with async parameter destructuring crash due to improper Promise handling and missing headers guards.
Out of Scope Changes check ✅ Passed All changes are scoped to fixing the HEAD method handling issue; no unrelated or out-of-scope modifications are present in the changeset.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3ad795b and b8f5688.

📒 Files selected for processing (1)
  • src/compose.ts (1 hunks)
🔇 Additional comments (1)
src/compose.ts (1)

2294-2308: Review comment is incorrect—the described fixes are not present in the actual code.

The review comment claims to show the final state of lines 2294-2308 with Promise.resolve() wrapping and a headers guard (if(!_res.headers)_res.headers=new Headers();). However, the actual current code in src/compose.ts does not contain these fixes.

Actual state (lines 2294-2310):

  • Uses direct getResponseLength(_res).then(...) without Promise.resolve() wrapping
  • No headers guard before _res.headers.set()
  • If a handler returns a Promise, _res becomes a Promise object, not a Response—causing getResponseLength() to fail since it expects a Response parameter

Comparison with static route handling (lines 2383-2388):

  • Static routes correctly use Promise.resolve(ht[...].composed(c)).then(_ht=>getResponseLength(_ht)...)
  • This pattern properly handles both sync and async handlers

The dynamic route HEAD handling has a real bug that the review comment's proposed snippet would fix, but that fix has not been applied to the codebase.

Likely an incorrect or invalid review comment.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@gosvoh gosvoh marked this pull request as ready for review November 28, 2025 20:35
@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new bot commented Nov 29, 2025

Open in StackBlitz

npm i https://pkg.pr.new/elysiajs/elysia@1579

commit: b8f5688

@SaltyAom SaltyAom merged commit 5a28cd7 into elysiajs:main Nov 29, 2025
3 checks passed
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.

Error: undefined is not an object (evaluating '_res.headers.set')

2 participants