Repository: FormidableLabs/urql
Pull request #745 (2020-04-23 14:27:17)

(graphcache) - Shift mutation results to top layer
This makes a slight adjustment to the data layer reservation.
Optimistic Mutation layers are now not only cleared but will be
shifted to the front of the optimistic order queue when they're
written with a concrete result (non optimistic result).

The optimistic mutation layer is therefore still discarded but
the layer itself is reused, which allows for slight adjustments
in how layers are handled when they transition from being
optimistic to non-optimistic.

This also makes an adjustment that allows layers to transition
from being non-optimistic to optimistic. This shouldn't happen
in practice, but protects us from race-conditions.

changeset-bot[bot]: ###  💥  No Changeset

Latest commit: b943c09dab95c2baf93b60046636dc625cc6026c

Merging this PR will not cause any packages to be released. If these changes should not cause updates to packages in this repo, this is fine 🙂

**If these changes should be published to npm, you need to add a changeset.**

<details><summary>This PR includes no changesets</summary>

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

</details>

[Click here to learn what changesets are, and how to add one](https://github.com/changesets/changesets/blob/master/docs/adding-a-changeset.md).

[Click here if you're a maintainer who wants to add a changeset to this PR](https://github.com/FormidableLabs/urql/new/feat/adjust-commutativity-of-mutations?filename=.changeset/smooth-beans-deliver.md&value=---%0A%22%40urql%2Fexchange-graphcache%22%3A%20patch%0A---%0A%0AExperimental%3A%20(graphcache)%20-%20Shift%20mutation%20results%20to%20top%20layer%20above%20pre-reservation%0A)


frederikhors: @kitten is this somehow related to https://spectrum.chat/urql/help/create-mutation-with-a-cache-and-network-list~2d06beed-6d13-4dd6-b977-a78bbc50d82a?
JoviDeCroock: > @kitten is this somehow related to https://spectrum.chat/urql/help/create-mutation-with-a-cache-and-network-list~2d06beed-6d13-4dd6-b977-a78bbc50d82a?

We're trying some stuff out related to offline
kitten: @frederikhors Yes! I'm trying to see whether it's better to strike a balance with commutativity, where we sacrifice some consistency but make mutations take precedence, so updates will always show up on top of query results that have been started after the mutations (but arrive before the mutation result).

You can try this branch out with the Graphcache build from CodeSandbox CI, and let us know if that helps :+1:
frederikhors: I tried.

Something is changed.

I recorded a GIF; I'll comment again in a moment to explain.

![123456](https://user-images.githubusercontent.com/41120635/80117146-16557580-8587-11ea-97f9-d4950680c594.gif)

frederikhors: Now it "reloads"/"refetches" todos after the slow mutation but it shows also the optimistic one.

Maybe because I'm using:

```js
const optimistic = {
  addTodo: (variables, cache, info) => {
    return {
      id: 0 - Date.now(), // <--- this
      text: variables.text,
      complete: false,
      __typename: "Todo"
    };
  }
};
```

and the `ID` from server is different.

I use `0 - Date.now()` to make it negative for this: `<tr class:newTodo={todo.id < 0}>` to make `tr` `background` green.

And again now when I come back to `cache-and-network` list my optimistic todo one disappear!
kitten: @frederikhors Hm, I think I accidentally had a change here that converted your optimistic result to a non-optimistic one: https://github.com/FormidableLabs/urql/pull/745/commits/b943c09dab95c2baf93b60046636dc625cc6026c#diff-59e023625accfe756fba94b9832ba785L228-R230

Could you try the latest change on `c91506e` please, once it finishes on CodeSandbox CI? It seems that we had only loose protections against `reserveLayer` converting an optimistic layer to a non-optimistic one, which I accidentally did.
frederikhors: One problem fixed! 😄

One problem left! 😢

**GIF**:

![123456](https://user-images.githubusercontent.com/41120635/80123347-0f326580-858f-11ea-901b-4697a523e130.gif)

---

It works now the refetch after slow mutation.

And again now when I come back to `cache-and-network` list - during slow mutation - my optimistic todo one disappear!
kitten: That's expected behaviour. You may be overusing `cache-and-network`. If you think about the executed operations what happens, if I'm not mistaken is the following:

- You add a todo, which triggers the mutation. This adds your optimistic todo and updates related queries
- You return to the todo list; `cache-and-network` is used, which means that all todos refetch.
- The todo list query completes; this overrides your optimistic todo
- The todo mutation completes; this overrides, which means that's now back, since it's reflected in server-state.

There are several ways to address this and the two are:

- Consider whether this use of optimistic updates is valid. In some cases explicit actions by the user that have wide-ranging changes (typically adding/removing an item) should maybe not be optimistic and instead have explicit progress. I recognise however that this is just an example app, so let's move on from this suggestion
- Don't overuse `cache-and-network`. Ultimately any query that is optimistically altered runs the risk of being overwritten by actual data refetches, which is intentional.

What we're currently discussing though is to "block" `cache-and-network` from sending queries, when an optimistic mutation is in progress for it.
frederikhors: I've thought about it a lot.

I use optimistics for the most part to signal the user that something is happening; I know I could use Svelte (or React) "simply", but I noticed that it is much simpler and more immediate to use something that I already have available: the optimistic.

In Apollo I had no problem because:

- a slow mutation starts which adds an element
- I create an optimistic element
- back to the list; the optimisitc element is present; the list updates (because `cache-and-network`) but does not delete the optimistic element (or more than one) in progress
- the user notes that they are still in progress
- as soon as the slow mutations end, they update what they need to update

I don't know if Apollo forbids `cache-and-networks` to leave if there are optimistic mutations in progress, but what could be the negative consequences?

In the meantime, I'm trying the news with the example of the mutation slow delete.

_Just everything works and we agree the most idiomatic way, it would be great to create a Wiki or FAQ to document these recipes._
kitten: @frederikhors it just happens to work in Apollo, because in this case your slow-down happens to be in-order, i.e. the mutation completing last, and optimistic layers always taking precedence in Apollo 😉 

I've been also thinking about `cache-and-network`, and it's simply a bad default to have with `@urql/exchange-graphcache`, since a normalised cache seeks to _avoid_ excessive network requests and share cache updates as much as possible, without sending more requests. It makes more sense to have an exchange that _upgrades_ `cache-first` queries to `cache-and-network` if they haven't been sent in a while (TTL-based).

Anyway, I've pushed a WIP-commit (no tests yet) of an initial implementation of blocking stale requests from being refetched when an optimistic update is in progress. That'll need some tests if it works and maybe some tweaks before it's ready, but it may make this behaviour more intuitive.
kitten: I’ll probably branch off the changes, where we block network refetches for data that has been altered by optimistic mutations, and will first get the optimistic layer change merged in 
frederikhors: I tried both with #4b63410.

It works now the adding and the `cache-and-network`. But as soon I try to add 2 new todos some problem again:

![123456](https://user-images.githubusercontent.com/41120635/80132016-5d4d6600-859b-11ea-9adb-5f87d9007bea.gif)


frederikhors: Interesting what is happening with deletions here (https://codesandbox.io/s/urql-svelte-cacheinvalidate-many-items-j89ru):

![123456](https://user-images.githubusercontent.com/41120635/80133942-4f4d1480-859e-11ea-8a02-5c2454e087f4.gif)

kitten: > But as soon I try to add 2 new todos some problem again

As far as I can tell, that looks like expected behaviour, your `updateQuery` is being run when the actual result comes back, but either updaters (whichever executes first) will see the optimistic todo of the other, so you need to account for that manually.

For us, there's not much we can do automatically for lists. If we ignore optimistic data in `updateQuery` then your new layer will lose the old optimistic references. So that's something that has to be handled manually; that goes back to what I was saying about optimistic updates. For lists they're inherently tricky, and for us there's only so much we can do to prevent duplication. At some point those heuristics are API-specific and hence on you 😅 

> Interesting what is happening with deletions here

I think that warning is simply at the wrong position; I added a quick fix for that.
kitten: Collecting some future ideas and issues;

Seems like we need to add a `popDebugNode` in development for some places, which I simply forgot to add before.

Optimistic duplication can actually be addressed. But to do that we’d have to check how many optimistic mutations are in progress, then enforce that they’re _settled together_. That means if multiple optimistic updates have been applied, we wait until the last of their mutations have settled and clear the optimistic data & write the results as one batch.
frederikhors: > I think that warning is simply at the wrong position; I added a quick fix for that.

@kitten the warning is fixed with https://github.com/FormidableLabs/urql/pull/745/commits/be80a04715d7023ac7928c548c98874906a06f64 but I was referring to the behavior: after the first slow mutation (a delete one) all optimistic todos (with a deleting mutation in progress) are removed and after each mutation finish with `invalidate` them are removed. But I loose optimistic green background.

It seems we are close.

Is this what are you talking about in your latest comment?

> Optimistic duplication can actually be addressed. But to do that we’d have to check how many optimistic mutations are in progress, then enforce that they’re settled together. That means if multiple optimistic updates have been applied, we wait until the last of their mutations have settled and clear the optimistic data & write the results as one batch.
kitten: Yep, it seems that the blocking there doesn't quite work. But as that's just an experiment, I think there are some bugs related to that.

Because these improvements will take some time they'll definitely be separate from this PR, so I opened an RFC for them: https://github.com/FormidableLabs/urql/issues/747

And I've got another for a warning improvement: https://github.com/FormidableLabs/urql/issues/748

What I'm talking about in the last two paragraphs there is a solution to the problem where an update after a mutation result may commit other optimistic updates to a permanent, non-optimistic layer. That's all described in the RFC though, which also contains a spec for the refetch blocking.

Patch:
diff --git a/.changeset/long-rocks-give.md b/.changeset/long-rocks-give.md
new file mode 100644
index 00000000..1105a747
--- /dev/null
+++ b/.changeset/long-rocks-give.md
@@ -0,0 +1,5 @@
+---
+'@urql/exchange-graphcache': patch
+---
+
+Adjust mutation results priority to always override query results as they arrive, similarly to subscriptions. This will prevent race conditions when mutations are slow to execute at the cost of some consistency.
diff --git a/exchanges/graphcache/src/cacheExchange.ts b/exchanges/graphcache/src/cacheExchange.ts
index 0b5b823b..cc1bb5ef 100644
--- a/exchanges/graphcache/src/cacheExchange.ts
+++ b/exchanges/graphcache/src/cacheExchange.ts
@@ -140,22 +140,17 @@ export const cacheExchange = (opts?: CacheExchangeOpts): Exchange => ({
       reserveLayer(store.data, operation.key);
     } else if (operation.operationName === 'teardown') {
       noopDataState(store.data, operation.key);
-    } else if (operation.operationName === 'mutation') {
+    } else if (
+      operation.operationName === 'mutation' &&
+      operation.context.requestPolicy !== 'network-only'
+    ) {
       // This executes an optimistic update for mutations and registers it if necessary
-      if (operation.context.requestPolicy !== 'network-only') {
-        const { dependencies } = writeOptimistic(
-          store,
-          operation,
-          operation.key
-        );
-        if (dependencies.size !== 0) {
-          optimisticKeysToDependencies.set(operation.key, dependencies);
-          const pendingOperations = new Set<number>();
-          collectPendingOperations(pendingOperations, dependencies);
-          executePendingOperations(operation, pendingOperations);
-        }
-      } else {
-        reserveLayer(store.data, operation.key);
+      const { dependencies } = writeOptimistic(store, operation, operation.key);
+      if (dependencies.size !== 0) {
+        optimisticKeysToDependencies.set(operation.key, dependencies);
+        const pendingOperations = new Set<number>();
+        collectPendingOperations(pendingOperations, dependencies);
+        executePendingOperations(operation, pendingOperations);
       }
     }
 
@@ -175,15 +170,7 @@ export const cacheExchange = (opts?: CacheExchangeOpts): Exchange => ({
   const updateDependencies = (op: Operation, dependencies: Set<string>) => {
     dependencies.forEach(dep => {
       (deps[dep] || (deps[dep] = [])).push(op.key);
-
-      if (!ops.has(op.key)) {
-        ops.set(
-          op.key,
-          op.context.requestPolicy === 'network-only'
-            ? toRequestPolicy(op, 'cache-and-network')
-            : op
-        );
-      }
+      ops.set(op.key, op);
     });
   };
 
diff --git a/exchanges/graphcache/src/operations/query.ts b/exchanges/graphcache/src/operations/query.ts
index 7807b1a9..cf90d4f8 100644
--- a/exchanges/graphcache/src/operations/query.ts
+++ b/exchanges/graphcache/src/operations/query.ts
@@ -230,11 +230,9 @@ const readSelection = (
       (result && result.__typename)
     : key;
 
-  if (
-    typeof typename !== 'string' ||
-    (result && typename !== result.__typename)
-  ) {
-    // TODO: This may be an invalid error for resolvers that return interfaces
+  if (typeof typename !== 'string') {
+    return;
+  } else if (result && typename !== result.__typename) {
     warn(
       'Invalid resolver data: The resolver at `' +
         entityKey +
@@ -243,7 +241,7 @@ const readSelection = (
       8
     );
 
-    return undefined;
+    return;
   }
 
   // The following closely mirrors readSelection, but differs only slightly for the
diff --git a/exchanges/graphcache/src/store/data.ts b/exchanges/graphcache/src/store/data.ts
index 92a60819..b4c938c1 100644
--- a/exchanges/graphcache/src/store/data.ts
+++ b/exchanges/graphcache/src/store/data.ts
@@ -80,18 +80,18 @@ export const initDataState = (
 
   if (!layerKey) {
     currentOptimisticKey = null;
-  } else if (
-    isOptimistic ||
-    (data.optimisticOrder.length > 1 &&
-      data.optimisticOrder.indexOf(layerKey) > -1)
-  ) {
+  } else if (isOptimistic || data.optimisticOrder.length > 0) {
     // If this operation isn't optimistic and we see it for the first time,
     // then it must've been optimistic in the past, so we can proactively
     // clear the optimistic data before writing
     if (!isOptimistic && !data.commutativeKeys.has(layerKey)) {
-      clearLayer(data, layerKey);
-      data.commutativeKeys.add(layerKey);
+      reserveLayer(data, layerKey);
+    } else if (isOptimistic) {
+      // NOTE: This optimally shouldn't happen as it implies that an optimistic
+      // write is being performed after a concrete write.
+      data.commutativeKeys.delete(layerKey);
     }
+
     // An optimistic update of a mutation may force an optimistic layer,
     // or this Query update may be applied optimistically since it's part
     // of a commutate chain
@@ -457,10 +457,19 @@ export const writeLink = (
 
 /** Reserves an optimistic layer and preorders it */
 export const reserveLayer = (data: InMemoryData, layerKey: number) => {
-  if (data.optimisticOrder.indexOf(layerKey) === -1) {
+  const index = data.optimisticOrder.indexOf(layerKey);
+  if (index === -1) {
     // The new layer needs to be reserved in front of all other commutative
     // keys but after all non-commutative keys (which are added by `forceUpdate`)
     data.optimisticOrder.unshift(layerKey);
+  } else if (!data.commutativeKeys.has(layerKey)) {
+    // Protect optimistic layers from being turned into non-optimistic layers
+    // while preserving optimistic data
+    clearLayer(data, layerKey);
+    // If the layer was an optimistic layer prior to this call, it'll be converted
+    // to a new non-optimistic layer and shifted ahead
+    data.optimisticOrder.splice(index, 1);
+    data.optimisticOrder.unshift(layerKey);
   }
 
   data.commutativeKeys.add(layerKey);


Test patch:
diff --git a/exchanges/graphcache/src/cacheExchange.test.ts b/exchanges/graphcache/src/cacheExchange.test.ts
index 8bfc0550..ae3b14d1 100644
--- a/exchanges/graphcache/src/cacheExchange.test.ts
+++ b/exchanges/graphcache/src/cacheExchange.test.ts
@@ -1545,7 +1545,7 @@ describe('commutativity', () => {
     });
 
     expect(reexec).toHaveBeenCalledTimes(2);
-    expect(data).toHaveProperty('node.name', 'query b');
+    expect(data).toHaveProperty('node.name', 'mutation');
   });
 
   it('applies optimistic updates on top of commutative queries', () => {
diff --git a/exchanges/graphcache/src/store/data.test.ts b/exchanges/graphcache/src/store/data.test.ts
index 361f0ffc..026750f9 100644
--- a/exchanges/graphcache/src/store/data.test.ts
+++ b/exchanges/graphcache/src/store/data.test.ts
@@ -240,10 +240,43 @@ describe('commutative changes', () => {
 
     InMemoryData.initDataState(data, null);
     expect(InMemoryData.readRecord('Query', 'index')).toBe(1);
+    InMemoryData.clearDataState();
 
     expect(data.optimisticOrder).toEqual([]);
   });
 
+  it('discards optimistic order when concrete data is written', () => {
+    InMemoryData.reserveLayer(data, 1);
+    InMemoryData.reserveLayer(data, 2);
+    InMemoryData.reserveLayer(data, 3);
+
+    InMemoryData.initDataState(data, 2, true);
+    InMemoryData.writeRecord('Query', 'index', 2);
+    InMemoryData.writeRecord('Query', 'optimistic', true);
+    InMemoryData.clearDataState();
+
+    InMemoryData.initDataState(data, 3);
+    InMemoryData.writeRecord('Query', 'index', 3);
+    InMemoryData.clearDataState();
+
+    // Expect Layer 3
+    expect(data.optimisticOrder).toEqual([3, 2, 1]);
+    InMemoryData.initDataState(data, null);
+    expect(InMemoryData.readRecord('Query', 'index')).toBe(3);
+    expect(InMemoryData.readRecord('Query', 'optimistic')).toBe(true);
+
+    // Write 2 again
+    InMemoryData.initDataState(data, 2);
+    InMemoryData.writeRecord('Query', 'index', 2);
+    InMemoryData.clearDataState();
+
+    // 2 has moved in front of 3
+    expect(data.optimisticOrder).toEqual([2, 3, 1]);
+    InMemoryData.initDataState(data, null);
+    expect(InMemoryData.readRecord('Query', 'index')).toBe(2);
+    expect(InMemoryData.readRecord('Query', 'optimistic')).toBe(undefined);
+  });
+
   it('overrides data using optimistic layers', () => {
     InMemoryData.reserveLayer(data, 1);
     InMemoryData.reserveLayer(data, 2);