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

fix: ensure variance of types matches how values are used #38

Merged
merged 1 commit into from
Sep 24, 2020

Conversation

ForbesLindesay
Copy link
Contributor

The issue with specifying them as separate properties is that you get the wrong variance.

Consider the case of:

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:

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);

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);
```
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.
@chriskrycho
Copy link

Strong 👍 on this; thanks @ForbesLindesay for catching and fixing this!

@dotansimha
Copy link
Owner

Great, thank you @ForbesLindesay , let's hope it will get merged upstream in graphql-js as well :)

@dotansimha dotansimha merged commit e3ac5b0 into dotansimha:master Sep 24, 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