Repository: urql-graphql/urql
Pull request #3264 (2023-06-12 19:33:06)

fix(graphcache): Allow partial optimistic results
## Summary

We basically had some logic that would allow writes to be incomplete, however, this didn't apply to optimistic results, which would always cause cached fields to be deleted. Furthermore, in production, partial results will always lead to cached values being deleted from the cache.

Instead, we can apply a better heuristic which only triggers the warning when a field is missing from the cache, but always allow writes to continue, no matter whether values are set or not.

This should in theory be safe, since we're moving potential errors over to reading from the cache.

This also adds a change which makes optimistic mutations read previous keys and `__typename` fields from the cache to complete mutation results from prior cache results. This should tremendously help the developer experience of having to create these functions.

## Set of changes

- Allow `continue` to be applied to `undefined` field values
- Include `ctx.optimistic` in the above check
- Update a test accordingly which checks for `console.warn`s
- Only trigger a warning if the field isn't cached
- Auto-fallback to `__typename` from cached values
- Auto-fallback to keys from cached links



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/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');