Skip to content
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

Closed
juliusmarminge opened this issue May 16, 2024 · 9 comments · Fixed by #3269
Labels
bug Something isn't working fetch

Comments

@juliusmarminge
Copy link

juliusmarminge commented May 16, 2024

Bug Description

Reproducible By

// 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,
    ...requestHeaders,
  };
  console.log("Result", headers);

  await fetch("https://google.com", {
    method: "POST",
    headers,
  });
}

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

CleanShot 2024-05-16 at 12 00 10

Environment

  • Latest MacOS
  • Node 20.13

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:

CleanShot 2024-05-16 at 12 04 29

@kettanaito
Copy link
Contributor

Reported tangentially by MSW users as well.

@tsctx
Copy link
Member

tsctx commented May 16, 2024

You are using the spread operator incorrectly.
In Headers you need to use it with arrays, not objects.

@tsctx
Copy link
Member

tsctx commented May 16, 2024

just use Object.fromEntries.

// 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,
  });
}

@juliusmarminge
Copy link
Author

juliusmarminge commented May 16, 2024

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)

@mcollina
Copy link
Member

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 Content-Type in headers object.

Having said that, I think it should not crash but works as expected & similarly to browsers.


@KhafraDev ping

@kettanaito
Copy link
Contributor

@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).

@juliusmarminge
Copy link
Author

juliusmarminge commented May 16, 2024

It will result in not having Content-Type in headers object

Oh wow I didnt even realize that. Was too focused looking at the Symbols xD

@juliusmarminge
Copy link
Author

juliusmarminge commented May 16, 2024

Checked some other runtimes:

CleanShot 2024-05-16 at 13 04 57

CleanShot 2024-05-16 at 13 04 17

@mcollina mcollina added the good first issue Good for newcomers label May 16, 2024
@KhafraDev KhafraDev added fetch and removed good first issue Good for newcomers labels May 16, 2024
@KhafraDev
Copy link
Member

This was caused by fd66cf0 which exposed the issue. Our only choice is to move towards private properties I think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fetch
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants