Repository: urql-graphql/urql
Pull request #3265 (2023-06-12 22:49:55)

fix(graphcache): Prevent marking operations as reexecuting after optimistic mutation
## Summary

Prevent marking operations as reexecuting when an optimistic mutation executes and instead delay rotating them

## Set of changes

- Prevent `reexecutingOperations` rotation after optimistic mutation



Patch:
diff --git a/.changeset/five-icons-agree.md b/.changeset/five-icons-agree.md
new file mode 100644
index 00000000..78eb832e
--- /dev/null
+++ b/.changeset/five-icons-agree.md
@@ -0,0 +1,5 @@
+---
+'@urql/exchange-graphcache': patch
+---
+
+Make "Invalid undefined" warning heuristic smarter and allow for partial optimistic results. Previously, when a partial optimistic result would be passed, a warning would be issued, and in production, fields would be deleted from the cache. Instead, we now only issue a warning if these fields aren't cached already.
diff --git a/.changeset/serious-dots-sit.md b/.changeset/serious-dots-sit.md
new file mode 100644
index 00000000..aebeb000
--- /dev/null
+++ b/.changeset/serious-dots-sit.md
@@ -0,0 +1,5 @@
+---
+'@urql/exchange-graphcache': patch
+---
+
+Optimistic mutation results should never result in dependent operations being blocked.
diff --git a/exchanges/graphcache/src/cacheExchange.ts b/exchanges/graphcache/src/cacheExchange.ts
index 54591c73..7a049ef9 100644
--- a/exchanges/graphcache/src/cacheExchange.ts
+++ b/exchanges/graphcache/src/cacheExchange.ts
@@ -111,7 +111,8 @@ export const cacheExchange =
 
     const executePendingOperations = (
       operation: Operation,
-      pendingOperations: Operations
+      pendingOperations: Operations,
+      isOptimistic: boolean
     ) => {
       // Reexecute collected operations and delete them from the mapping
       for (const key of pendingOperations.values()) {
@@ -130,13 +131,15 @@ export const cacheExchange =
         }
       }
 
-      // Upon completion, all dependent operations become reexecuting operations, preventing
-      // them from reexecuting prior operations again, causing infinite loops
-      const _reexecutingOperations = reexecutingOperations;
-      if (operation.kind === 'query') {
-        (reexecutingOperations = dependentOperations).add(operation.key);
+      if (!isOptimistic) {
+        // Upon completion, all dependent operations become reexecuting operations, preventing
+        // them from reexecuting prior operations again, causing infinite loops
+        const _reexecutingOperations = reexecutingOperations;
+        if (operation.kind === 'query') {
+          (reexecutingOperations = dependentOperations).add(operation.key);
+        }
+        (dependentOperations = _reexecutingOperations).clear();
       }
-      (dependentOperations = _reexecutingOperations).clear();
     };
 
     // This registers queries with the data layer to ensure commutativity
@@ -171,7 +174,7 @@ export const cacheExchange =
           // Update related queries
           const pendingOperations: Operations = new Set();
           collectPendingOperations(pendingOperations, dependencies);
-          executePendingOperations(operation, pendingOperations);
+          executePendingOperations(operation, pendingOperations, true);
         }
       }
 
@@ -424,7 +427,7 @@ export const cacheExchange =
           // Update the cache with the incoming API result
           const cacheResult = updateCacheWithResult(result, pendingOperations);
           // Execute all dependent queries
-          executePendingOperations(result.operation, pendingOperations);
+          executePendingOperations(result.operation, pendingOperations, false);
           return cacheResult;
         })
       );
@@ -458,7 +461,7 @@ export const cacheExchange =
             );
 
           // Execute all dependent queries as a single batch
-          executePendingOperations(result.operation, pendingOperations);
+          executePendingOperations(result.operation, pendingOperations, false);
 
           return fromArray(results);
         })
