Implement the requirement of IncomingHttpHeaders["set-cookie"]#1893
Implement the requirement of IncomingHttpHeaders["set-cookie"]#1893pan93412 wants to merge 1 commit intonodejs:mainfrom
Conversation
Currently `headers["set-cookie"]` does not split, which can't match the expected type of IncomingHttpHeader. This commit attempts to split `headers["set-cookie"]` by `;` like what 'node:http' does: https://nodejs.org/api/http.html#class-httpserverresponse Fixed nodejs#1892 Signed-off-by: pan93412 <[email protected]>
|
Note that there are at least 1 unmatched behavior of IncomingHttpHeaders:
Besides, I found another inconsistent part between current's According to the However, Lines 223 to 227 in 196c4da …which will break the type declaration of As |
Codecov ReportBase: 90.33% // Head: 90.33% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #1893 +/- ##
==========================================
- Coverage 90.33% 90.33% -0.01%
==========================================
Files 70 70
Lines 6045 6050 +5
==========================================
+ Hits 5461 5465 +4
- Misses 584 585 +1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
|
I'm a bit lost with this PR. The original issue refers a typescript problem. This change alters the current behavior in code to match the types. Let's do the other way around: the types should follow what the code implements. This change can ship in v5. A breaking change to match the RFC or the current behavior of node is ok, but it will have to ship on v6. |
Sure, let me fix the type to match what |
According to core/util@parseHeaders, the current behavior is: - By default, the entry type is a `string`. - The type turns to a `string[]` when there are duplicated entries. This behavior is not as same as what `node:http` does currently; therefore, `IncomingHttpHeaders` can't reflect our parsed data. This commit attempts to reflect this parsing behavior by redefining it to `Record<string, string | string[]>`. Fixed nodejs#1892 The simpler solution of nodejs#1893 Signed-off-by: pan93412 <[email protected]>
|
I found my way here by way of being confused that I think the specific problem in my case is a bug in urllib which I reported as node-modules/urllib#467. (I'm using that package only because I need HTTP digest auth, and that was the first working implementation I could find. Open to suggestions.) Urllib makes what I believe is a flawed assumption that the So this is just me adding my upward thumb to the idea of changing the behaviour and types to match node's in a future version. Having to check whether it's nullish, or an array, or a string, and handle each appropriately, is a needless pain point. |
This relates to...
Fixed #1892
Rationale
Currently
headers["set-cookie"]does not split, which can't match the expected type of IncomingHttpHeader.Changes
This commit attempts to split
headers["set-cookie"]by;like what 'node:http' does: https://nodejs.org/api/http.html#class-httpserverresponseBug Fixes
See above.
Breaking Changes and Deprecations
set-cookieis now always anstring[].Status
KEY: S = Skipped, x = complete