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: always parse headers #3408

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

fix: always parse headers #3408

wants to merge 7 commits into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Jul 17, 2024

Changes onHeaders and onComplete headers/trailers to always be a pre-parsed headers record.

@ronag
Copy link
Member Author

ronag commented Jul 17, 2024

@metcoder95 @mcollina I have some http2 and mock test failures I haven't been able to figure out. Would appreciate some help.

@ronag ronag added the semver-major Features or fixes that will be included in the next semver major release label Jul 17, 2024
@ronag ronag force-pushed the headers-object branch 2 times, most recently from 8160369 to 2666cdc Compare July 17, 2024 08:42
Changes onHeaders and onComplete headers/trailers to always
be a pre-parsed headers record.
Copy link
Member

@metcoder95 metcoder95 left a 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.

@ronag
Copy link
Member Author

ronag commented Jul 17, 2024

Shall we update the docs to state the change?

Where how? I tried updating docs. Did I miss something?

@metcoder95
Copy link
Member

This dispatcher handlers will start receiving the headers already parsed instead of in a Buffer format, isn't it?

@ronag ronag requested a review from metcoder95 July 17, 2024 09:19
@metcoder95
Copy link
Member

Will check the H2 issues throughout the day

@ronag
Copy link
Member Author

ronag commented Jul 17, 2024

I think I managed to fix the failures

@mertcanaltin
Copy link
Member

I think I managed to fix the failures

greetings some test errors appear I wonder if these could be flakky?

@metcoder95
Copy link
Member

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 fetch and parsing non-Latin1 chars on headers.

} 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')
Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

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

@metcoder95
Copy link
Member

It seems CI is still not happy...

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.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-major Features or fixes that will be included in the next semver major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants