Support TypedDocumentString via DocumentTypeDecoration in @apollo/client

[benasher44]

Hi there! As of v4 of graphql-code-generator, there is now a string alternative to using TypedDocumentNode, which still brings along the type.

I think @apollo/client could support this by accepting DocumentTypeDecoration as its type in react hooks, and then it would get the benefit of not having to convert the query to a string for the network request (while still having nice types)

[jerelmiller]

@benasher44 thanks for bring this to our attention! I don't think any of us realized this was a new addition. We can certainly take a look and figure out what kind of compatibility there is.

it would get the benefit of not having to convert the query to a string for the network request

Unfortunately we still need access to the underlying AST of the query because Apollo Client performs some underlying query transformations that require the AST (such as adding __typename to field selections). Again, we'll need to figure out the compatibility story here.

Thanks a bunch for the request!

[benasher44]

👍 even if deserializing the queries is still required for Apollo Client, it looks like it might still be a win for web bundling. Thanks for the consideration!

[Grohden]

@jerelmiller what would be the problem on clients (devs) doing something like this?

export function useQuery<
  TData = any,
  TVariables extends OperationVariables = OperationVariables,
>(
  query: TypedDocumentString<TData, TVariables>,
  options?: QueryHookOptions<NoInfer<TData>, NoInfer<TVariables>>
) {
  return useApolloQuery<TData, TVariables>(gql(document.toString()), options);
}

the re-parsing on every call for the hook?

edit:

think that if this ^ is the problem we can prob cache by instance (need to confirm if new instances of the class will actually be different entries on map)

const queryMap = new Map()

// ...useQuery....
const query = (queryMap[document] || queryMap[document] = gql(document.toString()))

edit 2: oh... totally forgot we have use memo.... but still it would be a parse per 'location on tree' if we use use memo

[jerelmiller]

@Grohden looking into TypedDocumentString a bit further, I don't think there is any problem here. Initially I thought TypedDocumentString was an already parsed AST, but that doesn't seem to be the case. Since its already a string, its not doing any more work than it already does today since gql takes a string and parses it to an AST.

No need to create your own cache as graphql-tag already does this for you by the way 🙂.

Apologies we haven't had time to evaluate this yet. If we adopt this change, it will likely cascade throughout the entire codebase (there are a lot of places that accept DocumentNode | TypedDocumentNode currently), so we want to be sure we understand the scope of work here.

[Grohden]

@jerelmiller oh, nice to know! I was already exploring using weak maps for the cache 😅

And that has been working for us here for like 2 months, I just didn't bother to check the cache part. So while we don't get the support in apollo I'll be sticking with that solution (its just a pain to import types but I can live with that)

[jerelmiller]

I was already exploring using weak maps for the cache 😅

Haha no worries! I've been using Apollo Client for years before I was a maintainer and I only recently discovered this 😆. Seems we have a bit of education to do!

its just a pain to import types

I definitely hear you there. At least TypedDocumentNode makes it a bit less noisy at call sites (useQuery, useMutation, etc), but definitely understand how TypedDocumentString takes the DX of this all a step further.

Thanks for the questions!

[phryneas]

Chiming in with a little bit of a curveball here:

Right now, the "document parsing" part of graphql (the gql tag function) doesn't need to be bundled if users e.g. use the graphql codegen to create the AST, or have a bundler plugin that transparently replaces gql strings with AST nodes.

If we made a choice like this, it would mean that gql would become a fixed, non-treeshakeable part of Apollo Client, and that all of those users would have to ship a few extra kilobytes of JavaScript code that would never be executed for them.

We might be able to find a way around that (e.g. we could accept a gql function as an option to Apollo Client), but it would probably not be seamless. The TypeScript story could also be difficult in that scenario, as this would imply some kind of "situationally active feature".

Just as a warning: we will evaluate this at some point, but there is a chance that we find that it doesn't integrate nicely and actually complicates DX more than it helps.