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: set reply's default charset to utf8 #3789

Merged
merged 2 commits into from Mar 22, 2022
Merged

Conversation

xtx1130
Copy link
Contributor

@xtx1130 xtx1130 commented Mar 22, 2022

ref: #3788

Checklist

@xtx1130
Copy link
Contributor Author

xtx1130 commented Mar 22, 2022

benchmark:

this branch:
image
master:
image


fastify.listen({ port: 0 }, err => {
t.error(err)
fastify.server.unref()
Copy link
Member

Choose a reason for hiding this comment

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

I know it is used in other tests, but could you use the t.teaddown function instead of server.unref?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PTAL, but I want to know what is the difference between t.teardown & server.unref, more semantic?

Copy link
Member

Choose a reason for hiding this comment

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

t.teardown run when the test end and you can provide callback function to exit the environment cleanly.
server.unref just didn't block the program end when no code need to run inside the event loop. It is an unclean close.

@Eomm Eomm changed the title fix: #3788 fix: set reply's default charset to utf8 Mar 22, 2022
@Eomm Eomm linked an issue Mar 22, 2022 that may be closed by this pull request
2 tasks
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

Good spot! This is an amazing speed bump. How did you find it?

Copy link
Member

@zekth zekth left a comment

Choose a reason for hiding this comment

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

Nice ✌️

@xtx1130
Copy link
Contributor Author

xtx1130 commented Mar 22, 2022

Good spot! This is an amazing speed bump. How did you find it?

Thanks for the bug report, and I find these codes can be more simplified

@@ -261,6 +261,7 @@ Sets the content type for the response. This is a shortcut for
```js
reply.type('text/html')
```
When set `Content-Type` with json type, if you don't set `charset`, it will use `utf-8` by default.
Copy link
Member

@Fdawgs Fdawgs Mar 22, 2022

Choose a reason for hiding this comment

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

Suggested change
When set `Content-Type` with json type, if you don't set `charset`, it will use `utf-8` by default.
If the `Content-Type` has a JSON subtype, and the charset parameter is not set, `utf-8` will be used as the charset by default.

Copy link
Member

Choose a reason for hiding this comment

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

Ooops, I missed this. I'll amend on main.

Copy link
Member

Choose a reason for hiding this comment

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

Cheers @mcollina!

Copy link
Member

@delvedor delvedor left a comment

Choose a reason for hiding this comment

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

🚀

Copy link
Member

@RafaelGSS RafaelGSS left a comment

Choose a reason for hiding this comment

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

LGTM.

@mcollina mcollina merged commit 78e9ddb into fastify:main Mar 22, 2022
xtx1130 added a commit to xtx1130/docs-chinese that referenced this pull request Mar 29, 2022
fralonra pushed a commit to fastify/docs-chinese that referenced this pull request Jun 13, 2022
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reply's content-type header value suffix stripped when charset is omitted
8 participants