(graphcache) - Reenable cumulative optimistic updates (#1074)

* Add failing test case for cumulative optimistic updates * Restrict `currentIgnoreOptimistic` condition to read * Fix condition to still skip persistence runs * Add changeset * Ensure that persistData isn't accidentally reading optimistic data This is just to prevent us from messing this up in the future.
urql-graphql · Oct 20, 2020 · 28afd23 · 28afd23
1 parent a770f18
commit 28afd23
Show file tree

Hide file tree

Showing 4 changed files with 81 additions and 11 deletions.
diff --git a/.changeset/ten-turkeys-appear.md 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
@@ -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];

diff --git 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
@@ -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' },
+    ],
+  });
+});