NotificationsYou must be signed in to change notification settings
Fork2.7k
Star19.6k

Don't call`onError` if errors are thrown in`onCompleted`#12174

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

jerelmiller merged 5 commits intorelease-3.13fromjerel/issue-12008-onerror

Dec 6, 2024

Merged

Don't call`onError` if errors are thrown in`onCompleted`#12174

jerelmiller merged 5 commits intorelease-3.13fromjerel/issue-12008-onerror

Dec 6, 2024

Conversation

Copy link

Member

jerelmiller commentedDec 5, 2024

Fixes#12008

Errors thrown inonCompleted as a result of an error thrown insideonCompleted exhibit two behaviors:

The error becomes theerrors property on the resolved value from the mutation
onError is also called with the error thrown fromonCompleted

This is a bit surprising behavior asonError is meant to handle errors from the mutation.

This PR moves the error handling from a.catch on the promise to the 2nd argument to.then so that errors thrown inonCompleted aren't handled the same way. This results in the mutation promise throwing that error instead and ensuresonError is not called as a result.

jerelmiller requested a review fromphryneas

December 5, 2024 18:24

Copy link

svc-apollo-docs commentedDec 5, 2024•
edited
Loading

✅ Docs Preview Ready

No new or changed pages found.

Copy link

changeset-botbot commentedDec 5, 2024•
edited
Loading

🦋 Changeset detected

Latest commit:fe8cd6f

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	Minor

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

Copy link

pkg-pr-newbot commentedDec 5, 2024•
edited
Loading

npm i https://pkg.pr.new/@apollo/client@12174

commit:c7d6358

Copy link

netlifybot commentedDec 5, 2024

✅ Deploy Preview forapollo-client-docs ready!

Name	Link
🔨 Latest commit	`5904a06`
🔍 Latest deploy log	https://app.netlify.com/sites/apollo-client-docs/deploys/6751eff84bc8b400082d21b9
😎 Deploy Preview	https://deploy-preview-12174--apollo-client-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to yourNetlify site configuration.

Copy link

Contributor

github-actionsbot commentedDec 5, 2024•
edited
Loading

size-limit report 📦

Path	Size
dist/apollo-client.min.cjs	40.64 KB (-0.01% 🔽)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/main.cjs"	50.02 KB (0%)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/main.cjs" (production)	47.14 KB (0%)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/index.js"	36.14 KB (0%)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/index.js" (production)	33.54 KB (0%)
import { ApolloProvider } from "dist/react/index.js"	1.26 KB (0%)
import { ApolloProvider } from "dist/react/index.js" (production)	1.24 KB (0%)
import { useQuery } from "dist/react/index.js"	5.21 KB (0%)
import { useQuery } from "dist/react/index.js" (production)	4.29 KB (0%)
import { useLazyQuery } from "dist/react/index.js"	5.7 KB (0%)
import { useLazyQuery } from "dist/react/index.js" (production)	4.78 KB (0%)
import { useMutation } from "dist/react/index.js"	3.62 KB (-0.09% 🔽)
import { useMutation } from "dist/react/index.js" (production)	2.84 KB (-0.14% 🔽)
import { useSubscription } from "dist/react/index.js"	4.42 KB (0%)
import { useSubscription } from "dist/react/index.js" (production)	3.48 KB (0%)
import { useSuspenseQuery } from "dist/react/index.js"	5.51 KB (0%)
import { useSuspenseQuery } from "dist/react/index.js" (production)	4.17 KB (0%)
import { useBackgroundQuery } from "dist/react/index.js"	5.01 KB (0%)
import { useBackgroundQuery } from "dist/react/index.js" (production)	3.66 KB (0%)
import { useLoadableQuery } from "dist/react/index.js"	5.09 KB (0%)
import { useLoadableQuery } from "dist/react/index.js" (production)	3.74 KB (0%)
import { useReadQuery } from "dist/react/index.js"	3.41 KB (0%)
import { useReadQuery } from "dist/react/index.js" (production)	3.35 KB (0%)
import { useFragment } from "dist/react/index.js"	2.36 KB (0%)
import { useFragment } from "dist/react/index.js" (production)	2.31 KB (0%)

jerelmiller force-pushed thejerel/issue-12008-onerror branch froma260e06 to9a71fc8Compare

December 5, 2024 18:26

Copy link

netlifybot commentedDec 5, 2024

✅ Deploy Preview forapollo-client-docs ready!

Name	Link
🔨 Latest commit	`a260e06`
🔍 Latest deploy log	https://app.netlify.com/sites/apollo-client-docs/deploys/6751f039e389d3000803c7e5
😎 Deploy Preview	https://deploy-preview-12174--apollo-client-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to yourNetlify site configuration.

Copy link

netlifybot commentedDec 5, 2024•
edited
Loading

✅ Deploy Preview forapollo-client-docs ready!

Name	Link
🔨 Latest commit	`fe8cd6f`
🔍 Latest deploy log	https://app.netlify.com/sites/apollo-client-docs/deploys/6753262a42d6990008f53272
😎 Deploy Preview	https://deploy-preview-12174--apollo-client-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to yourNetlify site configuration.

phryneas approved these changes

Dec 6, 2024

View reviewed changes

Copy link

Member

phryneas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Generally in favor of this, but I'm not sure if this is a patch or should at least be part of a minor, since it does change behaviour considerably and people might not read patch notes.

github-actionsbot added the auto-cleanup🤖 label

Dec 6, 2024

Copy link

MemberAuthor

jerelmiller commentedDec 6, 2024

@phryneas I'd be happy to wait for a minor. I'd like to getuseSuspenseFragment out somewhat quickly anyways, so perhaps we can get this in a 3.13 release.