Skip to content
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: default head requests for endpoints when no explicit head method… #13210

Merged
merged 5 commits into from
Feb 13, 2025

Conversation

VitaliyR
Copy link
Contributor

… implemented

Changes

I've noticed that /_image endpoint doesn't respond nicely (non 404) to HEAD requests. After investigation, I've found that none of server endpoints respond to the HEAD - they respond as 404.

User must explicitly implement export const HEAD to add support for this endpoint or provide export const ALL.

But typically users won't be doing this, since they typically might want just to add some GET handler and thats it. Since HEAD endpoint is useful for various things, for instance, checking if there is a route there but not caring about it's response - this PR adds default HEAD implementation for such cases.

  • In case user would implement it explicitly, their implementation works export const HEAD
  • If there are no export const GET handler - HEAD won't be auto-provided as well
  • If user implemented ALL -> HEAD won't be auto-provided

Testing

  1. TDD: Added test cases, tested before & after
  2. Run examples/ssr/ via npm run dev:
    2.1. fetch('http://localhost:4321/api/products') -> 200 (API endpoint)
    2.2. fetch('http://localhost:4321/api/products', { method: 'HEAD' }) -> before 404, after 200
    2.3. fetch('http://localhost:4321/products/2', { method: 'HEAD' }) -> before 200, after 200 (Page endpoint, not affected)

Docs

Not sure about if docs should be updated.
Personally I'd expect this to work outside the box.

Copy link

changeset-bot bot commented Feb 10, 2025

🦋 Changeset detected

Latest commit: 3709bf9

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Feb 10, 2025
Copy link

codspeed-hq bot commented Feb 10, 2025

CodSpeed Performance Report

Merging #13210 will not alter performance

Comparing VitaliyR:fix-endpoint-head-requests (3709bf9) with main (3b66955)

Summary

✅ 6 untouched benchmarks

@ascorbic
Copy link
Contributor

I think the idea of automatically enabling HEAD for GET handlers is good, but I think this needs a different implementation. The spec says that HEAD should return the same headers as the equivalent GET request, but this PR has it returning nothing. I think the right approach would be to call the GET handler and then return a new response with the returned status and headers.

@VitaliyR VitaliyR force-pushed the fix-endpoint-head-requests branch from 67a58dc to 5d2395f Compare February 12, 2025 14:16
@VitaliyR
Copy link
Contributor Author

@ascorbic originally I've thought about saving time on the executing the GET handler, but, perhaps, you are right - according to spec we should aim to return the same status && headers, and we can do that only by executing the GET itself.

I've updated the PR, wdyt?

Copy link
Member

@florian-lefebvre florian-lefebvre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@github-actions github-actions bot added the semver: minor Change triggers a `minor` release label Feb 12, 2025
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is blocked because it contains a minor changeset. A reviewer will merge this at the next release if approved.

@ascorbic ascorbic added this to the 5.3.0 milestone Feb 12, 2025
@ascorbic
Copy link
Contributor

Updated the changeset and added a line to the docs: withastro/docs#10947

@ascorbic ascorbic merged commit 344e9bc into withastro:main Feb 13, 2025
16 checks passed
@astrobot-houston astrobot-houston mentioned this pull request Feb 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope) semver: minor Change triggers a `minor` release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants