Hey @ronag and @KhafraDev, I've got a pull request up that I'd appreciate your eyes on. Your feedback would be incredibly helpful!

lgtm

as far as I recall, Headers had some special handling logic for cookies. Can you check that everything works correctly with those?

Thank you for noticing potencial issue, will check it out.

Codecov Report

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

Comparison is base (e39a632) 85.54% compared to head (a6913dd) 85.29%.
Report is 277 commits behind head on main.

@@            Coverage Diff             @@
##             main    #2708      +/-   ##
==========================================
- Coverage   85.54%   85.29%   -0.26%     
==========================================
  Files          76       84       +8     
  Lines        6858     7591     +733     
==========================================
+ Hits         5867     6475     +608     
- Misses        991     1116     +125     

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

lgtm

as far as I recall, Headers had some special handling logic for cookies. Can you check that everything works correctly with those?

What I've found is Headers filtering out and forbid access to Set-Cookie header, but this is true only for client-side javascript.
Done some checks and everything in this case seems absolutely fine on server-side for node.

const headers = new Headers()
headers.append('Set-Cookie', 'key=value')
headers.append('Set-Cookie', 'key2=value2')

for (const [key, value] of headers) {
  console.log(key, value) // will print one cookie per row
}

console.log(headers.getSetCookie()) // will return list of cookies

Should I look for any other potential issues with cookies?

Hi, we got a bug report that seems strongly related to this change. I generally think it is a good change, however, the thought struck me that this might have been a breaking change (at least in combination with diagnostics_channel). Should we as library maintainers be more defensive moving forward?

I traced this a bit more and recognized it as related to this PR:
#2874

I think this PR is just related to the chain of updates.

Is there a minimal repro for the issue? It should be fixed on our end.

@KhafraDev What I am referring to is not really a bug, so I am not sure it can be fixed. I am simply saying that this change should potentially have gone into a Node.js major.

But looking at the situation again, Node.js 20 still seems to be on undici 5 so was probably a bit too eager to comment here. As long as this feature is only lands in Node as part of a major, everything is fine. Sorry for the distraction!

I would consider a breaking change in a minor release of undici to be a bug. If you provide a minimal repro I'd be willing to take a look.

@KhafraDev here's a simple repro demonstrating the change that occurred between 6.6.2 and 6.7.0.

import 'undici';
import ds from 'diagnostics_channel';

ds.subscribe('undici:request:create', ({ request }) => {
  console.log(typeof request.headers);
  console.log(JSON.stringify(request.headers));
});

fetch('https://example.com', {
  headers: { 'X-Foo': 'bar' }
});

$ npm install [email protected] && node index.js

changed 1 package, and audited 3 packages in 400ms

found 0 vulnerabilities
string
"X-Foo: bar\r\naccept: */*\r\naccept-language: *\r\nsec-fetch-mode: cors\r\nuser-agent: node\r\naccept-encoding: br, gzip, deflate\r\n"

$ npm install [email protected] && node index.js

changed 1 package, and audited 3 packages in 408ms

found 0 vulnerabilities
object
["X-Foo","bar","accept","*/*","accept-language","*","sec-fetch-mode","cors","user-agent","node","accept-encoding","br, gzip, deflate"]

Yeah, I can confirm this bug; this changes the contract and might be better if we aim it to next major instead.
cc: @ronag @mcollina