Add 'errorPolicy' to FetchMoreQueryOptions interface

[dlin303]

Adds errorPolicy option to FetchMoreQueryOptions interface to expose errorPolicy functionality on fetchMore to TypeScript.

Fixes: #12297

[apollo-cla]

@dlin303: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

[netlify]

👷 Deploy request for apollo-client-docs pending review.

Visit the deploys page to approve it

Name	Link
🔨 Latest commit	3f61712

[svc-apollo-docs]

❌ Docs preview failed

The preview failed to build.

Build ID: f5c7dd9f3e5e19fd23dea4af

Errors

react/data/queries

• Cannot find module '@microsoft/tsdoc/schemas/tsdoc.schema.json' from '/var/task/node_modules/.pnpm/@microsoft+tsdoc-config@0.17.0/node_modules/@microsoft/tsdoc-config/lib'

[changeset-bot]

🦋 Changeset detected

Latest commit: 3f61712

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

[dlin303]

Not sure why security-scans workflow is failing. It looks like I don't have access in CircleCI to see the actual job or failure.

[jerelmiller]

@dlin303 don't worry about that one. For some reason that security scan fails on all forked PRs. Thanks for opening the PR! We'll try and get to it soon!

[jerelmiller]

This is generally really great! The only change I'm requesting here is to check the rest of the renders after the error returns to ensure we understand the behavior of the hook and how it affects the component. Really appreciate you adding those additional tests to make sure this behavior is documented!

[jerelmiller]

I do find this exhisting behavior a bit odd and not entirely sure why this was written that way (oversight perhaps?). Really appreciate you adding this test as it documents the current behavior.

That said, would you mind adding a TODO comment above this test that mentions we should come back to re-evaluate whether this is the correct long-term behavior? I see no reason why adding a query option should behave much different than not adding one. The TODO comment will at least help remind us that this may not be optimal for long-term (definitely not something we have to fix/implement right now).

In the mean time, I'll ask around and see if I can figure out at all why this might behave this way.

[jerelmiller]

I think we should probably also check what the next rendered value is after this error (i.e. you should have another await takeSnapshot() block. Can you update this test to include that? If the hook doesn't actually rerender, add an await expect(takeSnapshot).not.toRerender() call at the end (I think we should do this regardless after we've checked all the renders)

[dlin303]

Sure thing. Updated.

[jerelmiller]

Same as the other comment, can we check the rest of the renders to see how the error affects the hook in the component itself?

[dlin303]

👍

[jerelmiller]

Ditto, let's check the rest of the renders here.

[dlin303]

Updated. Turns out this test was actually missing the query parameter on line 6205 despite the description, so added that as well.

[dlin303]

Not quite sure why that particular test for "useQuery Hook polling stops polling when component unmounts with cache-and-network fetch policy" is failing. I'm unable to repro the test failure locally, and it seems like it should be unrelated to my changes. However , it does show up in the useQuery.test.tsx test suites so maybe some suspicion there.

CircleCI UI seems to suggest that it's a flaky test, but I don't appear to have the ability to rerun it in CircleCI from the console without force-pushing my change over and over.

Anyone have any insight whether or not this is an expected flaky test when running in a CI environment?

[jerelmiller]

@dlin303 we have a few flaky tests and this happens to be one of them. I've restarted that test suite to see if it passes this time. Unfortunately the frequency of these flakes seems to be increasing lately 🤦‍♂️

[jerelmiller]

Hey @dlin303 👋

The more we've talked about this throughout the week, the more @phryneas and I agree that an errorPolicy other than none doesn't make much sense with fetchMore, specifically because its too easy to merge in partial data with a non-partial result and this could get very difficult to manage. We think there are too many footguns by allowing that errorPolicy to be set to anything different.

As such, we think the correct approach would be to leave the TypeScript types as-is and instead fix the issue by always overriding errorPolicy to none in the options set inside of fetchMore. Is this something you'd like to update in this PR? If not, I can close this and we can do it separately.

I'm very sorry for the back and forth on the solution here.

For your specific situation where you're ok with partial results, what I'd recommend is a custom link that would remove errors from the result payload before it makes it back to the core client. I'd recommend using this in combination with context to tell the link when to perform that transformation. Here is a rough idea:

const ignoreErrorsLink = new ApolloLink((operation, forward) => {
  return forward(operation).map((result) => {
    if (operation.getContext().ignoreErrors && result.errors) {
      return { ...result, errors: undefined };
    }

    return result;
  });
})

Then in your fetchMore call:

fetchMore({ context: { ignoreErrors: true } });

Hopefully something like this will work for your use case.

[dlin303]

Hey @jerelmiller, thanks for the response and the thought you and @phryneas have put into the issue!

