Better TData types when using returnPartialData or a different errorPolicy

[jerelmiller]

Fixes #10713

This PR introduces more robust types for the data property returned from useSuspenseQuery. In general, data should be TData, but there are some cases where TData could be different:

1. When using an errorPolicy other than none

When using the default error policy of none, the hook will throw the error to the nearest suspense boundary. In these cases, we can guarantee data is TData since GraphQL guarantees that data will always have a value when there are no errors returned. When enabling all or ignore error policies, the hook will no longer throw, so there is a chance that data could now be undefined. Rather than returning TData | undefined as the type, useSuspenseQuery incorrectly reported this type as TData which could cause runtime errors.

2. When using returnPartialData: true

When returnPartialData is set to true and we have data in the cache for the query, useSuspenseQuery will avoid suspending and instead show render any data it can. Because the data could be partial, the data type was incorrectly set to TData and instead should be a DeepPartial<TData>.

Both of these scenarios are now fixed in this PR. You can combine the options and receive the correct typings. For example:

const result = useSuspenseQuery<QueryData>(QUERY, { 
  returnPartialData: true, 
  errorPolicy: 'all' 
});

const data = result.data
//     ^? DeepPartial<QueryData> | undefined

This PR also changes the default type of TData to unknown instead of any when TData is not given or cannot be inferred.

Checklist:

• If this PR contains changes to the library itself (not necessary for e.g. docs updates), please include a changeset (see CONTRIBUTING.md)
• If this PR is a new feature, please reference an issue where a consensus about the design was reached (not necessary for small changes)
• Make sure all of the significant new logic is covered by tests

[changeset-bot]

🦋 Changeset detected

Latest commit: fa854ea

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@apollo/client	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[phryneas]

For the Caveat:

All this logic could also be replaced by

export function useSuspenseQuery_experimental<
  TData,
  TVariables extends OperationVariables,
  Options extends SuspenseQueryHookOptions<NoInfer<TData>, NoInfer<TVariables>>
>(
  query: DocumentNode | TypedDocumentNode<TData, TVariables>,
  options?: SuspenseQueryHookOptions<NoInfer<TData>, NoInfer<TVariables>> & Options
): UseSuspenseQueryResult<
  Options['errorPolicy'] extends 'ignore' | 'all'
    ? Options['returnPartialData'] extends true
      ? DeepPartial<TData> | undefined
      : TData | undefined
    : Options['returnPartialData'] extends true
    ? DeepPartial<TData>
    : TData,
  TVariables
>;

This would have it's own Caveats, though:

1. where TData defaulted to any before, it now defaults to unknown.
2. This only works in inferred scenarios, where the user does not manually specify TData or TVariables. (Which is why I suggested we use overloads instead in the first place.)

I would generally say that 1. is a good change we should make anyways before shipping useSuspenseQuery.

As for 2., that is unfortunate - and we could "fix" that by combining both approaches: add this as an additional first signature for inferred use and keep all the others for user-specified generic usages.

[jerelmiller]

it now defaults to unknown.

Yes, I absolutely agree with this. I'd like to start leaning on unknown for new hooks from here on out. I actually meant to make this change as part of this PR, but forgot to do it. Thanks for calling this out!

[jerelmiller]

Updated the type tests to also check for the return type when you explicitly set the generic type: 27efa5c

[jerelmiller]

Updated the default TData to be unknown instead of any in 66dcc43

[jerelmiller]

@phryneas your solution actually works quite well, except it breaks this test which checks that a wider TVariables type is not allowed.

This is one area of TypeScript I'm not very good at. Do you know if there is a way to ensure the NoInfer carries to Options generic? I'd love to ensure variables is properly set to TVariables to disallow extra properties from being passed. If I comment out the code from your solution and leave it to the overloads, everything works as expected. Let me know if you have any other ideas.

[phryneas]

@phryneas your solution actually works quite well, except it breaks this test which checks that a wider TVariables type is not allowed.

This is one area of TypeScript I'm not very good at. Do you know if there is a way to ensure the NoInfer carries to Options generic? I'd love to ensure variables is properly set to TVariables to disallow extra properties from being passed. If I comment out the code from your solution and leave it to the overloads, everything works as expected. Let me know if you have any other ideas.

Irritatingly, that test works fine for me. Are we using different TypeScript versions? I'm on 5.0.4.

Also, why are the tests green in that case - do TypeScript tests not fail our CI yet?

[jerelmiller]

Are we using different TypeScript versions? I'm on 5.0.4.

Thats a good question and very possible. Let me make sure my editor is using the right TS version. For some reason this is difficult information to get in neovim 🙃.

why are the tests green in that case

I haven't pushed up any code that causes this to fail, so its expected that everything on this branch is passing. If I copy your code snippet into useSuspenseQuery as the first overload, I see the test I referenced start breaking yet.

Our CI should catch TS failures and cause our tests to fail (in fact, here is one of them for commit e008004).

[jerelmiller]

Can confirm via VSCode that I'm running 5.0.4 locally. When I copy that code snippet above into useSuspenseQuery, here is the test that starts failing:

[phryneas]

Yeah I see it now.

I believe that this would be easier with only options and query as a option on that, not query as an extra argument - and if I remember correctly, that was a change we were planning to do anyways.

So maybe now would be the best moment in time to do that, and tackle this PR again after that?

[jerelmiller]

@phryneas got it figured out! 95ff940. Turns out omitting variables from the extends allowed me to bypass the inference of variables from the TOptions generic. It now infers variables from TVariables instead.

[alessbell]

🎉🎉🎉