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

fix(graphcache): Fix defer field state becoming sticky and affecting future fields #3167

Merged
merged 4 commits into from
Apr 20, 2023

Conversation

kitten
Copy link
Member

@kitten kitten commented Apr 19, 2023

Resolves #3161

Note: This will need a rebase after #3165 is merged and should be merged on top.

Summary

A rather old regression would cause @defer directives to manipulate the deferRef state and then become “sticky”, meaning that all future fields would also be considered deferred.

Instead, we need to make sure that this field resets properly and that the state returns to normal.

On a side-note, I tried to convert makeSelectionIterator to a generator function, since this would make it more elegant, but an initial rough implementation decreased overall cache read/write performance by 15–20%, so we'll have to investigate whether there's more we can do here. In general, traversal is the most expensive part of the cache operations, so if we can instead make it faster we'd profit from that quite a lot.

Set of changes

  • Refactor makeSelectionIterator to reset deferRef
  • Add missing test cases for makeSelectionIterator

@kitten kitten requested a review from JoviDeCroock April 19, 2023 22:00
@kitten
Copy link
Member Author

kitten commented Apr 19, 2023

Unanswered question that may go along with traversal performance; cc @JoviDeCroock,
when fixing the regression I also realised that we potentially don't cover overlapping deferred and non-deferred selection sets. When a fragment is deferred but not fulfilled, however, an overlapping selection does provide part of the nested, deferred fragment, we'd currently fail with an unexpected cache miss, e.g.:

{
  child { id }
  ... @defer {
    # `child` would be traversed here...
    child {
      id # ...because this object is present because of `id`
      extra # however, here we'd run into a cache miss
    }
  }
}

In the above case, we can see that we have a need for defer to become recursive. I can submit a small fix for this after this comment, however, this highlights the need for us to optimise traversal further

Edit: Fix is submitted and fields now are marked as deferred recursively across traversals

@kitten kitten force-pushed the fix/defer-tracking branch from ea41be5 to 89a8e4b Compare April 20, 2023 10:02
@kitten kitten force-pushed the fix/defer-tracking branch from 89a8e4b to 4ee6fc2 Compare April 20, 2023 10:04
@kitten kitten merged commit a2bb8ea into main Apr 20, 2023
@kitten kitten deleted the fix/defer-tracking branch April 20, 2023 10:07
@github-actions github-actions bot mentioned this pull request Apr 20, 2023
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.

cache issues when using @defer directive and @urql/exchange-graphcache
2 participants