Repository: urql-graphql/urql
Pull request #3228 (2023-05-30 20:11:16)

fix(graphcache): Fix reference equality of lists not always being preserved
## Summary

A prior fix in #3165 prevented us from mixing API and Graphcache’s data. While we previously avoided modifying API data or prior results from Graphcache, this created new corner cases that were hard to cover.

Specifically, nested lists are hard to diff against prior results as they can be re-entered, `null` must be skipped, and there’s more cases where comparing data and returning previous results could be dangerous and lead to the wrong outcome.

This PR aims to start addressing this issue but may forget to cover some corner cases.

## Set of changes

- Restore and create lists against prior data


vercel[bot]: [vc]: #raKWOoScJC5iv46cHxyuz14yI/9KLJGZEQ1siYpRva4=:eyJpc01vbm9yZXBvIjp0cnVlLCJ0eXBlIjoiZ2l0aHViIiwicHJvamVjdHMiOlt7Im5hbWUiOiJ1cnFsLWRvY3MiLCJpbnNwZWN0b3JVcmwiOiJodHRwczovL3ZlcmNlbC5jb20vZm9ybWlkYWJsZS1sYWJzL3VycWwtZG9jcy82VVhyMWlzRDJZRzExUlphZ1k0TEJnWWhwcGtVIiwibmV4dENvbW1pdFN0YXR1cyI6IklHTk9SRUQifV19
**The latest updates on your projects**. Learn more about [Vercel for Git ↗︎](https://vercel.link/github-learn-more)


<details><summary>1 Ignored Deployment</summary>

| Name | Status | Preview | Updated (UTC) |
| :--- | :----- | :------ | :------ |
| **urql-docs** | ⬜️ Ignored ([Inspect](https://vercel.com/formidable-labs/urql-docs/6UXr1isD2YG11RZagY4LBgYhppkU)) |  | May 30, 2023 8:11pm |
</details>



Patch:
diff --git a/.changeset/spicy-comics-shave.md b/.changeset/spicy-comics-shave.md
new file mode 100644
index 00000000..9b9b3c73
--- /dev/null
+++ b/.changeset/spicy-comics-shave.md
@@ -0,0 +1,5 @@
+---
+'@urql/exchange-graphcache': patch
+---
+
+Fix reference equality not being preserved. This is a fix on top of [#3165](https://github.com/urql-graphql/urql/pull/3165), and was previously not addressed to avoid having to test for corner cases that are hard to cover. If you experience issues with this fix, please let us know.
diff --git a/exchanges/graphcache/src/operations/query.ts b/exchanges/graphcache/src/operations/query.ts
index 83f3cf69..cf4d2b5f 100644
--- a/exchanges/graphcache/src/operations/query.ts
+++ b/exchanges/graphcache/src/operations/query.ts
@@ -523,9 +523,9 @@ const resolveResolverResult = (
     const _isListNullable = store.schema
       ? isListNullable(store.schema, typename, fieldName)
       : false;
-    const data = new Array(result.length);
+    const data = InMemoryData.makeData(prevData, true);
     let hasChanged =
-      !isOwnedData ||
+      InMemoryData.currentForeignData ||
       !Array.isArray(prevData) ||
       result.length !== prevData.length;
     for (let i = 0, l = result.length; i < l; i++) {
@@ -561,7 +561,7 @@ const resolveResolverResult = (
   } else if (!isOwnedData && prevData === null) {
     return null;
   } else if (isDataOrKey(result)) {
-    const data = (prevData || InMemoryData.makeData()) as Data;
+    const data = (prevData || InMemoryData.makeData(prevData)) as Data;
     return typeof result === 'string'
       ? readSelection(ctx, result, select, data)
       : readSelection(ctx, key, select, data, result);
@@ -592,11 +592,11 @@ const resolveLink = (
     const _isListNullable = store.schema
       ? isListNullable(store.schema, typename, fieldName)
       : false;
-    const newLink = new Array(link.length);
+    const newLink = InMemoryData.makeData(prevData, true);
     let hasChanged =
-      !isOwnedData ||
+      InMemoryData.currentForeignData ||
       !Array.isArray(prevData) ||
-      newLink.length !== prevData.length;
+      link.length !== prevData.length;
     for (let i = 0, l = link.length; i < l; i++) {
       // Add the current index to the walked path before reading the field's value
       ctx.__internal.path.push(i);
@@ -632,7 +632,7 @@ const resolveLink = (
     ctx,
     link,
     select,
-    (prevData || InMemoryData.makeData()) as Data
+    (prevData || InMemoryData.makeData(prevData)) as Data
   );
 };
 
diff --git a/exchanges/graphcache/src/store/data.ts b/exchanges/graphcache/src/store/data.ts
index 6d936303..7346eb74 100644
--- a/exchanges/graphcache/src/store/data.ts
+++ b/exchanges/graphcache/src/store/data.ts
@@ -8,6 +8,7 @@ import {
   SerializedEntries,
   Dependencies,
   OperationType,
+  DataField,
   Data,
 } from '../types';
 
@@ -59,8 +60,8 @@ export interface InMemoryData {
   storage: StorageAdapter | null;
 }
 
-let currentOwnership: null | WeakSet<Data> = null;
-let currentDataMapping: null | WeakMap<Data, Data> = null;
+let currentOwnership: null | WeakSet<any> = null;
+let currentDataMapping: null | WeakMap<any, any> = null;
 let currentData: null | InMemoryData = null;
 let currentOptimisticKey: null | number = null;
 export let currentOperation: null | OperationType = null;
@@ -68,20 +69,28 @@ export let currentDependencies: null | Dependencies = null;
 export let currentForeignData = false;
 export let currentOptimistic = false;
 
+export function makeData(data: DataField | void, isArray?: false): Data;
+export function makeData(data: DataField | void, isArray: true): DataField[];
+
 /** Creates a new data object unless it's been created in this data run */
-export const makeData = (data?: Data): Data => {
-  let newData: Data;
+export function makeData(data?: DataField | void, isArray?: boolean) {
+  let newData: Data | Data[] | undefined;
   if (data) {
     if (currentOwnership!.has(data)) return data;
-    newData = currentDataMapping!.get(data) || ({} as Data);
+    newData = currentDataMapping!.get(data) as any;
+  }
+
+  if (newData == null) {
+    newData = (isArray ? [] : {}) as any;
+  }
+
+  if (data) {
     currentDataMapping!.set(data, newData);
-  } else {
-    newData = {} as Data;
   }
 
   currentOwnership!.add(newData);
   return newData;
-};
+}
 
 export const ownsData = (data?: Data): boolean =>
   !!data && currentOwnership!.has(data);
diff --git a/packages/site/vercel.json b/packages/site/vercel.json
new file mode 100644
index 00000000..7ae9a3de
--- /dev/null
+++ b/packages/site/vercel.json
@@ -0,0 +1,5 @@
+{
+  "github": {
+    "silent": true
+  }
+}


Test patch:
diff --git a/exchanges/graphcache/src/operations/query.test.ts b/exchanges/graphcache/src/operations/query.test.ts
index e90d9639..6c99ddf4 100644
--- a/exchanges/graphcache/src/operations/query.test.ts
+++ b/exchanges/graphcache/src/operations/query.test.ts
@@ -363,10 +363,14 @@ describe('Query', () => {
     expect(previousData).toHaveProperty('todos.0.textB', 'old');
   });
 
-  it('should keep references stable', () => {
+  it('should keep references stable (1)', () => {
     const QUERY = gql`
       query todos {
         __typename
+        todos {
+          __typename
+          test
+        }
         todos {
           __typename
           id
@@ -383,14 +387,17 @@ describe('Query', () => {
         {
           __typename: 'Todo',
           id: '0',
+          test: '0',
         },
         {
           __typename: 'Todo',
           id: '1',
+          test: '1',
         },
         {
           __typename: 'Todo',
           id: '2',
+          test: '2',
         },
       ],
       __typename: 'query_root',
@@ -405,15 +412,18 @@ describe('Query', () => {
         todos: [
           {
             __typename: 'Todo',
-            id: 'prev-0',
+            id: '0',
+            test: 'prev-0',
           },
           {
             __typename: 'Todo',
             id: '1',
+            test: '1',
           },
           {
             __typename: 'Todo',
             id: '2',
+            test: '2',
           },
         ],
         __typename: 'query_root',
@@ -426,5 +436,87 @@ describe('Query', () => {
     expect(prevData.todos[0]).toBe(data.todos[0]);
     expect(prevData.todos[1]).toBe(data.todos[1]);
     expect(prevData.todos[2]).toBe(data.todos[2]);
+    expect(prevData.todos).toBe(data.todos);
+    expect(prevData).toBe(data);
+  });
+
+  it('should keep references stable (negative test)', () => {
+    const QUERY = gql`
+      query todos {
+        __typename
+        todos {
+          __typename
+          id
+        }
+        todos {
+          __typename
+          test
+        }
+      }
+    `;
+
+    const store = new Store({
+      schema: alteredRoot,
+    });
+
+    const expected = {
+      todos: [
+        {
+          __typename: 'Todo',
+          id: '0',
+          test: '0',
+        },
+        {
+          __typename: 'Todo',
+          id: '1',
+          test: '1',
+        },
+        {
+          __typename: 'Todo',
+          id: '2',
+          test: '2',
+        },
+      ],
+      __typename: 'query_root',
+    };
+
+    write(store, { query: QUERY }, expected);
+
+    const prevData = query(
+      store,
+      { query: QUERY },
+      {
+        todos: [
+          {
+            __typename: 'Todo',
+            id: '0',
+            test: 'prev-0',
+          },
+          {
+            __typename: 'Todo',
+            id: '1',
+            test: '1',
+          },
+          {
+            __typename: 'Todo',
+            id: '2',
+            test: '2',
+          },
+        ],
+        __typename: 'query_root',
+      }
+    ).data as any;
+
+    expected.todos[0].test = 'x';
+    write(store, { query: QUERY }, expected);
+
+    const data = query(store, { query: QUERY }, prevData).data as any;
+    expect(data).toEqual(expected);
+
+    expect(prevData.todos[1]).toBe(data.todos[1]);
+    expect(prevData.todos[2]).toBe(data.todos[2]);
+    expect(prevData.todos[0]).not.toBe(data.todos[0]);
+    expect(prevData.todos).not.toBe(data.todos);
+    expect(prevData).not.toBe(data);
   });
 });