apollo-client: Type inference broken in 3.8

I’m on mobile right now and will respond with more detail in the evening, but can you give an example of such a generic hook? Are you dynamically creating Graphql queries in runtime here?

Generally: a deprecation would not mean that we would remove it anytime soon, it’s just the only visible “discouraging” indicator we have at our disposal (apart from a TS error) to indicate “you probably shouldn’t use this in most use cases” to nudge people towards more typesafe patterns where it’s possible.

My assumption here would be that your inline gql call is not typed correctly, but just returning a DocumentNode of unknown type.

Generally, I know this is not what you want to hear, but I would say that this might be a good thing that this is failing - let me try to explain:

The idea of that change was that types only flow in one direction: from the query to the options. So if you pass in a TypedDocumentNode<SomeQuery, SomeQueryVars>, the options are automatically seen as QueryHookOptions<SomeQuery, SomeQueryVars>. But if you pass in QueryHookOptions<SomeQuery, SomeQueryVars>, we still know nothing about the query.

In the past, that flow worked in both directions, but it had a big problem: If those types were mismatching, they “widened”. So if you passed in TypedDocumentNode<QA, AV> and QueryHookOptions<QB, BV>, the result ended up being QA & QB with variables AV & BV. As a result, users never saw a warning if you passed in variables that were actually not part of your query.

So, why am I saying “this is a good thing” in your case? Because it highlights that you have no actual type safe connection between your query and variables!

Someone could accidentally mix up a type in your QueryHookOptions that doesn’t match up with your query, and you would never get a type error out of it.

Now, for you there are a few ways out of it, I would recommend one of those two: Either you could change from gql to the graphql helper function of GraphQL Codegen. That would make your query typesafe and actually show you errors if your query and variable ever mismatched Alternatively, you could switch up your pattern here - the thing is that since GraphQL codegen automatically generates TypedDocumentNodes for you, there is no real need to write custom hooks like this any more at all. This example is perfectly typesafe:

import { SomeQueryDocument } from './generated-types'

useQuery(SomeQueryDocument, { ... })

Because the SomeQueryDocument already contains all the necessary type information, you do not need to declare a custom hook at all, and can just use the generated Document with useQuery and our other hooks. It also has the benefit that unlike your hooks, the gql helper does not need to do extra execution work every time the hook is called.

@plausible-phry appreciate the detailed response and suggestions.

However, there are still various reasons to create custom hooks. Sometimes there is custom logic (omitted in the example above), but even if not we still prefer to extract the details of setting up a query/mutation to a custom hook to reduce the visual noise it otherwise introduces and to facilitate re-use.

Totally understand the rationale behind the changes here and believe the intention is correct, however I do not think it makes a great deal of sense to force consumers to only do it this way. Without looking at the code, it seems like the same outcome could be achieved by only enabling these extra checks when a consumer opt-in by choosing to use a TypedDocumentNode.

Sometimes it isn’t even possible to use a TypedDocumentNode. For example, we have a useQueryAll hook which accepts a generically-typed options and auto-paginates the query:

/**
  * `TData` must extend:
  * 
  *  <field> {
  *    pageInfo { hasNextPage hasPreviousPage startCursor endCursor }
  *    edges { cursor node { id } }
  *    totalCount
  *  }
  */

type KeyOf<T, TData extends Record<string, unknown>> = keyof {
  [K in keyof TData]: TData[K] extends T ? K : never;
};

export const useQueryAll = <
  TData extends Record<string, Connection>,
  TVariables extends OperationVariables & {page?: Maybe<PaginationArgs>},
>(
  field: KeyOf<Connection, TData>,
  document: DocumentNode,
  options: QueryHookOptions<TData, TVariables>,
): Pick<
  QueryResult<TData, TVariables>,
  "called" | "client" | "data" | "error" | "loading" | "networkStatus" | "refetch"
> => {
  const {data, loading, fetchMore, refetch, client, called, networkStatus} = useQuery(document, {
    ...options,
    pollInterval: 0,
  });

  // etc ...
}

Using typed-options has honestly worked incredibly well for us, and presumably for many others. I hope you agree that it is worth preserving the option to do things this way!

Hey @jxdp 👋

So sorry to hear about the regression! FWIW we spent a lot of time iterating on the new suspense hooks with and without inference so I’d encourage you to give them a try (here are the type tests to give you an idea). In fact, I’m willing to bet you’ll have a better time with those hooks than useQuery 🙂.

I’m willing to bet #10506 might have had something to do with it. We added a fix to ensure that variables weren’t accidentally widened when passing extra variables that weren’t included in your query. This change was released in 3.8.0-alpha.7. Would you be willing to install v3.8.0-alpha.6 or earlier and see if this type issue goes away? This should at least tell us if #10506 is responsible.

Thank you @jerelmiller. I’ve tried installing a few of the alpha versions and you’re exactly right–it is with alpha.7 that the type errors begin.

So I’ll be sticking with alpha.6 for now–hopefully it is only a minor tweak that is required! Perhaps we are all crazy for using a language in which writing/understanding types is much more difficult than writing/understanding code. 😂