types: fix misleading Headers type definition

[ignatiusmb]

Currently, Headers is set such that its value will always be a string, which gives the assumption that it will always exists even if it's just an empty string. But, this is not the case, especially when one is checking for cookies in request.headers.cookie, in which it can be undefined and the same applies to other headers.

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

[dummdidumm]

I think it's better to rely on TypeScript's compiler flags for this. People who want to have this level of strictness should probably enable this flag: https://www.typescriptlang.org/docs/handbook/release-notes/typescript-4-1.html#checked-indexed-accesses---nouncheckedindexedaccess

[ignatiusmb]

It would seem so, but then it'll be enabled for the whole project and might have unintended consequences for other types, as well as array indexing and such. Not really sure what's the best approach usually is, but I feel that baking it to kit's types itself would be better so general TS or even JSDoc users would get that type safety out of the box.

[benmccann]

Would we also need to say that ServerRequest.locals, ReadOnlyFormData.entries, Location.params, LoadInput.context, LoadOutput.props, etc. can also have possibly undefined values? I'm not very experienced in this area, and am mostly coming from Java where null is implied as a possible value everywhere, but I think I'd lean towards @dummdidumm's suggestion to avoid littering the code with |undefined everywhere

[ignatiusmb]

Not really, that shouldn't be the case at all. All of those mentioned can (now, thanks to #1447) be typed by the user, with the exception of entries in ReadOnlyFormData which should always return a non-nullish value (skimming from the source). In the case of Headers, the users have no control of it, and needing to change the compiler options for the whole project to workaround it doesn't feel right. From what I can tell, Headers should be the only place we need to guard from possibly undefined properties.

Also, the request headers do actually returns undefined in the key-pair values, so anyone parsing these headers that iterates over using the keys will receive an actual string | undefined values at runtime. But, they'll only be expecting a string and this can trip up someone when they immediately access string prototypes and get TypeErrors.

---

Additional context from #1369 where it's necessary to guard endpoint output with | undefined, but just with a different syntax using Partial to not disrupt the other types using Headers.

This would also be a nice preliminary for the upcoming --strictOptionalProperties as mentioned in microsoft/TypeScript#44421.

[dummdidumm]

After some thought I think I'm now in favor of this change. If some headers could be set (property exists on the object) but be undefined, then the types should reflect that.

[orta]

Personally I wouldn't do it, this is an opt-in feature for codebases via the noUncheckedIndexedAccess

[ignatiusmb]

What about object with actual undefined values in it that exists on runtime as well, should that really be left to noUncheckedIndexedAccess flag that the users need to manually opt-in? I'm not sure I understand the rationale here. Wouldn't it be safer and more convenient if we guard it directly from kit's type?

[dummdidumm]

If headers can look like

{
   foo: 'hi',
   bar: undefined
}

Then I think it's probably better to explicitely type it that way. There's a difference between something not being defined as a property and something being defined as a property which is set to undefined

[ignatiusmb]

That is indeed the case I encountered, this is an actual headers I received (a production app, and not just once) in the log

{
  authorization: undefined,
  cookie: undefined
}

It might be cached from previous requests/responses, but nonetheless, it still sends those through handle and endpoints.

[benmccann]

I'm surprised by

{
  authorization: undefined,
  cookie: undefined
}

Where would that be coming from? If it's something internal, then I think maybe I'd rather change that because it's a weird thing to get. Like at that point you could list all possible headers with undefined value

[changeset-bot]

🦋 Changeset detected

Latest commit: b8d7819

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

[ignatiusmb]

I can't pinpoint it enough to the internal stuff, but here's where it's coming from and what I think is happening.

The application sets (and removes) the cookie header in the endpoint to utilize the httpOnly flag, nothing else other than the endpoints tampers with it. The authorization header is passed to a client-side fetch targeting an external api url, no connection to any endpoints.

My guess is that it was cached even after the user logs out (cookie removed and no fetch calls with authorization header). I haven't tested it again, but if my guess is correct, any arbitrary headers that's returned from an endpoint or passed with a fetch call would also show up in handle hooks.

[benmccann]

Hmm, this is a tough one. But since I don't think it can come from a browser request and only from the developer's own endpoint and because we have noUncheckedIndexedAccess I think I'm going to go ahead and close this one