Totally understand the concern about building footguns and why that might be a problem.

I'd like to consider a few things however. It seems that the concern with merging partial data would apply regardless, as there is nothing preventing the main query, or any other query touching that node, from writing partial data to the cache even if fetchMore enforced an errorPolicy of none.

Overriding errorPolicy to none for all cases seems like it wouldn't significantly prevent that issue, but would end up reducing the flexibility to support pagination cases that are willingly resilient to partial errors.

To address the original footgun, would it be unreasonable to default to always overriding errorPolicy to none, unless explicitly stated in the fetchMore params? That way, only developers who intentionally want a different errorPolicy would specify it, accepting any associated risks.

[jerelmiller]

It seems that the concern with merging partial data would apply regardless, as there is nothing preventing the main query, or any other query touching that node, from writing partial data to the cache even if fetchMore enforced an errorPolicy of none

This is true, but I think of it more on the mechanics of how fetchMore works under the hood. The issue isn't that we can prevent you from doing it entirely, but rather ensuring that we aren't doing it without you knowing.

fetchMore will write to the cache using the fetchMore result if you don't provide an updateQuery function:

apollo-client/src/core/ObservableQuery.ts

Lines 552 to 556 in eeba2ef

	cache.writeQuery({
	query: combinedOptions.query,
	variables: combinedOptions.variables,
	data: fetchMoreResult.data as Unmasked<TFetchData>,
	});

If you do provide an updateQuery function, fetchMore will write to the cache given the return value of that function:

apollo-client/src/core/ObservableQuery.ts

Lines 533 to 544 in eeba2ef

	cache.updateQuery(
	{
	query: this.query,
	variables: this.variables,
	returnPartialData: true,
	optimistic: false,
	},
	(previous) =>
	updateQuery(previous! as any, {
	fetchMoreResult: fetchMoreResult.data as any,
	variables: combinedOptions.variables as TFetchVars,
	})

updateQuery is at least a little more explicit that something will be changing. But because the default behavior is just to perform that cache write for you, its easy to miss that a partial result might get written and mixed with your existing query data. This can be frustrating to hunt down if you're unaware of whats happening, especially if that partial data overwrites and nullifies any of those fields (imagine, for example, a pageInfo field returning null and overwriting your page information on the query without you knowing).

If you perform explicit cache writes in your own code with partial data, at least the write operation itself is signal enough that you intend to write that data as-is. Again, the goal isn't to eliminate the cache write entirely, its just to ensure the default behavior doesn't get you in a state that you don't expect. Since errorPolicy: 'none' sets the value of data to undefined, there isn't a chance of this happening since the data is just ignored and never written.

---

That said, perhaps you can give me a more detailed view at your use case. I'd like to better understand more in-depth how you've designed your API to work with pagination so that errors with partial data can be handled correctly. If we support this in the client, we need to be able to explain cases where its ok to use this to other users who ask these questions.

The best I can promise is that I'll noodle on this some more over the weekend. Better understanding your use case will help me understand the best course of action. Appreciate the conversation!

[dlin303]

Thanks for the in-depth explanation! I'll double check with my teammates on exactly how we're handling things on the service to make sure I'm not missing any gaps. It could absolutely be the case that we are inadvertently blowing our toes off without knowing 💥 Will let you know!

I appreciate the time and hope ya'll have a great weekend!

[dlin303]

Hey @jerelmiller, got a chance to chat with my teammates on the service side. Here's a rough summary of our usecase: We are paginating through dynamic content - i.e. recommendations often change for the user, and the user is not aware of any enforced order.

In the event of a partial error in this scenario, we would like to make a best effort rendering of what we are able to receive, rather than error out completely and not show the user anything at all. In most cases of partial error, our service ends up returning the correct # of items, as the errors usually pertain to pieces of missing metadata within the item, which generally do not affect rendering or cache in a significant way.

In the event that pagination itself is affected, our service returns a full error, rather than erroneous or incomplete pageInfo results. For example if 'hasNextPage: true` but token or cursor is undefined, or pageInfo itself is nulled out.

The general takeaway though, are that strong defaults with the option to override seems like it would provide the most intuitive developer ergonomics to handle a variety of different pagination behaviors.

Perhaps naming the param something like errorPolicyOverride can provide a strong signal to the developer that they are pressing the red button, so to speak.

[dlin303]

its easy to miss that a partial result might get written and mixed with your existing query data. This can be frustrating to hunt down if you're unaware of whats happening, especially if that partial data overwrites and nullifies any of those fields

I obviously can't speak for every organization out there, we're far from exports but I think we've learned to carefully monitor cache state and the effects our various queries have on it for troubleshooting 😆