diff --git a/exchanges/graphcache/src/operations/write.ts b/exchanges/graphcache/src/operations/write.ts
index 33b552e1..02a8f0ff 100644
--- a/exchanges/graphcache/src/operations/write.ts
+++ b/exchanges/graphcache/src/operations/write.ts
@@ -210,7 +210,13 @@ const writeSelection = (
   const rootField = ctx.store.rootNames[entityKey!] || 'query';
   const isRoot = !!ctx.store.rootNames[entityKey!];
 
-  const typename = isRoot ? entityKey : data.__typename;
+  let typename = isRoot ? entityKey : data.__typename;
+  if (!typename && entityKey && ctx.optimistic) {
+    typename = InMemoryData.readRecord(entityKey, '__typename') as
+      | string
+      | undefined;
+  }
+
   if (!typename) {
     warn(
       "Couldn't find __typename when writing.\n" +
@@ -239,34 +245,6 @@ const writeSelection = (
     const fieldAlias = getFieldAlias(node);
     let fieldValue = data[ctx.optimistic ? fieldName : fieldAlias];
 
-    // Development check of undefined fields
-    if (process.env.NODE_ENV !== 'production') {
-      if (
-        rootField === 'query' &&
-        fieldValue === undefined &&
-        !deferRef &&
-        !ctx.optimistic
-      ) {
-        const expected =
-          node.selectionSet === undefined
-            ? 'scalar (number, boolean, etc)'
-            : 'selection set';
-
-        warn(
-          'Invalid undefined: The field at `' +
-            fieldKey +
-            '` is `undefined`, but the GraphQL query expects a ' +
-            expected +
-            ' for this field.',
-          13
-        );
-
-        continue; // Skip this field
-      } else if (ctx.store.schema && typename && fieldName !== '__typename') {
-        isFieldAvailableOnType(ctx.store.schema, typename, fieldName);
-      }
-    }
-
     if (
       // Skip typename fields and assume they've already been written above
       fieldName === '__typename' ||
@@ -278,6 +256,12 @@ const writeSelection = (
       continue;
     }
 
+    if (process.env.NODE_ENV !== 'production') {
+      if (ctx.store.schema && typename && fieldName !== '__typename') {
+        isFieldAvailableOnType(ctx.store.schema, typename, fieldName);
+      }
+    }
+
     // Add the current alias to the walked path before processing the field's value
     ctx.__internal.path.push(fieldAlias);
 
@@ -298,6 +282,32 @@ const writeSelection = (
       fieldValue = ensureData(resolver(fieldArgs || {}, ctx.store, ctx));
     }
 
+    if (fieldValue === undefined) {
+      if (process.env.NODE_ENV !== 'production') {
+        if (
+          !entityKey ||
+          !InMemoryData.hasField(entityKey, fieldKey) ||
+          (ctx.optimistic && !InMemoryData.readRecord(entityKey, '__typename'))
+        ) {
+          const expected =
+            node.selectionSet === undefined
+              ? 'scalar (number, boolean, etc)'
+              : 'selection set';
+
+          warn(
+            'Invalid undefined: The field at `' +
+              fieldKey +
+              '` is `undefined`, but the GraphQL query expects a ' +
+              expected +
+              ' for this field.',
+            13
+          );
+        }
+      }
+
+      continue; // Skip this field
+    }
+
     if (node.selectionSet) {
       // Process the field and write links for the child entities that have been written
       if (entityKey && rootField === 'query') {
@@ -306,7 +316,10 @@ const writeSelection = (
           ctx,
           getSelectionSet(node),
           ensureData(fieldValue),
-          key
+          key,
+          ctx.optimistic
+            ? InMemoryData.readLink(entityKey || typename, fieldKey)
+            : undefined
         );
         InMemoryData.writeLink(entityKey || typename, fieldKey, link);
       } else {
@@ -353,7 +366,8 @@ const writeField = (
   ctx: Context,
   select: SelectionSet,
   data: null | Data | NullArray<Data>,
-  parentFieldKey?: string
+  parentFieldKey?: string,
+  prevLink?: Link
 ): Link | undefined => {
   if (Array.isArray(data)) {
     const newData = new Array(data.length);
@@ -365,7 +379,8 @@ const writeField = (
         ? joinKeys(parentFieldKey, `${i}`)
         : undefined;
       // Recursively write array data
-      const links = writeField(ctx, select, data[i], indexKey);
+      const prevIndex = prevLink != null ? prevLink[i] : undefined;
+      const links = writeField(ctx, select, data[i], indexKey, prevIndex);
       // Link cannot be expressed as a recursive type
       newData[i] = links as string | null;
       // After processing the field, remove the current index from the path
@@ -377,7 +392,9 @@ const writeField = (
     return getFieldError(ctx) ? undefined : null;
   }
 
-  const entityKey = ctx.store.keyOfEntity(data);
+  const entityKey =
+    ctx.store.keyOfEntity(data) ||
+    (typeof prevLink === 'string' ? prevLink : null);
   const typename = data.__typename;
 
   if (


Test patch:
diff --git a/exchanges/graphcache/src/operations/query.test.ts b/exchanges/graphcache/src/operations/query.test.ts
index 6c99ddf4..6e48bd99 100644
--- a/exchanges/graphcache/src/operations/query.test.ts
+++ b/exchanges/graphcache/src/operations/query.test.ts
@@ -164,8 +164,6 @@ describe('Query', () => {
       todos: [{ __typename: 'Todo', id: '0', text: 'Solve bug' }],
     });
 
-    // The warning should be called for `__typename`
-    expect(console.warn).toHaveBeenCalledTimes(1);
     expect(console.error).not.toHaveBeenCalled();
   });
 
diff --git a/exchanges/graphcache/src/operations/write.test.ts b/exchanges/graphcache/src/operations/write.test.ts
index 822809f3..d2b0a5fc 100644
--- a/exchanges/graphcache/src/operations/write.test.ts
+++ b/exchanges/graphcache/src/operations/write.test.ts
@@ -162,15 +162,17 @@ describe('Query', () => {
       }
     `;
 
-    write(store, { query }, { field: 'test' } as any);
     // This should not overwrite the field
     write(store, { query }, { field: undefined } as any);
     // Because of us writing an undefined field
     expect(console.warn).toHaveBeenCalledTimes(2);
-    expect((console.warn as any).mock.calls[0][0]).toMatch(
-      /The field `field` does not exist on `Query`/
+
+    expect((console.warn as any).mock.calls[1][0]).toMatch(
+      /Invalid undefined: The field at `field`/
     );
 
+    write(store, { query }, { field: 'test' } as any);
+    write(store, { query }, { field: undefined } as any);
     InMemoryData.initDataState('read', store.data, null);
     // The field must still be `'test'`
     expect(InMemoryData.readRecord('Query', 'field')).toBe('test');