Repository: FormidableLabs/urql
Pull request #1074 (2020-10-20 12:53:09)

(graphcache) - Reenable cumulative optimistic updates
This was discovered in this Discussions thread: https://github.com/FormidableLabs/urql/discussions/1073

## Summary

In #866 we previously disallowed optimistic updates to read other optimistically written data, which meant to prevent conflicts between these data layers that will later trickle down into permanent layers. This isn't a concern anymore because optimistic layers will never contribute to concrete, non-optimistic data and are thrown away, if they're for optimistic mutations.

The edge case we aimed to prevent is also not possible anymore because we now never apply concrete updates on top of optimistic ones since #750, namely:

> - Block refetching of operations that have active optimistic updates and keep track of them (see above)
> - Keep track of optimistic mutation that are pending at the same time, and flush their results simultaneously, also erasing the optimistically updated (i.e. blocked) dependencies at the same time

I'm not 100% sure anymore at this time, why we deemed this unsafe before, but it shouldn't be.

## Set of changes

- Allow optimistic updates to see other optimistically written data
- Rename `currentIgnoreOptimistic` to just `currentOptimistic`
- Fix `getNode` to allow `currentOptimistic` to remain `true` so that `updatePersist` doesn't queue up noop updates for optimistic mutations

changeset-bot[bot]: ###  🦋  Changeset detected

Latest commit: 16c0f581cd24fcde6cbf15c3f8be8ccc6c41d08f

**The changes in this PR will be included in the next version bump.**

<details><summary>This PR includes changesets to release 1 package</summary>

  | Name                      | Type  |
| ------------------------- | ----- |
| @urql/exchange-graphcache | Patch |

</details>

