-
Notifications
You must be signed in to change notification settings - Fork 100
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: wildcard head return content-length header #448
Conversation
Haha, you are fast ;) Thanks for the PR, you are right |
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.
lgtm
Can you release a patch? |
I can, but most of the time I will wait for two approval before merge. |
@@ -1740,7 +1740,7 @@ t.test('with fastify-compress', t => { | |||
}) | |||
}) | |||
t.test('register /static/ with schemaHide true', t => { | |||
t.plan(4) | |||
t.plan(3) |
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.
Why? There weren't unfinished test errors before?
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.
Because we no longer have GET
and HEAD
registered separately.
FYI: The internal HEAD
route also trigger onRoute
hook.
@climba03003 this looks like a problem in Fastify too, no? A What do you think? |
No, as described in the PR The automatic one requires to always send the actual payload otherwise it can not calculate properly. |
Okay, thanks |
Fixes #447
The reason behind we should not use
exposeHeadRoute
is that insidepumpSendToReply
, we never providepayload
forHEAD
route. So,fastify
think it is providingempty
response and override theContent-Length
unexpectedly.Checklist
npm run test
andnpm run benchmark
and the Code of conduct