Conversation
| // | ||
| // in order to presist the encoding, always encode it | ||
| // back to latin1 | ||
| function patchUndiciHeaders (headers) { |
There was a problem hiding this comment.
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.
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
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
5 participants
Fixes #287
Finally found the problem is caused by both
httpandundici.httpis not always treats theheadertolatin1.https://github.com/nodejs/node/blob/2a29df64645a70bbb833298423a29206c4ec6a2e/lib/_http_outgoing.js#L361-L373
undicialways performBuffer.from('').toString('utf8')regardless theheaderencoding.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 testandnpm run benchmarkand the Code of conduct