Not sure what this means? [Click here  to learn what changesets are](https://github.com/changesets/changesets/blob/master/docs/adding-a-changeset.md).

[Click here if you're a maintainer who wants to add another changeset to this PR](https://github.com/FormidableLabs/urql/new/fix/cumulative-optimistic-updates?filename=.changeset/ninety-llamas-push.md&value=---%0A%22%40urql%2Fexchange-graphcache%22%3A%20patch%0A---%0A%0A(graphcache)%20-%20Reenable%20cumulative%20optimistic%20updates%0A)



Patch:
diff --git a/.changeset/ten-turkeys-appear.md b/.changeset/ten-turkeys-appear.md
new file mode 100644
index 00000000..3ac9a9d8
--- /dev/null
+++ b/.changeset/ten-turkeys-appear.md
@@ -0,0 +1,5 @@
+---
+'@urql/exchange-graphcache': patch
+---
+
+Fix optimistic updates not being allowed to be cumulative and apply on top of each other. Previously in [#866](https://github.com/FormidableLabs/urql/pull/866) we explicitly deemed this as unsafe which isn't correct anymore given that concrete, non-optimistic updates are now never applied on top of optimistic layers.
diff --git a/exchanges/graphcache/src/store/data.ts b/exchanges/graphcache/src/store/data.ts
index d1d31ab0..aef31eba 100644
--- a/exchanges/graphcache/src/store/data.ts
+++ b/exchanges/graphcache/src/store/data.ts
@@ -58,7 +58,7 @@ let currentOperation: null | OperationType = null;
 let currentData: null | InMemoryData = null;
 let currentDependencies: null | Dependencies = null;
 let currentOptimisticKey: null | number = null;
-let currentIgnoreOptimistic = false;
+let currentOptimistic = false;
 
 const makeNodeMap = <T>(): NodeMap<T> => ({
   optimistic: makeDict(),
@@ -75,7 +75,7 @@ export const initDataState = (
   currentOperation = operationType;
   currentData = data;
   currentDependencies = makeDict();
-  currentIgnoreOptimistic = !!isOptimistic;
+  currentOptimistic = !!isOptimistic;
   if (process.env.NODE_ENV !== 'production') {
     currentDebugStack.length = 0;
   }
@@ -96,7 +96,7 @@ export const initDataState = (
 
     // 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
+    // of a commutative chain
     currentOptimisticKey = layerKey;
     createLayer(data, layerKey);
   } else {
@@ -116,7 +116,7 @@ export const clearDataState = () => {
 
   const data = currentData!;
   const layerKey = currentOptimisticKey;
-  currentIgnoreOptimistic = false;
+  currentOptimistic = false;
   currentOptimisticKey = null;
 
   // Determine whether the current operation has been a commutative layer
@@ -144,7 +144,7 @@ export const clearDataState = () => {
   if (process.env.NODE_ENV !== 'test' && !data.defer) {
     data.defer = true;
     Promise.resolve().then(() => {
-      initDataState('write', data, null);
+      initDataState('read', data, null);
       gc();
       persistData();
       clearDataState();
@@ -246,7 +246,8 @@ const getNode = <T>(
     // If the node and node value exists it is returned, including undefined
     if (
       optimistic &&
-      (!currentIgnoreOptimistic ||
+      (!currentOptimistic ||
+        currentOperation === 'write' ||
         currentData!.commutativeKeys.has(layerKey)) &&
       (node = optimistic.get(entityKey)) !== undefined &&
       fieldKey in node
@@ -383,7 +384,7 @@ const updateDependencies = (entityKey: string, fieldKey?: string) => {
 };
 
 const updatePersist = (entityKey: string, fieldKey: string) => {
-  if (!currentIgnoreOptimistic && currentData!.storage) {
+  if (!currentOptimistic && currentData!.storage) {
     currentData!.persist.add(serializeKeys(entityKey, fieldKey));
   }
 };
@@ -557,7 +558,8 @@ export const inspectFields = (entityKey: string): FieldInfo[] => {
 
 export const persistData = () => {
   if (currentData!.storage) {
-    currentIgnoreOptimistic = true;
+    currentOptimistic = true;
+    currentOperation = 'read';
     const entries: SerializedEntries = makeDict();
     currentData!.persist.forEach(key => {
       const { entityKey, fieldKey } = deserializeKeyInfo(key);
@@ -571,7 +573,7 @@ export const persistData = () => {
       }
     });
 
-    currentIgnoreOptimistic = false;
+    currentOptimistic = false;
     currentData!.storage.writeData(entries);
     currentData!.persist.clear();
   }
@@ -582,7 +584,7 @@ export const hydrateData = (
   storage: StorageAdapter,
   entries: SerializedEntries
 ) => {
-  initDataState('read', data, null);
+  initDataState('write', data, null);
 
   for (const key in entries) {
     const value = entries[key];


Test patch:
diff --git a/exchanges/graphcache/src/store/store.test.ts b/exchanges/graphcache/src/store/store.test.ts
index e0a62ba5..60d00693 100644
--- a/exchanges/graphcache/src/store/store.test.ts
+++ b/exchanges/graphcache/src/store/store.test.ts
@@ -783,7 +783,7 @@ describe('Store with storage', () => {
     InMemoryData.writeRecord('Query', 'base', false);
     InMemoryData.clearDataState();
 
-    InMemoryData.initDataState('write', store.data, null);
+    InMemoryData.initDataState('read', store.data, null);
     expect(InMemoryData.readRecord('Query', 'base')).toBe(false);
     InMemoryData.persistData();
     InMemoryData.clearDataState();
diff --git a/exchanges/graphcache/src/test-utils/examples-1.test.ts b/exchanges/graphcache/src/test-utils/examples-1.test.ts
index 23c4bf8d..b3a7031f 100644
--- a/exchanges/graphcache/src/test-utils/examples-1.test.ts
+++ b/exchanges/graphcache/src/test-utils/examples-1.test.ts
@@ -489,3 +489,66 @@ it('correctly resolves optimistic updates on Relay schemas', () => {
   expect(queryRes.partial).toBe(false);
   expect(queryRes.data).not.toBe(null);
 });
+
+it('allows cumulative optimistic updates', () => {
+  let counter = 1;
+
+  const store = new Store({
+    updates: {
+      Mutation: {
+        addTodo: (result, _, cache) => {
+          cache.updateQuery({ query: Todos }, data => {
+            (data as any).todos.push(result.addTodo);
+            return data as Data;
+          });
+        },
+      },
+    },
+    optimistic: {
+      addTodo: () => ({
+        __typename: 'Todo',
+        id: 'optimistic_' + ++counter,
+        text: '',
+        complete: false,
+      }),
+    },
+  });
+
+  const todosData = {
+    __typename: 'Query',
+    todos: [
+      { id: '0', complete: true, text: '0', __typename: 'Todo' },
+      { id: '1', complete: true, text: '1', __typename: 'Todo' },
+    ],
+  };
+
+  write(store, { query: Todos }, todosData);
+
+  const AddTodo = gql`
+    mutation {
+      __typename
+      addTodo {
+        __typename
+        complete
+        text
+        id
+      }
+    }
+  `;
+
+  writeOptimistic(store, { query: AddTodo }, 1);
+  writeOptimistic(store, { query: AddTodo }, 2);
+
+  const queryRes = query(store, { query: Todos });
+
+  expect(queryRes.partial).toBe(false);
+  expect(queryRes.data).toEqual({
+    ...todosData,
+    todos: [
+      todosData.todos[0],
+      todosData.todos[1],
+      { __typename: 'Todo', text: '', complete: false, id: 'optimistic_2' },
+      { __typename: 'Todo', text: '', complete: false, id: 'optimistic_3' },
+    ],
+  });
+});