`client.query()` return type should narrow data to non-null when `errorPolicy: "none"`

[ItaiYosephi]

Issue Description

Description:

Trying to upgrade to version 4, there's a lot of ts error around promise based api.
When using the promise-based client.query() API with errorPolicy: "none" (the default), the promise rejects on errors and only resolves when data is successfully returned ( see docs). However, the TypeScript return type still shows data as potentially undefined, requiring unnecessary non-null assertions or runtime checks.

const result = await client.query<MyDocumentQuery, MyDocumentVariables>({
  query: MyDocument,
  errorPolicy: 'none', // default
});

// TypeScript error: result.data is possibly undefined
const data: MyDocumentQuery =  result.data; // ❌ Type 'MyDocumentQuery | undefined'

Expected:

the return type of promised-based APIs should have a different return type based on the errorPolicy option.
EDIT: Thinking about it, the default value of errorPolicy can be modified by passing defaultOptions to ApolloClient constructor, so this needs to be taken into consideration

Link to Reproduction

no link

Reproduction Steps

No response

@apollo/client version

4.1.2

[jerelmiller]

Hey @ItaiYosephi 👋

You nailed it on why the type is returned the way it is due to defaultOptions. We don't have a type-safe way to handle defaultOptions right now and unfortunately we have enough usage on it that we didn't want to cause runtime issues because of it. Because the promise-based APIs are much simpler, we felt that a small additional check for data !== undefined (or optional chaining) was less harmful than smarter types that don't know about defaultOptions. I know its less than ideal and annoying to have to do this though.

We've had this question come up a few times now since the release of 4.0. I'm going to put this on the table for our next team meeting and see if we want to revisit that decision. If we go this route, we'll probably need to put a big disclaimer in our docs that specify the risks of defaultOptions until we get a more type-safe solution in place. Thanks for bringing it up!

[ItaiYosephi]

@jerelmiller Thanks for the quick response!
Though the defaultOptions issue might be harder to solve, there could be a solution when errorPolicy is explicitly set to none- in that case, we know dor sure data should not be nullable, what do you think?
Also, I noticed that defaultOptions.watchQuery affects the promise-based api, so what's defaultOptions.query for? or is it (un?)intended inheritance?

[jerelmiller]

Hey @ItaiYosephi 👋

I just opened #13130 which will apply smarter types for client.query so that the data and error properties better reflect the applied error policy. Note that the changes are not aware of defaultOptions. I believe we are going to make an attempt at providing a solution for that before this type change makes it in a public release. If not, we plan to add a big disclaimer in the docs.

We plan to release this change in 4.2. If you'd like to test it out, you can try the PR build:

npm i https://pkg.pr.new/apollographql/apollo-client/@apollo/client@13130

We'll plan to get an alpha out in the next few days with this change as well.

---

On another note, I wanted to followup with a couple things you said.

Also, I noticed that defaultOptions.watchQuery affects the promise-based api, so what's defaultOptions.query for?

Is "promised-based api" here the client.query method or are you referring to something else? Assuming client.query, it should absolutely merge defaultOptions.query options, not watchQuery. See this bit of code which applies it:

apollo-client/src/core/ApolloClient.ts

Lines 988 to 990 in 6f114e1

	if (this.defaultOptions.query) {
	options = mergeOptions(this.defaultOptions.query, options);
	}

Is there something you're seeing that suggests otherwise?

[ItaiYosephi]

Hi @jerelmiller ,
Yes, I meant client.query (this is the terminology used in the docs).
In our app, we have the following default options:

  defaultOptions: {
    watchQuery: {
      errorPolicy: 'all',
      fetchPolicy: 'network-only',
      nextFetchPolicy: 'cache-first',
    },
  },

notice we don't configure query default options, only watchQuery.

So when we call client.query, and an error returns, the promise is not rejected (all behavior, even though the default should be none), but if we change the above options to none, it does get rejected. so it seems that watchQuery default options, affect the promised base api for some reason.
I suspect that also happens to fetchPolicy, but didn't check that.

To be honest, not sure what's the use case for separate default options for watchQuery and query, it might make more sense to have default options for both promise based and observable based.

[jerelmiller]

@ItaiYosephi great catch! I was able to reproduce in #13135. We'll need to dig into that further. As this is a separate issue, I've created #13136 to discuss further. Thanks for letting us know!

[jerelmiller]

To be honest, not sure what's the use case for separate default options for watchQuery and query, it might make more sense to have default options for both promise based and observable based.

You're actually contradicting yourself a little bit here 😆. The separation between query and watchQuery default options are that separation between the promise-based and observable based APIs. More technically, it separates default options for the client.query and client.watchQuery APIs.

The issue regarding client.query reading defaultOptions.watchQuery was a bug and not intentional 😅