urql-graphql / urql

The highly customizable and versatile GraphQL client with which you add on features like normalized caching as you grow.
https://urql.dev/goto/docs
MIT License
8.65k stars 454 forks source link

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

Closed kitten closed 1 year ago

kitten commented 1 year ago

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

kitten commented 1 year ago

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