Skip to content
New issue

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

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

Consolidate error properties #12396

Open
wants to merge 26 commits into
base: jerel/rxjs
Choose a base branch
from
Open

Consolidate error properties #12396

wants to merge 26 commits into from

Conversation

jerelmiller
Copy link
Member

Removes the errors from all derived result types and consolidates on the error property thoughout the client. This is a precursor to #12200 which will do work to split up ApolloError into separate types.

@jerelmiller jerelmiller requested a review from phryneas March 4, 2025 00:31
@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Mar 4, 2025 •
edited
Loading

🦋 Changeset detected

Latest commit: d77e202

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 Major

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

@svc-apollo-docs
Copy link

svc-apollo-docs commented Mar 4, 2025 •
edited
Loading

⚠️ Docs preview not attached to branch

The preview was not built because the PR's base branch jerel/rxjs is not in the list of sources.

An Apollo team member can comment one of the following commands to dictate which branch to attach the preview to:

  • !docs set-base-branch version-2.6
  • !docs set-base-branch main

Build ID: 33109b8de3f8bedbd55dfbbc

@pkg-pr-new pkg.pr.new
Copy link

pkg-pr-new bot commented Mar 4, 2025 •
edited
Loading 

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

commit: d77e202

jerelmiller
jerelmiller commented Mar 4, 2025
View reviewed changes
src/core/__tests__/ApolloClient/general.test.ts
@@ -5174,14 +5169,6 @@ describe("ApolloClient", () => {
});

describe("refetchQueries", () => {
let consoleWarnSpy: jest.SpyInstance;
Copy link
Member Author

Choose a reason for hiding this comment

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

This change is unrelated to the errors change, but fixes a flake that's been driving me nuts.

Copy link
Member

Choose a reason for hiding this comment

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

:nod:

@jerelmiller jerelmiller force-pushed the jerel/consolidate-error-properties branch from 56937ce to a8800ff Compare March 6, 2025 03:39
jerelmiller added 21 commits March 7, 2025 17:17
@jerelmiller
Remove errors property from emitted results
dc27490
@jerelmiller
Update core tests to remove check for errors property
38d3a6c
@jerelmiller
Ensure error is set in QueryManager
a0284f4
@jerelmiller
Update top-level tests to remove errors property
85f64a9
@jerelmiller
Add delay to returned value in test
43ff55b
@jerelmiller
Fix issue not throwing for useReadQuery on errors
8c63f9f
@jerelmiller
Ensure non-pending updates handle errors correctly
c5e370b
@jerelmiller
Use result.error directly for useReadQuery
419fe8c
@jerelmiller
Use result.error in useSuspenseQuery
a5d1c4f
@jerelmiller
Remove errors from QueryResult types
0a49a0b
@jerelmiller
Remove errors from checked keys
b14f5e5
@jerelmiller
Remove check for errors field in useQuery tests
a6418ba
@jerelmiller
Remove unneeded handling for errors not error set
c3e1a64
@jerelmiller
Update useSubscription to wrap errors itself
4ea6c58
@jerelmiller
Remove toApolloError in useQuery
dfb98f6
@jerelmiller
Fix ObservableQuery tests that emitted errors
ea92871
@jerelmiller
Remove unused imports
913dc94
@jerelmiller
Remove check for errors property in useLazyQuery tests
94c4771
@jerelmiller
Rerun api report
bdc1054
@jerelmiller
Add changesets
29d416d
@jerelmiller
DRY
5bff424
@jerelmiller jerelmiller force-pushed the jerel/consolidate-error-properties branch from a8800ff to a550a44 Compare March 8, 2025 00:24
@jerelmiller jerelmiller force-pushed the jerel/consolidate-error-properties branch from a550a44 to aec740b Compare March 8, 2025 00:28
phryneas
phryneas reviewed Mar 10, 2025
View reviewed changes
src/react/hooks/useSubscription.ts
Comment on lines +242 to 246
error:
fetchResult.errors ?
new ApolloError({ graphQLErrors: fetchResult.errors })
: undefined,
variables,
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this happen in ApolloClient.subscribe to be consistent with .query and .mutate?

phryneas
phryneas approved these changes Mar 10, 2025
View reviewed changes
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.

One question about consistency for subscriptions, but assuming you want to keep it this way: ✅

@github-actions github-actions bot added the auto-cleanup 🤖 label Mar 10, 2025
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@phryneas phryneas phryneas approved these changes

Assignees
No one assigned
Labels
auto-cleanup 🤖
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jerelmiller @svc-apollo-docs @phryneas