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

Refetch should not return partial data with errorPolicy none #10321

Merged

Conversation

alessbell
Copy link
Member

@alessbell alessbell commented Nov 30, 2022

Closes #10317. git bisect identified 2d5e498 as the commit that introduced the regression.

errorPolicy: 'none'

A quick recap of the default errorPolicy:

If the response includes GraphQL errors, they are returned on error.graphQLErrors and the response data is set to undefined even if the server returns data in its response. This means network errors and GraphQL errors result in a similar response shape.

In the linked issue, two sibling components make two separate queries, where one query's selection set is included in the second query's. The bug is seen when one query fails with an error, but on refetch(), the partial data in the cache is incorrectly rendered.

Since the previous behavior was to indiscriminately unset result.data if the diff.complete || this.options.returnPartialData check failed, this PR reinstates this behavior, but only when errorPolicy is none and networkStatus is refetching.

Checklist:

  • 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

Copy link
Member

@jerelmiller jerelmiller left a comment

Choose a reason for hiding this comment

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

This is a great start! I included a couple suggestions for you to take a look at. Thanks for working to fix this!

src/core/ObservableQuery.ts Outdated Show resolved Hide resolved
src/core/ObservableQuery.ts Outdated Show resolved Hide resolved
src/react/hooks/__tests__/useQuery.test.tsx Show resolved Hide resolved
@changeset-bot
Copy link

changeset-bot bot commented Jan 4, 2023

🦋 Changeset detected

Latest commit: 7c16b85

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

@alessbell alessbell force-pushed the issue-10317-refetch-gets-partial-data-with-errorPolicy-none branch from bf37255 to 7439e61 Compare January 19, 2023 19:31
Copy link
Member

@jerelmiller jerelmiller left a comment

Choose a reason for hiding this comment

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

Nice work! I had 2 minor asks, but this fix looks great! Thanks for looking into this!

src/react/hooks/__tests__/useQuery.test.tsx Show resolved Hide resolved
src/core/QueryManager.ts Show resolved Hide resolved
Copy link
Member

@jerelmiller jerelmiller left a comment

Choose a reason for hiding this comment

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

🚢 🚢 🚢

@jerelmiller jerelmiller merged commit bbaa3ef into main Jan 20, 2023
@jerelmiller jerelmiller deleted the issue-10317-refetch-gets-partial-data-with-errorPolicy-none branch January 20, 2023 21:05
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refetch gets partial data from cache with errorPolicy none
3 participants