-
-
Notifications
You must be signed in to change notification settings - Fork 78
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: undici headers #289
fix: undici headers #289
Conversation
// | ||
// in order to presist the encoding, always encode it | ||
// back to latin1 | ||
function patchUndiciHeaders (headers) { |
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.
should there be an option in undici
to leave headers as latin1
encoding?
EDIT: Ah, noticed nodejs/undici#1903 now
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 think yes. Otherwise, the patch
is prone to error.
undici
always trans-code the header. Maybe it should really detect the encoding before trans-code. That means the behavior should be identical to http.request
.
Actually, I already open a issue on undici
.
See nodejs/undici#1903
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
Co-authored-by: Simen Bekkhus <[email protected]>
Would this need a revert after undici is fixed? |
I believe, yes. |
FYI this causes regression with |
Can you provide a reproducible code in a separate issue? |
The change can probably be reverted together with a bump of the |
Even with bumping |
It would |
Could you open a fresh PR? |
Partial revert of 09cb8e7 Keep the test-case and upgrade undici
Fixes #287
Finally found the problem is caused by both
http
andundici
.http
is not always treats theheader
tolatin1
.https://github.com/nodejs/node/blob/2a29df64645a70bbb833298423a29206c4ec6a2e/lib/_http_outgoing.js#L361-L373
undici
always performBuffer.from('').toString('utf8')
regardless theheader
encoding.https://github.com/nodejs/undici/blob/2b260c997ad4efe4ed2064b264b4b546a59e7a67/lib/core/util.js#L216-L229
In combine of the above two problem.
The headers will parsed as a completely difference byte when chaining.
In this fix, we always treats the header as
latin1
.So, we are always consistence in the encoding.
Checklist
npm run test
andnpm run benchmark
and the Code of conduct