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

(graphcache) - Fix uncached GraphQLError fields making it impossible to get first API result #1367

Merged
merged 3 commits into from
Feb 5, 2021

Conversation

kitten
Copy link
Member

@kitten kitten commented Feb 5, 2021

This is a follow-up for #1356

Summary

The #1356 change introduced behaviour that prevents null values from fields that have errored and are associated with GraphQLError from being cached. This caused a regression where when we "read-back" results from the cache that have just arrived from the API these fields would cause a cache miss:

https://github.com/FormidableLabs/urql/blob/6a687091b67dc3d0ee78353ca79b0aa401ba983c/exchanges/graphcache/src/cacheExchange.ts#L237-L243

We can prevent this by also tracking errored fields in the query and ignoring these uncached fields there too by replacing them with null while we still have access to the GraphQLErrors.
This is preferable to other solutions as we've gotten rid of comparing the cached result to the API result in #1196 as we had trouble differentiating between null selection sets from previous selections and null values from the API result.

No changeset is needed since the PR is unreleased.

Set of changes

  • Add (previously failing) test for retrieving a result with null'd fields due to GraphQLErrors from an API
  • Track ctx.__internal.path in all environments in query too
  • Check for errors before aborting a selection in query and set the field to null if it does have an error

@changeset-bot
Copy link

changeset-bot bot commented Feb 5, 2021

⚠️ No Changeset found

Latest commit: 1a253a5

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Contributor

@wgolledge wgolledge left a comment

Choose a reason for hiding this comment

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

This makes sense to me! The result.error was probably going to have to be brought in at some point anyway

@kitten kitten merged commit 832d9fd into main Feb 5, 2021
@kitten kitten deleted the fix/graphcache-readback-results branch February 5, 2021 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants