-
Notifications
You must be signed in to change notification settings - Fork 597
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: always parse headers #3408
base: main
Are you sure you want to change the base?
Conversation
@metcoder95 @mcollina I have some http2 and mock test failures I haven't been able to figure out. Would appreciate some help. |
8160369
to
2666cdc
Compare
Changes onHeaders and onComplete headers/trailers to always be a pre-parsed headers record.
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.
Shall we update the docs to state the change?
LGTM, I'm having a hectic morning; I'll see the H2 issues throughout the day and see if I can submit a PR or hint you the issue.
Where how? I tried updating docs. Did I miss something? |
This dispatcher handlers will start receiving the |
Will check the H2 issues throughout the day |
I think I managed to fix the failures |
greetings some test errors appear I wonder if these could be flakky? |
I don't believe they are flaky as they are pretty consistent across jobs: https://github.com/nodejs/undici/actions/runs/9980661941/job/27582582584?pr=3408#step:8:2242 Seems to be related with |
} else { | ||
const headersValue = headers[i + 1] | ||
if (typeof headersValue === 'string') { | ||
obj[key] = headersValue | ||
} else { | ||
obj[key] = Array.isArray(headersValue) ? headersValue.map(x => x.toString('utf8')) : headersValue.toString('utf8') | ||
obj[key] = Array.isArray(headersValue) ? headersValue.map(x => x.toString('latin1')) : headersValue.toString('latin1') |
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.
@metcoder95 @mcollina PTAL
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.
Adjusted the tests to align with the change
It seems CI is still not happy... |
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.
We should update the global dispatcher API version number.
Ideally it would be best if we could provide a polyfill for the v1 API, so that using latest undici on old node would still work.
Changes onHeaders and onComplete headers/trailers to always be a pre-parsed headers record.