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

feature: added support for automatic type inference with typed-document-node #6720

Merged
merged 7 commits into from
Aug 4, 2020
Merged

feature: added support for automatic type inference with typed-document-node #6720

merged 7 commits into from
Aug 4, 2020

Conversation

dotansimha
Copy link
Contributor

@dotansimha dotansimha commented Jul 28, 2020

Hi!

This PR adds support for typed-document-node interface as part of the core API and React support.

The idea behind it is to have TData and TVariables burnt into the DocumentNode, and allow TypeScript to automatically infer the result type and variables type, based on the query object passed.

TL;DR

  1. TypeScript types can be burnt into DocumentNode instead of being separated, and specified in each use.
  2. No need to specify any types manually, it's being defined once when TypedDocumentNode is declared, and then inferred automatically by TS.
  3. Simplifies integration of TypeScript and GraphQL.
  4. I also wrote an article with some examples here

Overview

It makes integration of GraphQL frontend consumers and TypeScript much easier, since types are located "inside" the DocumentNode. This simplifies work for developers, since they don't need to manually specify TData or TVariables in order to have typings for their operations, and the types are being defined within the operation itself.

It can be used with manual typings, but the core goal is to use GraphQL Codegen, in order to pre-compile .graphql files into DocumentNode, and generate and burn in the types, and produce TypedDocumentNode<TData, TVariables> that can be used.

Changes in this PR

This PR is not breaking any API, since it uses TData when possible (I added it to some interfaces where, as 2nd generic, with default any, so all APIs remains the same). Logic isn't effected at all, since it's all types. So does bundle-size, since the @graphql-typed-document-node/core package just contains types, and not code.

I added a unit tests, just for typings test, if something will break, the TS diagnostics will kick in and fail the test. The other alternative is to build a TS.Program and compile code to do tests, like done here, I can add tests like those, but it might be an overkill.

Code Examples

Without typed-document-node:

type Data {
  // ...
}

type Variables {
  // ...
}

const query: DocumentNode = gql` ... `;
// types must be specified manually
const result = client.query<Data, Variables>({ query, variables: { ... } });
// must specify types again
const moreData = client.readQuery<Data, Variables>({ query });

This can be misused, since developers can use SomeOtherData or SomeOtherVariables types incorrectly, and it will cause TS to pass build, but fail in runtime, since you might expect different results/variables.

With typed-document-node:

type Data {
  // ...
}

type Variables {
  // ...
}

const query: TypedDocumentNode<Data, Variables> = gql` ... `;

// Just by passing `query`, `variables` and result will be typed, without manually specify types
const result = client.query({ query, variables: { ... } });

// This is also typed, no need to repeat types
const moreData = client.readQuery({ query });

@dotansimha
Copy link
Contributor Author

Hi @benjamn ! I heard that 0.0.1 hurts your eyes :D I published it as 1.0.0 and updated the PR ;)

Btw, I think we should be able to treat this package as devDependency (just like @types/), because we are using it only as a type, so it worth a check.

@dotansimha
Copy link
Contributor Author

dotansimha commented Aug 2, 2020

If someone want to try it out now with latest apollo-client v3 (or even with v2 and older), please refer to the README of the repo: https://github.com/dotansimha/graphql-typed-document-node , we are publishing patch file and a solution for applying support for TypedDocumentNode for apollo-client and other libraries ;)
Live demo is available here: https://codesandbox.io/s/github/dotansimha/graphql-typed-document-node/tree/master/examples/apollo-client-3

@benjamn
Copy link
Member

benjamn commented Aug 3, 2020

@dotansimha This is a really fantastic improvement, and definitely the direction we want to take TypeScript code generation for Apollo Client. I could show you some Slack conversations proving that we've discussed something like this before, but you went ahead and implemented it, so you deserve all the credit here.

Thanks for bumping the major version of @graphql-typed-document-node/core—and I see you're already at 2.1.0!

Longer term, I would love to see TypedDocumentNode incorporated into the graphql package. I don't even think DocumentNode needs to change, since it's so widely used already. TypedDocumentNode can stand alone as a complementary type that makes it clear you're bundling the data/variables types with the document type.

In the short term, I think we (Apollo Client) would be happy to merge this PR, add a (dev) dependency on @graphql-typed-document-node/core, and re-export its TypedDocumentNode type from @apollo/client.

