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

Use TypeScript assert condition signature for invariant(condition, message) #12

Merged
merged 1 commit into from
Oct 23, 2020

Conversation

PowerKiKi
Copy link
Contributor

This allow TypeScript to know that an assertion was made and that
types can be narrowed down after a call to invariant().

For more details see microsoft/TypeScript#32695

@PowerKiKi
Copy link
Contributor Author

This PR is required if we want to solve apollographql/apollo-client#6073

@benjamn
Copy link
Member

benjamn commented Apr 6, 2020

@PowerKiKi Do you know if using this syntax is safe for applications that use an older version of TypeScript?

@SimenB
Copy link

SimenB commented Apr 6, 2020

It is not, I broke some people on an older version of TS by adding this to Jest. Seems worth it to make a breaking change though, so invariant works also at the type level. And just tell people to use at least TS@3.7

@PowerKiKi
Copy link
Contributor Author

PowerKiKi commented Apr 7, 2020

Correct, we now require TypeScript 3.7. I have no idea how tests did not fail in my local install... I rebased and forced push a new commit including versions increase.

I am wondering whether this should also be declared as peerDependencies in packages/ts-invariant/package.json ?

PS: also notice that I simplified the README, as it is no longer true. And we could argue whether InvariantError should still be part of public API or not. But since apollo-client make use of that, it's probably best to keep it as is for the time being.

@PowerKiKi
Copy link
Contributor Author

@benjamn would you have time to have another look at this?

@PowerKiKi
Copy link
Contributor Author

@benjamn it seems you were interested in this PR earlier this year. And I fixed the issues raised in the meantime. I still think it would add a lot of value to this lib. Would you have time to review/merge it ?

@benjamn
Copy link
Member

benjamn commented Oct 22, 2020

@PowerKiKi Thanks for the ping. I am checking with my team (@hwillson and a few others) to see if we can raise the minimum to 3.7 at this point, and I will report back (hopefully today).

@benjamn benjamn requested review from benjamn and removed request for benjamn October 22, 2020 18:33
@benjamn benjamn self-assigned this Oct 22, 2020
@benjamn benjamn self-requested a review October 22, 2020 18:33
benjamn added a commit that referenced this pull request Oct 23, 2020
Among other important features, this update enables assertion signatures,
as needed by #12. Since we are not ready to require typescript@4, this
commit closes #34 for now.
This allow TypeScript to know that an assertion was made and that
types can be narrowed down after a call to `invariant()`.

For more details see microsoft/TypeScript#32695
@benjamn benjamn changed the base branch from master to main October 23, 2020 13:56
@benjamn benjamn changed the title Let TypeScript know about the assertion made Use TypeScript assert condition signature for invariant(condition, message) Oct 23, 2020
@benjamn benjamn merged commit 1bada70 into apollographql:main Oct 23, 2020
@SimenB
Copy link

SimenB commented Oct 23, 2020

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants