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

[chore] separate RequestHeaders and ResponseHeaders types #2248

Merged
merged 5 commits into from
Aug 25, 2021

Conversation

benmccann
Copy link
Member

@benmccann benmccann commented Aug 19, 2021

There doesn't seem to be a way to tell TypeScript that only set-cookie can be a string[], so just say that all header values can be string | string[] and deal with doing type assertions internally so that we can make things nice for our users

@changeset-bot
Copy link

changeset-bot bot commented Aug 19, 2021

🦋 Changeset detected

Latest commit: 3c57c7c

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
Copy link
Member

ignatiusmb commented Aug 20, 2021

I know we've discussed this on discord, but man it feels really wrong to type cast everywhere, almost as wrong as using any in TS.

I'm still hoping there's another way around this, how did other frameworks handle this, and how did we get this far with having 'set-cookie' as the only one that can be an array? Can we somehow force it to just be string even more multiple cookies? Can we somehow provide a constrained identity function to work around this?

Edit:

how did we get this far with having 'set-cookie' as the only one that can be an array?

I meant this as "how is there no general Headers type available yet, that isn't a constructor or Map"

@benmccann
Copy link
Member Author

Ok, I added a function to get the header rather than using the type cast

Copy link
Member

@ignatiusmb ignatiusmb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm really conflicted on this one, I hope it's okay if I call in another pair of eyes again 😅 @dummdidumm

Also, I haven't tried this so it might not work, but since we never (or rarely) use the string[] value from headers, we can probably get away with keeping them as string internally, but have them exported publicly as string | string[], so it'll only be loose when used outside of development, importing from @sveltejs/kit.

Copy link
Member

@dummdidumm dummdidumm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm in favor, too. It's pragmatic, and we could change it some time in v2 if TypeScript is then able to type this.
How to handle this internally: I don't know where the public points are where users can use this, but the idea of having a transformation in one place and use string internally sounds nice and a little bit better than the current approach of always using the method. So my approval is more about the external change, and if the internal code can be refactored a bit to make the typings easier that would be great.

@ignatiusmb
Copy link
Member

Alright, sounds good! It may take a while (or never if it isn't possible), but I'll try to think of a way we can achieve having only string internally

@ignatiusmb ignatiusmb merged commit 02f7aba into master Aug 25, 2021
@ignatiusmb ignatiusmb deleted the typescript-todo branch August 25, 2021 11:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants