-
Notifications
You must be signed in to change notification settings - Fork 12
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
feat: add contentType
option
#93
Conversation
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.
Copilot reviewed 4 out of 4 changed files in this pull request and generated no suggestions.
Should I just add a new test block to the types tests or add |
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 except the type test
I think you can add it to an existing test as well, both should be fine
Signed-off-by: Frazer Smith <[email protected]>
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
Any thoughts on this @climba03003? I know you've worked a lot on the rewrite of this module so didn't want to go ahead and merge without your input! |
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.
I am thinking the needs of disable option.
Since this library is just provides whatever information the consumer needs, it is up to the consumer to decide if he / she want what header.
It is a bit overkill provide an option to disable feature. (The function here is just a giant map lookup. Should not be costly.)
So... is that a tentative approval? 😆 |
Yes, but I do believe |
This PR adds the ability to disable the
Content-Type
response header via thecontentType
option.Whilst the automatic setting of the
Content-Type
response header is great, it's not always right for some edge cases, such as for vendor-specific or proprietary extensions.Likewise, the charset is hardcoded to
utf-8
fortext/plain
,text/html
andapplication/json
files; these types can support more than just utf-8 character encoded content so it'd be nice to get around that.Related to fastify/fastify-static#480.
Checklist
npm run test
andnpm run benchmark
and the Code of conduct