kit icon indicating copy to clipboard operation
kit copied to clipboard

Allow Developers to Set Multiple Headers with the Same Name Using `event.setHeaders`

Open ITenthusiasm opened this issue 2 years ago • 2 comments

Describe the problem

According to the docs,

Setting the same header multiple times (even in separate load functions) is an error — you can only set a given header once.

This isn't too big of a deal, and it at least helps prevent developers from accidentally running into redundancy or fake "bugs". However, I feel like there may be scenarios where a developer (or some package that they employ) would want to return the same header multiple times. This is a use case worth supporting (unless Svelte Kit absolutely knows with 100% certainty that such a use case would never appear). But right now event.setHeaders cannot handle this use case.

Describe the proposed solution

It should be sufficient for event.setHeaders to accept Record<string, string | string[]> instead of Record<string, string>. Whenever an array is passed as an object property's value in event.setHeaders, that could be used to indicate that a developer (or some package which they are using) wants to return multiples of a given header. Regular string values would result in the header only getting set once -- as things are today. I would assume that whatever mapping logic Svelte Kit is currently doing should be able to accommodate for that fairly easily.

I'm assuming that no bugs would come from this implementation given that Svelte Kit already has guards in place for event.setHeaders.

Alternatives considered

No response

Importance

nice to have

Additional Information

No response

ITenthusiasm avatar Jan 16 '23 15:01 ITenthusiasm

Could you give a practical use case example where this would be benefitial to have?

dummdidumm avatar Jan 16 '23 21:01 dummdidumm

My main concern is similar to another issue that I recently opened; it relates to authentication. There are authentication-related packages (e.g., SuperTokens) that often need to return headers and cookies; and sometimes a header needs to be set ("appended") multiple times for the tool to work correctly. In these instances, the developer doesn't have any control over what the tool does (regarding headers), so they simply would not be able to use Svelte Kit.

There are probably other use cases where it could be helpful to set headers multiple times outside of auth packages, but I don't have enough backend experience to know what those situations would be. Even so, it makes sense to allow, right? I feel like the general rule of thumb should be to avoid restricting behavior unnecessarily. For instance, it would be odd if the Headers implementation had the append method removed simply because it didn't seem necessary for devs to the authors. (I understand there's the whole matching the API spec deal; that was just for the sake of example.)

ITenthusiasm avatar Jan 17 '23 14:01 ITenthusiasm

@dummdidumm Just pinging in case this got missed. I saw the label so I didn't know if you were awaiting a ping or not.

ITenthusiasm avatar Jan 19 '23 17:01 ITenthusiasm

The cookies header should be set through the cookies API. Which other headers would be set multiple times? Still having a hard time to understand how this would be a problem in practice for libraries. Is there a current issue with one of them?

dummdidumm avatar Jan 19 '23 20:01 dummdidumm

Which headers they'd be setting, I'm not certain about. :confused: The SuperTokens codebase is a little difficult to dig around. But I did see that at times they set certain response headers multiple times -- not just the Set-Cookie header (they have a separate method for setting cookies). And since there are scenarios where they're returning multiple headers, I need to be able to set multiple headers whenever they require it. I'm trying to leverage a reusable function to handle the headers/cookies they send back by automatically mapping their returned cookies and automatically mapping their returned headers in a way that Svelte Kit supports. And I don't want to run into a situation where there are one-off bugs hidden somewhere because they (the SuperTokens package) expected the app to return multiple headers to the browser when none were returned.

Is there a reason to block setting multiple headers though? Like is it intentionally being disallowed or did it just so happen that it wasn't possible to set multiple headers in the implementation that came out?

ITenthusiasm avatar Jan 19 '23 20:01 ITenthusiasm

The API is the way it is because setting the same header from different places is likely an error. Because multiple loads happen concurrently, it's potentially also non-deterministic.

I'm not going to lie, the API you're dealing with sounds pretty confusing. I don't know that I'd personally choose to use a library whose return values I couldn't reason about or make guarantees about. But if you need to deal with arbitrary (and potentially duplicated) headers that you don't control, then my suggestion would be to attach them to event.locals and deal with them in handle.

Rich-Harris avatar Jan 21 '23 12:01 Rich-Harris

The API is the way it is because setting the same header from different places is likely an error. Because multiple loads happen concurrently, it's potentially also non-deterministic.

Yeah that makes sense. I wasn't advocating for

event.setHeaders("my-header", "first value");
event.setHeaders("my-header", "second value");

but rather

event.setHeaders("my-header", ["first value", "second value"]);

with the same rule that setting a single header multiple times would throw an error. So this would fail:

event.setHeaders("my-header", ["first value", "second value"]);
event.setHeaders("my-header", "third value");

and returning a given header with a single value would still pass, as long as it was only done once.

event.setHeaders("my-header", "only-value");

I was assuming this would be a fairly simple [and reasonable] change. Is that not the case? Svelte Kit could still throw the same errors and apply the same rules and everything. It's just that event.setHeaders would support an array of values (again, set only once).


I'm not going to lie, the API you're dealing with sounds pretty confusing.

That's fair. Unfortunately there aren't a lot of options for what I need to do. :confused: I could perhaps try to find a way to force things towards handle, but I feel like it would make more sense to handle multiple values in an array (in a single place, in a single call), no?

ITenthusiasm avatar Jan 21 '23 13:01 ITenthusiasm

In that case, you could just do this:

event.setHeaders({
  "my-header": ["first value", "second value"].join(', ')
});

We could add that logic to Kit itself, but this feels like an edge case

Rich-Harris avatar Jan 21 '23 13:01 Rich-Harris

Yes. When I tried that, what I saw in the Network tab was something like

my-header: first value, second value

But when I used Headers.append I saw this in the Network tab:

my-header: first value
my-header: second value

So at least as far as the Network Tab was concerned, there seemed to be a difference. Does the Browser functionally consider both of the results seen in the Network Tab in the same way though?

ITenthusiasm avatar Jan 21 '23 13:01 ITenthusiasm

With the exception of Set-Cookie, any HTTP header that can have multiple values can be send as multiple headers or a single header with comma-separated values; the result is the same. In fact, because the standard Headers object only has a get method, these are identical...

headers = new Headers();
headers.set('cache-control', 'public, max-age=10');
headers.get('cache-control'); // public, max-age=10

headers = new Headers();
headers.append('cache-control', 'public');
headers.append('cache-control', 'max-age=10');
headers.get('cache-control'); // public, max-age=10

...so in the case of some SvelteKit adapters (where the headers object need to be converted into res.setHeader calls, and we can't recover the original 'intent') there's no difference even at the network level.

Rich-Harris avatar Jan 21 '23 14:01 Rich-Harris

Ah, okay cool. In that case yeah, just doing a .join works fine. Thanks again for the clarity and patience 🙏🏾

ITenthusiasm avatar Jan 21 '23 14:01 ITenthusiasm

In that case, you could just do this:

event.setHeaders({
  "my-header": ["first value", "second value"].join(', ')
});

We could add that logic to Kit itself, but this feels like an edge case

Another use case would be sending Link headers for multiple assets/preconnect.

Link: <https://www.googletagmanager.com>; rel="preconnect"
Link: <https://www.somecdn.com>; rel="preconnect"

Currently using the .join(', ') method, but I feel like it could be added to SvelteKit logic.

frederichoule avatar Jul 14 '23 11:07 frederichoule