Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 5 commits into from
Dec 6, 2024

Conversation

jerelmiller
Copy link
Member

Fixes #12008

Errors thrown in onCompleted as a result of an error thrown inside onCompleted exhibit two behaviors:

  • The error becomes the errors property on the resolved value from the mutation
  • onError is also called with the error thrown from onCompleted

This is a bit surprising behavior as onError 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 in onCompleted aren't handled the same way. This results in the mutation promise throwing that error instead and ensures onError is not called as a result.

@jerelmiller jerelmiller requested a review from phryneas December 5, 2024 18:24
@svc-apollo-docs
Copy link

svc-apollo-docs commented Dec 5, 2024

✅ Docs Preview Ready

No new or changed pages found.

Copy link

changeset-bot bot commented Dec 5, 2024

🦋 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-new bot commented Dec 5, 2024

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

commit: c7d6358

Copy link

netlify bot commented Dec 5, 2024

Deploy Preview for apollo-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...

QR Code

Use your smartphone camera to open QR code link.

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

Copy link
Contributor

github-actions bot commented Dec 5, 2024

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 jerelmiller force-pushed the jerel/issue-12008-onerror branch from a260e06 to 9a71fc8 Compare December 5, 2024 18:26
Copy link

netlify bot commented Dec 5, 2024

Deploy Preview for apollo-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...

QR Code

Use your smartphone camera to open QR code link.

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

Copy link

netlify bot commented Dec 5, 2024

Deploy Preview for apollo-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...

QR Code

Use your smartphone camera to open QR code link.

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

Copy link
Member

@phryneas 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-actions github-actions bot added the auto-cleanup 🤖 label Dec 6, 2024
@jerelmiller
Copy link
Member Author

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

@jerelmiller jerelmiller changed the base branch from main to release-3.13 December 6, 2024 16:19
@jerelmiller jerelmiller force-pushed the jerel/issue-12008-onerror branch from 04296c1 to c7d6358 Compare December 6, 2024 16:25
@jerelmiller jerelmiller merged commit ba5cc33 into release-3.13 Dec 6, 2024
37 checks passed
@jerelmiller jerelmiller deleted the jerel/issue-12008-onerror branch December 6, 2024 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Question] Syntax Errors in onCompleted causes onError to occur
3 participants