http2: add headersDistinct to Http2ServerRequest fixes #64060#64071
http2: add headersDistinct to Http2ServerRequest fixes #64060#64071Shivay-98 wants to merge 1 commit into
Conversation
|
Review requested:
|
nodejs-github-bot
added
http2
pimterry
left a comment
pimterry
left a comment
There was a problem hiding this comment.
Nice work, thanks @Shivay-98!
There's a few JS linting issues to fix here (you can check locally with make lint). For the commit linting issue, you'll need to run git commit --amend -s to update the commit with the signed-off signature, and then force-push to update the commit here.
It'd be good to also include this change in the docs, in the compat API section in http2.md.
| const kSetHeader = Symbol('setHeader'); | ||
| const kAppendHeader = Symbol('appendHeader'); | ||
| const kAborted = Symbol('aborted'); | ||
| const kHeadersDistinct = Symbol('kHeadersDistinct'); |
There was a problem hiding this comment.
Nit: for symbols, generally the name doesn't have the k prefix - it's just the meaningful string
Ethan-Arrowood
left a comment
Ethan-Arrowood
left a comment
There was a problem hiding this comment.
Agree with @pimterry 's review. Once you fix lint and address the nits this is good to go.
| const raw = this[kRawHeaders]; | ||
|
|
||
| for (let i = 0; i < raw.length; i += 2) { | ||
| const name = String(raw[i]).toLowerCase(); |
There was a problem hiding this comment.
The String() might not be necessary.
Signed-off-by: Shivay-98 <shivay8306@gmail.com>
|
Thanks @pimterry @Ethan-Arrowood for the review! I've addressed the requested changes:
I'd appreciate it if you could take another look when you have a chance. Thanks again! |
pimterry
left a comment
pimterry
left a comment
There was a problem hiding this comment.
There's just a couple more changes to do on the docs here. There's also still a linting failure on the code.
You can run make lint to run all the linting checks locally before pushing, which will make these easier to catch. You might also want to run make docserve which will build & serve the docs locally, so you can check the output.
| @@ -4187,6 +4189,53 @@ removeAllHeaders(request.headers); | |||
| assert(request.url); // Fails because the :path header has been removed | |||
| ``` | |||
|
|
|||
| #### `request.headersDistinct` | |||
There was a problem hiding this comment.
The docs here are duplicated. One of these needs removing.
|
|
||
| * Type: {Object} | ||
|
|
||
| Similar to [`request.headers`][], but there is no join logic and the values are |
There was a problem hiding this comment.
This link name is defined in http.md but not here in http2.md.
If you want to keep it in the de-dupe, we'll need another definition here to make the link work. You can see those at the bottom of the file, just add this in there alphabetically.
| <!-- YAML | ||
| added: | ||
| - v18.3.0 | ||
| - v16.17.0 |
There was a problem hiding this comment.
In whichever doc you keep, the version number here should be left as a placeholder. It'll be automatically filled out when this is released, like so:
added:
- version: REPLACEME
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #64071 +/- ##
==========================================
- Coverage 92.01% 90.22% -1.80%
==========================================
Files 379 741 +362
Lines 166972 241713 +74741
Branches 25554 45543 +19989
==========================================
+ Hits 153639 218081 +64442
- Misses 13041 15163 +2122
- Partials 292 8469 +8177
🚀 New features to boost your workflow:
|
4 participants
Adds headersDistinct to Http2ServerRequest to improve parity with
http.IncomingMessage.
The implementation mirrors IncomingMessage.headersDistinct by
lazily constructing a null-prototype object from rawHeaders and
preserving all header values as arrays.
Includes tests covering:
fixes
headersDistinctfor http2 requests #64060