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

typecheck in CI #445

Merged
merged 10 commits into from
Mar 18, 2021
Merged

typecheck in CI #445

merged 10 commits into from
Mar 18, 2021

Conversation

Rich-Harris
Copy link
Member

this will fail, but at least it gives us a list of things we need to fix. #441

.github/workflows/ci.yml Outdated Show resolved Hide resolved
@Rich-Harris
Copy link
Member Author

Why is CI not running

Rich-Harris and others added 3 commits March 8, 2021 21:38
@benmccann benmccann mentioned this pull request Mar 18, 2021
This was linked to issues Mar 18, 2021
@benmccann
Copy link
Member

We might want RequestHandler.headers to be of type Headers

@benmccann
Copy link
Member

It might be useful to add a TODO above the Headers definition to update it after microsoft/TypeScript#26797 is released hopefully in TypeScript 4.3 to update the definition to something like:

{
  [key: string]: string;
  'set-cookie': string | string[]
}

@Conduitry
Copy link
Member

Would shipping types that require a newer version of TS be considered a breaking change? How do projects under semver generally handle the implied TS peerdep that goes along with their published types?

@benmccann
Copy link
Member

I suppose it might be. Probably still worth making a note of it even if we want to hold it for a major release

@Rich-Harris Rich-Harris merged commit 8326695 into master Mar 18, 2021
@Rich-Harris Rich-Harris deleted the typechecking branch March 18, 2021 16:00
@dummdidumm
Copy link
Member

I would consider that a breaking change. But it's possible to specify type definitions for certain TS version ranges, which could work if the type is not completely changed, just better typed in a more recent version.

@Rich-Harris
Copy link
Member Author

If we change the type from

Record<string, string>

to

Record<string, string> & { 'set-cookie': string | string[] }

then it wouldn't break anyone's app, even if they do typechecking, because they would only have been able to return a single set-cookie header as a string anyway. So I'm personally not concerned

@Rich-Harris
Copy link
Member Author

ah ok i just reread and you're talking about requiring a new TS version, not changing the types. yeah i guess that is a breaking change. still not concerned though because of what @dummdidumm said

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.

Headers cleanup Typecheck in CI
4 participants