fix: ensure variance of types matches how values are used#2786
fix: ensure variance of types matches how values are used#2786IvanGoncharov merged 6 commits intographql:masterfrom
Conversation
I've written up detailed reasoning in dotansimha/graphql-typed-document-node#38 I didn't remove the "FIXME" comment, as I'm not sure what you would prefer to do with it. Nominal typing wouldn't actually solve anything here, because the variables and responses are plain objects, not nominal types. What's needed is just accurate structural typing. I really don't view this as a hack because: 1. We're not lying about any of the types that exist at runtime 2. The `__apiType` property does accurately document/describe the API behavour 3. The type errors TypeScript would produce if you use these `TypedQueryDocumentNode`s incorrectly matches up with the issues you would see at runtime.
@ForbesLindesay I'm ok with merging it but if we declare a non-void property that is never set it is a clear hack in my book 😄
In Nominal type systems types with different template arguments treated as incompatible types, e.g. C++, Java, etc.
The name of the field should reflect its purpose so if I get an error message I don't need to look into |
@ForbesLindesay Please add test for this here: |
|
@dotansimha this bugfix blocks |
|
Hi @IvanGoncharov , sorry for the delay! I will look into this tomorrow :) |
|
@ForbesLindesay I think you are right here, I did some checks and it seems like the covariant might cause inconsistency here. @IvanGoncharov From my point of view, I think it's a better way to declare the usage of the generics, and it doesn't really matter since it's unused, same as the existing solution. |
|
@dotansimha @ForbesLindesay Can you please rename |
|
@ForbesLindesay I created a PR for your branch that includes what @IvanGoncharov asked for. |
Co-authored-by: Ivan Goncharov <[email protected]>
|
@ForbesLindesay This is another PR for linting errors :) |
|
@IvanGoncharov I think it is ready to merge? |
|
@ardatan Thanks a lot 👍 |
…ription * master: (211 commits) Update deps (graphql#2844) resources: use named groups in RegExp (graphql#2840) TS: exclude integration tests from root tsconfig.json (graphql#2838) Flow: remove support for measuring Flow coverage (graphql#2837) CI: test on node 15 (graphql#2836) Update deps (graphql#2835) build: add support for experimental releases (graphql#2831) 15.4.0 Update deps (graphql#2827) Update deps (graphql#2825) integrationTests: add Flow test (graphql#2819) fix: ensure variance of types matches how values are used (graphql#2786) Cleanup '__fixtures__' leftovers (graphql#2818) Convert fixtures to be JS files (graphql#2816) Update deps (graphql#2815) benchmark: extract benchmarks into separate folder and run them on NPM package Update deps (graphql#2810) Update outdated documentation (graphql#2806) Make print() break arguments over multiple lines (graphql#2797) Added check for specific symbols in polyfills/symbols (graphql#2804) ...
4 participants
I've written up detailed reasoning in dotansimha/graphql-typed-document-node#38
I didn't remove the "FIXME" comment, as I'm not sure what you would prefer to do with it. Nominal typing wouldn't actually solve anything here, because the variables and responses are plain objects, not nominal types. What's needed is just accurate structural typing. I really don't view this as a hack because:
__apiTypeproperty does accurately document/describe the API behavourTypedQueryDocumentNodes incorrectly matches up with the issues you would see at runtime.