fix: change set-cookie header to always be an array

[belochub]

For compatibility with the Node.js API set-cookie header, if present,
has to always be an array, even when there is only one value in it.

For reference, see https://nodejs.org/api/http.html#http_message_headers

There is also a possible compatibility problem with the way duplicate headers are handled right now since you always collect all duplicates to an array, but Node.js discards duplicates of some of the headers and joins the duplicate values with ', ' or '; ' for others. This can be fixed separately if you want full compatibility with Node.js, but for now, I only wanted to handle one issue that I discovered while using this package.

[ronag]

I think the original thought here was that undici considers as little user land edge cases as possible and assumes what is provided is correct (or at least correct in terms of the protocol) and leave these kind of compat logic to higher level libraries, e.g. we don't check that method is valid (though maybe we should at least check it is all uppercase and doesn't contain any invalid characters?).

That being said I'm not opposed to including these kind of things. @mcollina WDYT?

[ronag]

@delvedor @szmarczak

[mcollina]

unfortunately lgtm

[ronag]

I'm having some second thoughts on this. It feels a bit like a rabbit hole. If we do this should we implement all the other weird Node behaviors as well? Where do we stop?

Would like to think about this a bit more. Not sure adding edge cases is a good idea.

[ronag]

I don't think this is the right direction. If we go down this path we'll end up back where we started with Node's existing "strange" behaviors.

[ronag]

Anyone disagreeing to my objection or can we close this?

[mcollina]

no, not really

[belochub]

@mcollina @ronag I would like to clarify, to explain my idea for this PR.
I discovered the problem when using fastify-reply-from with unidici enabled in TypeScript. When passing rewriteHeaders() option, the way it is typed right now, that function accepts Node.js http.IncomingHttpHeaders type, which specifies that the value for set-cookie header is an array of strings, but as of right now, when undici is enabled in fastify-reply-from, that makes the provided types incorrect due to this and other possible differences related to header collection this package has from the Node.js API.
My first thought was to open an issue in the fastify-reply-from repository, but after looking into the details, I noticed that the issue was actually in the way this package is handling headers, and seeing that according to #312 you want this to possibly become a replacement for http.request, I thought that maybe it would make sense to notify you of this issue, and maybe fix this one specific difference that stands out the most. But I now see that I misunderstood the idea and that this package does not have to have the same API and behavior as Node.js API has.
I guess, the problem I discovered should be fixed in fastify-reply-from instead, but in my opinion, the current behavior when this header can be either a single string or an array of strings, depending on the number of times it is present in the response, is still less convenient than always having it presented as an array.

[ronag]

@belochub Thanks for the description. I think this should be resolved in fastify-reply-from.

is still less convenient than always having it presented as an array.

Depends... for most headers it's more convenient to have it as a string. set-cookie is a little bit of an exception IMO.

[belochub]

Depends... for most headers it's more convenient to have it as a string.

@ronag I agree, that for most headers this is better, thus the current behavior of this package, combining duplicate headers into an array instead of a string, might be an inconvenience for some users.