fix: ensure variance of types matches how values are used#38
Merged
dotansimha merged 1 commit intodotansimha:masterfrom Sep 24, 2020
Merged
fix: ensure variance of types matches how values are used#38dotansimha merged 1 commit intodotansimha:masterfrom
dotansimha merged 1 commit intodotansimha:masterfrom
Conversation
The issue with specifying them as separate properties is that you get the wrong variance.
Consider the case of:
```ts
declare function runQueryA(q: TypedDocumentNode<{output: string}, {input: string | null}>): void;
// valid
declare const optionalInputRequiredOutput: TypedDocumentNode<{output: string}, {input: string | null}>;
runQueryA(optionalInputRequiredOutput);
// invalid: query might return {output: null} but runQueryA expects to get {output: string}
declare const optionalInputOptionalOutput: TypedDocumentNode<{output: string | null}, {input: string | null}>;
runQueryA(optionalInputOptionalOutput);
// invalid: runQueryA might pass {input: null} but query expects {input: string}
declare const requiredInputRequiredOutput: TypedDocumentNode<{output: string}, {input: string}>;
runQueryA(requiredInputRequiredOutput);
// invalid: runQueryA might pass {input: null} but query expects {input: string} AND
// query might return {output: null} but runQueryA expects to get {output: string}
declare const requiredInputOptionalOutput: TypedDocumentNode<{output: string | null}, {input: string}>;
runQueryA(requiredInputOptionalOutput);
```
Because for the purposes of type checking, TypeScript assumes you will only read from queries (all properties are treated as "Covariant", it will correctly catch the inaccuracies in the Result type, but not in the Variables type. The purpose of specifying it as a function, is that it tells TypeScript that the Variables will be used as input (they are "contravariant") and the results will be read from (they are "covariant").
To see this variance in action, consider the following example, using the same queries as above, but a different runQuery definition that is more tollerant in both the input and output parameters:
```ts
declare function runQueryB(q: TypedDocumentNode<{output: string | null}, {input: string}>): void;
// still valid: We still accept {output: string} as a valid result.
// We're now passing in {input: string} which is still assignable to {input: string | null}
runQueryB(optionalInputRequiredOutput);
// valid: we now accept {output: null} as a valid Result
runQueryB(optionalInputOptionalOutput);
// valid: we now only pass {input: string} to the query
runQueryB(requiredInputRequiredOutput);
// valid: we now accept {output: null} as a valid Result AND
// we now only pass {input: string} to the query
runQueryA(requiredInputOptionalOutput);
```
Closed
ForbesLindesay
added a commit
to ForbesLindesay/graphql-js
that referenced
this pull request
Sep 3, 2020
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.
|
Strong 👍 on this; thanks @ForbesLindesay for catching and fixing this! |
Owner
|
Great, thank you @ForbesLindesay , let's hope it will get merged upstream in |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The issue with specifying them as separate properties is that you get the wrong variance.
Consider the case of:
Because for the purposes of type checking, TypeScript assumes you will only read from queries (all properties are treated as "Covariant", it will correctly catch the inaccuracies in the Result type, but not in the Variables type. The purpose of specifying it as a function, is that it tells TypeScript that the Variables will be used as input (they are "contravariant") and the results will be read from (they are "covariant").
To see this variance in action, consider the following example, using the same queries as above, but a different runQuery definition that is more tollerant in both the input and output parameters: