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

feat: Introduce rawHeaders #2435

Closed
wants to merge 33 commits into from
Closed

Conversation

tsctx
Copy link
Member

@tsctx tsctx commented Nov 14, 2023

This relates to...

Fixes #2415

Rationale

Changes

See #2415

Features

Bug Fixes

Breaking Changes and Deprecations

Status

@tsctx tsctx changed the title add: typescript type feat: Introduce rawHeaders Nov 14, 2023
@codecov-commenter
Copy link

codecov-commenter commented Nov 14, 2023

Codecov Report

Attention: 62 lines in your changes are missing coverage. Please review.

Comparison is base (e39a632) 85.54% compared to head (2c9904c) 85.58%.
Report is 94 commits behind head on main.

Files Patch % Lines
lib/handler/RetryHandler.js 70.00% 36 Missing ⚠️
lib/fetch/index.js 66.66% 11 Missing ⚠️
lib/fetch/headers.js 87.50% 5 Missing ⚠️
lib/api/readable.js 80.00% 4 Missing ⚠️
lib/client.js 92.85% 2 Missing ⚠️
lib/core/util.js 92.85% 2 Missing ⚠️
lib/core/request.js 96.55% 1 Missing ⚠️
lib/fetch/request.js 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2435      +/-   ##
==========================================
+ Coverage   85.54%   85.58%   +0.03%     
==========================================
  Files          76       77       +1     
  Lines        6858     7062     +204     
==========================================
+ Hits         5867     6044     +177     
- Misses        991     1018      +27     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tsctx
Copy link
Member Author

tsctx commented Nov 14, 2023

Hi @KhafraDev @metcoder95,
All current tests have passed on my machine.
There is still work to be done (add tests), etc., but could you take a look at it once?
Should I add rawHeaders to onInfo? I would like to hear your opinion.

@KhafraDev
Copy link
Member

Parsing the headers readily will majorly slow down undici

@metcoder95
Copy link
Member

It is quite different from what I would do 🤔
Have you considered just altering this part?

undici/lib/client.js

Lines 1799 to 1801 in ba4ca32

if (request.onHeaders(Number(headers[HTTP2_HEADER_STATUS]), headers, stream.resume.bind(stream), '') === false) {
stream.pause()
}

I would like to keep the h2 specifics as close as possible to the h2 only code (more specifically writeh2.
My idea was to use the statusText and extend it to also support an object.
If h2, it receives the rawHeaders from the response, if http/1.1it remains as string.

By following the approach I suggest, I won't see much of performance penalties, considering that the client is scoped to a single origin, so if the origin accepts h2 and is enabled, it will just remains as h2, and same for http/1.1.

@tsctx
Copy link
Member Author

tsctx commented Nov 14, 2023

@metcoder95
Yes, I thought about changing it there too, but quit because I thought it would add too many arguments.
I would like to do it your way.

@tsctx
Copy link
Member Author

tsctx commented Nov 15, 2023

All that remains is to add tests and modify the documentation.

@tsctx tsctx marked this pull request as ready for review November 15, 2023 13:28
@tsctx
Copy link
Member Author

tsctx commented Nov 15, 2023

Not enough tests have been implemented yet, but Passed all current and added tests. Could you please look at it again?
cc @KhafraDev @metcoder95

@tsctx tsctx marked this pull request as draft November 15, 2023 13:58
Copy link
Member

@KhafraDev KhafraDev left a comment

Choose a reason for hiding this comment

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

I agree with @metcoder95 mostly. This solution seems overly complicated and not correct; there shouldn't be a need to change the non-http2 parts. I do wonder - what benefit is there in having access to the :status header at all? Wouldn't it make sense to remove it with the rest of the pseudo headers, thereby simplifying this a ton?

lib/api/api-pipeline.js Outdated Show resolved Hide resolved
lib/api/api-request.js Outdated Show resolved Hide resolved
@tsctx tsctx closed this Nov 17, 2023
@tsctx tsctx deleted the feat/introduce-rawHeaders branch November 17, 2023 11:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The pseudo-header :status is included in h2 fetch.
4 participants