Would you be open to allowing your users to configure where the TypedDocumentNode type gets imported from? Ideally I'd like for Apollo Client users to be able to generate code that imports TypedDocumentNode from @apollo/client (which imports it from @graphql-typed-document-node/core), just so that nothing has to change in the future if TypedDocumentNode moves to a different package. This seems like the easiest way to make the source of TypedDocumentNode an implementation detail, but I'm open to any other ideas you have.

Thanks again for tackling this!

@dotansimha
Copy link
Contributor Author

Yeah exactly, we aim to get this merged into graphql-js and just extend Source and DocumentNode, so all libraries that deals with GraphQL operations will be able to leverage that and use it for type inference.
The newer versions that we published are all around patching existing libraries to support that concept, so it shouldn't really affect the changes here.

I think merging it and making it available in apollo-client will be great in short-term, and if at some point it will be available as part of graphql-js this could be amazing (and we can drop that package completely, or maybe just keep it only for patching other libraries that don't support it yet).

Dev-dependency and re-exporting sounds good, and the codegen plugin for generating the actual TypedDocumentNode instances already has the option to customize the import:

config:
  documentNodeImport: '@apollo/client#TypedDocumentNode'

I pushed change to update the version, move to devDependencies and added the re-export from the core package.

What do you think? @benjamn something is missing?

dotansimha and others added 7 commits August 4, 2020 18:34
When I `npm pack`ed @apollo/client and `npm install`ed it into a test
application, I found it simpler if @apollo/client depended directly on
@graphql-typed-document-node/core, so that I didn't have to install that
package explicitly in the test application.

We already get complaints from folks who don't realize they need to
install the graphql package (a peer dependency of @apollo/client), so I
can easily imagine how much confusion would be caused by needing to
explicitly install graphql-typed-document-node/core, too.

@dotansimha Enjoy that boost in download counts!
Copy link
Member

@benjamn benjamn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I rebased and wrote some more tests to get a feel for how this system works, and I can report it does work quite well!

In particular, I wanted to make sure that using a TypedDocumentNode for options.query would actually constrain the type of options.variables, so passing illegal variables would be a type error. Since both options.query and options.variables use the TVariables type parameter, I was worried that TypeScript might infer a more general/useless type for TVariables if you made a mistake in options.variables (see microsoft/TypeScript#14829 for discussion of that kind of problem). Thankfully, options.query seems to "win" over options.variables in all the cases I tested, so I don't think we need to use a NoInfer<T> wrapper for the various variables fields (but that's an option in the future if necessary).

I also decided it will make our lives a lot easier if @graphql-typed-document-node/core is a direct dependency of @apollo/client, so we don't have people complaining that they had to install it explicitly, like we currently do with the (much shorter and easier to type) graphql package.

Since this is a significant new feature, I'm also going to retarget this PR to the release-3.2 branch, which will eventually/soon be merged back into master, so that we can keep merging and releasing 3.1.x changes in the meantime.

@benjamn benjamn changed the base branch from master to release-3.2 August 4, 2020 22:47
@benjamn benjamn merged commit 081e604 into apollographql:release-3.2 Aug 4, 2020
@benjamn benjamn mentioned this pull request Aug 4, 2020
11 tasks
benjamn added a commit that referenced this pull request Aug 4, 2020
@benjamn
Copy link
Member

benjamn commented Aug 4, 2020

Ok, these changes have been released in @apollo/client@3.2.0-beta.0! Please give this functionality a try, especially if you're already using GraphQL Codegen in your project. You can follow the progress of v3.2.0 in the Release 3.2.0 pull request.

Thanks again to @dotansimha for this amazing contribution!

@dotansimha dotansimha deleted the feature/typed-document-node branch August 5, 2020 06:21
@dotansimha
Copy link
Contributor Author

That's great, thank you @benjamn !

@dotansimha
Copy link
Contributor Author

@benjamn I will create a PR to bump this to v3 in a few minutes. I moved everything that isn't related to the pure interface out of that library, removed all dependencies, and changed the defaults generics of TypedDocumentNode to be { [key: string]: any } (instead of just {}).

hwillson pushed a commit that referenced this pull request Aug 21, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants