-
Notifications
You must be signed in to change notification settings - Fork 525
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
bug: spreading Headers
object has breaking behavior in versions between Node 20.12 and 20.13
#3267
Comments
Reported tangentially by MSW users as well. |
You are using the spread operator incorrectly. |
just use // Comment out this comment and have the latest undici installed just to verify it's still
// present in latest Undici, but for the test you can use the global Undici bundled in Node
// import { fetch } from "undici";
{
const baseHeaders = { "x-foo": "bar" };
const requestHeaders = new Headers({ "Content-Type": "application/json" });
const headers = {
...baseHeaders,
...Object.fromEntries(requestHeaders),
};
console.log("Result", headers);
await fetch("https://google.com", {
method: "POST",
headers,
});
} |
Yes I know I can do that, (and I did work around the issue in pingdotgg/uploadthing#813) - but that's working around the issue that the behavior changed in a minor Node.js release (havent tracked down what exact Undici release the behavior changed in) |
From a quick look at the behaviors in browsers, I think that the correct implementation is: const baseHeaders = { "x-foo": "bar" };
const requestHeaders = new Headers({ "Content-Type": "application/json" });
const headers = {
...baseHeaders,
...Object.fromEntries(requestHeaders),
}; Note that the following does not work in browsers either: const baseHeaders = { "x-foo": "bar" };
const requestHeaders = new Headers({ "Content-Type": "application/json" });
const headers = {
...baseHeaders,
...requestHeaders,
};
console.log("Result", headers); It will result in not having Having said that, I think it should not crash but works as expected & similarly to browsers. @KhafraDev ping |
@mcollina, yeah, you are right. Headers instance itself won't spread, it will return an empty object. But neither will it throw, so that behavior is unexpected although useful as it lets the user know they aren't doing anything (the error is not hinting at that, though). |
Oh wow I didnt even realize that. Was too focused looking at the Symbols xD |
This was caused by fd66cf0 which exposed the issue. Our only choice is to move towards private properties I think. |
Bug Description
Reproducible By
Run the code in Node 20.12 and 20.13. 20.12 works fine, while 20.13 panics with
TypeError: Could not convert argument of type symbol to string
.Expected Behavior
It to work like it did in 20.12 - afterall I didn't bump any major version, so if this is an intentional breaking change on Undici's side it shouldn't have been included in a minor version of Node.js.
Logs & Screenshots
Environment
Additional context
A separate issue for Node perhaps, but spreading the Headers object probably shouldn't even include the
Symbol(headers list)
to begin with, this is how it looks in the browser for example:The text was updated successfully, but these errors were encountered: