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

Allow result type and variables type to be inferred from DocumentNode/Source using TypeScript type system #2727

Closed
dotansimha opened this issue Aug 3, 2020 · 7 comments

Comments

@dotansimha
Copy link
Member

dotansimha commented Aug 3, 2020

Hi!

We recently implemented an improved version of DocumentNode, called TypedDocumentNode, that allow type generics for result type and variables type:

export interface TypedDocumentNode<Result = { [key: string]: any }, Variables = { [key: string]: any }> extends DocumentNode {}

This improved version allows TypeScript to infer the types automatically when an object that matches the signature is used, for example:

const typedDocument: TypedDocumentNode<{ test: string }, { v: string }> = parse(`query($v: String) { test(v: $v) }`);

// Now `result` will be automatically of type `{ test: string }`
const result = await execute({
  // ...
  document: typedDocument,
  // variableValues will be typed to `{ v: string }` automatically
  variableValues: { v: 'test' },
});

This will allow to have the types set within the DocumentNode and not externally as used today, and avoid incorrect TypeScript types used.
This made possible because TypeScript does type inference when possible.

The existing solution is implemented in this repo: https://github.com/dotansimha/graphql-typed-document-node and it's currently implemented by patching graphql library and add support to it. I think this concept could be merged into this PR, just like we allow extensions or other to be typed with generics.

Developers who chose to automate the typings creation, can use GraphQL Code Generator to generate a pre-compiled DocumentNode<T, R> from their operations, the output file looks like that:

https://github.com/dotansimha/graphql-typed-document-node/blob/master/examples/graphql/typed-document-nodes.ts#L45

Adding this to the core implementation of graphql won't break anything, since it effects only TypeScript types, and the generics are mostly there today, so it's just a few non-breaking adjustments to support this.

To have it fully supported across graphql and execute method, it needs to be added to Source and DocumentNode. I created a PR here: #2728

Related:

@IvanGoncharov

@dotansimha dotansimha changed the title Allow result type and variables type to be inferred from DocumentNode using TypeScript type system? Allow result type and variables type to be inferred from DocumentNode/Source using TypeScript type system Aug 3, 2020
@benjamn
Copy link

benjamn commented Aug 3, 2020

Apollo Client would gladly use this type, and we think it would be ideal for it to be provided by the same package that defines DocumentNode, though that's not an immediate blocker for us. See also apollographql/apollo-client#6720 (comment).

@IvanGoncharov
Copy link
Member

@dotansimha @benjamn I'm totally for adding TypedQueryDocumentNode (since we type only query and not SDL) to the core. At the same time, I'm against adding types to execute that we can't enforce in runtime, see my arguments here: #2728 (review)
So let's split out TypedQueryDocumentNode that I will happily merge and release and continue the discussion on execute.
What do you think?

@IvanGoncharov
Copy link
Member

@dotansimha @benjamn I went ahead and merged #2749 can you please test it using npm branch:
https://github.com/graphql/graphql-js#want-to-ride-the-bleeding-edge

@yaacovCR
Copy link
Contributor

@dotansimha can this be closed? Is there additional pending integration with TypedDocumentNode?

@dotansimha
Copy link
Member Author

@dotansimha can this be closed? Is there additional pending integration with TypedDocumentNode?

While this was pending, most GraphQL client libraries adopted the original TypedDocumentNode from https://github.com/dotansimha/graphql-typed-document-node (also because you don't need it to be defined in graphql-js, so it supports all previous versions)

@yaacovCR
Copy link
Contributor

Fantastic!

@dotansimha
Copy link
Member Author

Fantastic!

Yeah, but I think in this repo we have a similar type, that's probably unused... :X

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

Successfully merging a pull request may close this issue.

4 participants