fix: normalize the usage of headers

[illright]

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

• It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
• This message body should clearly illustrate what problems it solves.
• Ideally, include a test that fails without this PR but passes with it.

Tests

• Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

• If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpx changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

This will normalize the usage of headers across the server-side fetch to ensure that Record<string, string>, string[][] and Headers are all treated correctly. Previosly, as described in #3009, whenever headers in the form of Headers would come to the server-side fetch, the would be lost under a certain condition.

This only fixes one of the problems in #3009, the other one about the hostnames not being compared correctly still persists.

[changeset-bot]

🦋 Changeset detected

Latest commit: fcb6d34

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@sveltejs/kit	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[illright]

I couldn't figure out how to resolve an error that was preventing me from running tests, so I haven't seen this error before. I'm guessing that the build is failing due to node-fetch being imported somewhere where it shouldn't be imported, but I'm honestly not sure how to approach fixing this error. Help is highly appreciated.

[benmccann]

PR to address the test failure here: sveltejs/vite-plugin-svelte#225

[illright]

@benmccann looks like the PR you mentioned is merged. Is there anything I can do to speed up the release of these changes? They address the issue that's blocking one of my projects that I must release soon.

[bluwy]

@illright We'll cut a new release for vite-plugin-svelte soon. There's some CI issue there as well, which will be solved by sveltejs/vite-plugin-svelte#226. Once that gets approved, we can update kit with that and rebase this PR.

[illright]

For a second thought, it's not a blocker, I can simply disable SSR for now, everything works in CSR mode. But this PR still remains very desirable for me. Just letting you know that there is no rush.

In the meantime, could you please take a look at the code and let me know if everything looks alright there?

[bluwy]

I'm not well-versed in this area of the code so I'll leave the others to review this (Not sure if supporting the Headers class a common thing). But at the meantime, master has updated to the latest vite-plugin-svelte, so a rebase should fix the CI issue.

[illright]

Here's the background for this issue – I'm using ky as the API client in my application. It handles header management, among other things, and passes a Headers instance to the underlying fetch, and I have no control over that. So I imagine that to all other users of ky this is desirable to support.

[Rich-Harris]

Thank you! We can make this quite a bit simpler, I think — new Headers(old_headers) will work whether old_headers is a POJO or an instance of Headers, and response.headers is always guaranteed to be an instance of Headers. Because of that we don't need any new helper functions. I've updated the PR

[benmccann]

this makes it sounds like you could pass either. is that correct or only a Headers instance is accepted now?

[illright]

This test was written by Rich, but I believe that both should be accepted, otherwise we'd make a breaking change

[Rich-Harris]

both should be accepted, because we're just mirroring the native fetch API (until this PR, i didn't realise you could pass a Headers object; turns out fetch accepts either)

[illright]

@Rich-Harris are you sure about this? It seems to me that, according to the Fetch Standard, the Headers class is only required to accept a POJO or an array of entries, nothing is mentioned about it accepting an instance of itself.

The reason I'm concerned is that SvelteKit doesn't ship an implementation of Headers, so for Node, we cannot guarantee that the people will use an implementation that is capable of this somewhat non-standard behaviour. But I agree that this makes the code quite a bit more bloated. Maybe we could somehow expose Headers from SvelteKit to allow users to ensure that they are using the correct implementation?

[Rich-Harris]

Yep! It'll accept any iterable that yields string pairs:

stuff = {
  [Symbol.iterator]: function* () {
    yield ['problems', '99'];
    yield ['answer', '42'];
  }
};

const headers = new Headers(stuff);

headers.get('problems'); // '99'
headers.get('answer'); // '42'

That includes Headers, Map and so on. If anyone is calling fetch with a Headers object that doesn't implement the iterable protocol, their code is broken and that's very much on them.

SvelteKit exposes Headers from node-fetch during development. In production, the environment is expected to provide a standards-compliant fetch implementation (adapter-node and others do this via node-fetch, but many environments like Deno etc already provide it).