[v4] data field in return type of .query() typed as undefined

Client SDKs
1

Currently, the type of the data field in the result of calling the query() method can be undefined :

  export interface QueryResult<TData = unknown> {
    /** {@inheritDoc @apollo/client!QueryResultDocumentation#data:member} */
    data: TData | undefined;

    /** {@inheritDoc @apollo/client!QueryResultDocumentation#error:member} */
    error?: ErrorLike;
  }

It creates issues on our side when trying to migrate to v4.
When we either call getClient().query() (RSC) or query() from useApolloClient(), we now have to do a check like this:

try {
  const result = await query(..);
  if (result.data === undefined) {
    throw new Error(..)
  }
} catch(e) {
  ...
}

We didn’t need to do that in v3.
I understand there was some improvement made for error handling in v4.

However, the v4 doc mentions that in the Promise-based API:

"With the default none error policy, an error causes the promise to reject.”

So we expect that the type of the field data must be TData only (not undefined) when we don’t pass an errorPolicy (or when we pass errorPolicy: ‘none’ which is the default).
It can’t be undefined as in the error case the Promise would reject.

I understand that you want to cover some more complicated cases with different errorPolicy but the default case (errorPolicy: ‘none’) that covers the vast majority of usage should be simple.
Having to do an unnecessary:

if (result.data === undefined) {
  throw new Error(..)
}

is quite bad for a library that we use in a lot of places in our codebase.

Did I miss something?

Thanks for your help :folded_hands:

2

Hey @Adrien_Pelegris :waving_hand:

I do agree that it’s not entirely ideal and we would like to make this type better if we can, but there are a couple issues at play here.

First off, the type in v3 was actually just flat out wrong in some cases. Here is the type of ApolloQueryResult (the type that client.query(…) returns)

export interface ApolloQueryResult<T> {
  /** {@inheritDoc @apollo/client!QueryResultDocumentation#data:member} */
  data: T;
  /**
   * A list of any errors that occurred during server-side execution of a GraphQL operation.
   * See https://www.apollographql.com/docs/react/data/error-handling/ for more information.
   */
  errors?: ReadonlyArray<GraphQLFormattedError>;
  /**
   * The single Error object that is passed to onError and useQuery hooks, and is often thrown during manual `client.query` calls.
   * This will contain both a NetworkError field and any GraphQLErrors.
   * See https://www.apollographql.com/docs/react/data/error-handling/ for more information.
   */
  error?: ApolloError;
  loading: boolean;
  networkStatus: NetworkStatus;
  // If result.data was read from the cache with missing fields,
  // result.partial will be true. Otherwise, result.partial will be falsy
  // (usually because the property is absent from the result object).
  partial?: boolean;
}

Notice how data is a non-nullable, non-optional value. This could actually introduce runtime crashes when using errorPolicy: ‘all’ or errorPolicy: ‘ignore’ when GraphQL errors are returned and data returns null due to error bubbling. data: T assumes you can at least access root fields on your query. In those cases where you get data: nullback from the server, TypeScript couldn’t catch the potential access of root fields on null(we might coerce this to undefined, but I don’t remember off the top of my head. Same principle applies though :slightly_smiling_face: )

As you’ve noticed with the error handling changes in v4, errorPolicynow applies to network errors as well so there is a greater chance for data: undefined when using other error policies. While this should only apply for all or ignore error policies, we had to consider defaultOptionswhich are unfortunately used quite heavily in order to change the default errorPolicy. defaultOptions aren’t type-safe so even if we make client.query(…) a bit smarter by changing the type of data to better suit the provided errorPolicy, defaultOptions still comes in and removes those guarantees (i.e. defaultOptions: { query: { errorPolicy: ‘all’ }} means data could now be TData | undefined, but TypeScript wouldn’t catch that at the client.query(…) call). Because we have so much usage with defaultOptions, we didn’t want to risk the potential runtime crashes involved in this type change.

The good news is, we’d like to change how defaultOptions work in v5 so that we can provide better type safety when using them. We already put quite a few changes in v4 and we wanted to wait one more major to buy us some more time to figure out how best to tackle type safety here.

So I apologize, unfortunately you’re stuck with the undefined checks for now. I hope that gives a bit of background though on why it is the way it is. I would agree though that its not ideal :frowning:.