Closed tremby closed 11 months ago
Looking at undici's code now, it looks like their IncomingHttpHeaders
type is not from node:http; it's homegrown. It's just Record<string, string | string[] | undefined>
. That's not compatible with urllib's (node:http's) IncomingHttpHeaders
type.
http server can response set-cookie
headers many times.
Yes, I know.
I'll try to explain differently.
If the server sends more than one set-cookie
header, res.headers["set-cookie"]
is an array of strings. This is fine.
If the server sends just one set-cookie
header, res.headers["set-cookie"]
is a single string. This disagrees with the typescript type associated with res.headers["set-cookie"]
, which is string | undefined
(which it gets from IncomingHttpHeaders
from node:http
). string[]
is not assignable to string | undefined
. So either the types are wrong, or there is a bug in the code. This means I can get errors in my code which Typescript cannot see.
Undici can also return a single string for that property, but that is not a problem since it reports the types differently. It associates type string | string[] | undefined
with that property. It is not using IncomingHttpHeaders
from node:http
, but rather its own type which it also calls IncomingHttpHeaders
. The two IncomingHttpHeaders
types are not compatible.
I would follow the undici IncomingHttpHeaders fix it.
https://github.com/nodejs/undici/blob/b0d3ca7701766b6cb9ebb8fed7dd08d3a684b91b/types/header.d.ts#L4
the undici IncomingHttpHeaders is a simple version of node:http IncomingHttpHeaders, I think the node:http IncomingHttpHeaders is better and more types defines.
I agree. If you see the issue I linked to above, you'll see that at some point they planned to switch to node's type, but as far as I can tell they never got around to it.
But the fact is, what you're providing on res.headers
is undici's type, but in your types you're declaring that it's node's type. You either need to fix the data so it matches the node type, or change the type declarations to be the same as undici's. Maybe you can import the relevant type straight from undici rather than recreating it; that way you shouldn't then need to change it again once they change their types.
I suppose a 3rd option would be to provide a patch to undici so that they are using node's types. But they wouldn't release that until a major version change.
https://github.com/node-modules/urllib/pull/471 will close it. @tremby Thanks for the big help message.
I'm making a response which is getting either zero or one or two
set-cookie
headers back. I need to do something with them.The broken cases are when exactly one
set-cookie
header was in the response.Looking at node's
IncomingHttpHeaders
type via package@types/node
, it seems it has various properties defined as optional and typestring | undefined
, and one,set-cookie
in particular, is optional and typestring[] | undefined
. On top of that it allows any other arbitrary properties and they are of typestring | string[]
.It seems to me that either there's an upstream bug (in undici?), or some special handling needs to be added to HttpClient.ts for the set-cookie header.