Throw errors returned from incremental chunks when error policy is none

[jerelmiller]

After some time away I came to realize that the behavior of errorPolicy: 'none' when an error is returned in an incremental chunk should be considered incorrect. useSuspenseQuery throws errors when error policy is none, except for this one case. This made the behavior of this error policy unpredictable.

Now all errors are thrown, regardless of whether they are in the first chunk or an incremental chunk when the errorPolicy is set to none. To get the previous behavior, set the errorPolicy to all, which also has the advantage that it keeps partial data.

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: 110d982

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

[jerelmiller]

It's not clear in this diff, but this.result is set above in this same function. This removes the unneeded duplication.

[github-actions]

size-limit report 📦

Path	Size
dist/apollo-client.min.cjs	36.84 KB (-0.01% 🔽)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/main.cjs"	43.38 KB (-0.01% 🔽)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/index.js"	32.56 KB (0%)
import { ApolloProvider } from "dist/react/index.js"	1.3 KB (0%)
import { useQuery } from "dist/react/index.js"	4.33 KB (0%)
import { useLazyQuery } from "dist/react/index.js"	4.65 KB (0%)
import { useMutation } from "dist/react/index.js"	2.56 KB (0%)
import { useSubscription } from "dist/react/index.js"	2.31 KB (0%)
import { useSuspenseQuery } from "dist/react/index.js"	3.71 KB (0%)
import { useBackgroundQuery } from "dist/react/index.js"	3.78 KB (-0.13% 🔽)
import { useReadQuery } from "dist/react/index.js"	2.48 KB (-0.08% 🔽)
import { useFragment } from "dist/react/index.js"	2.12 KB (0%)

[jerelmiller]

Turns out we never reached this case before! We were throwing the entire result as the error rather than the error itself, which is a bug. This change in behavior exposed this issue.

[phryneas]

If we never reached this before, why do we reach it now?
If I'm not mistaken, you didn't change any code flow above this block - and there should be plenty of cases where result.data could be undefined (getCurrentResult should return undefined whenever the data would be {}).

[jerelmiller]

Perhaps I'm making too much of an assumption with that claim. The reason I feel this way because this was throwing the result instead of the error and none of my tests for handling errors caught this. I have tests for all sorts of variations of errors (error on first render, error on refetch, error on incremental chunks of @defer, etc), so I think either the code block above this happened to catch all of those, or something else was amiss. I think the result.data just happened to be defined on all cases where it made it past line 195.

[jerelmiller]

FYI, I'll be refactoring this logic anyways in likely my next PR, so expect this stuff to change and work more robustly. I had to get this change in there first to help that refactor.

[phryneas]

Sorry for my stupid questions here - it might just be late and I might be missing some obvious things here :)

Apart from my questions, which are more about your comments than your code, the code looks fine :)

[jerelmiller]

By verbal comment, @phryneas has approved this PR 🙂