Throw when writing inconsistent query/data to InMemoryCache. (#6055)

The changes in src/cache/inmemory/writeToStore.ts make it an error rather than just a warning to write a result into the InMemoryCache with missing data fields, addressing an old XXX comment that was grandfathered into the StoreWriter class. This will be a **breaking change** for any code that was previously ignoring the warning, which means this change is appropriate for Apollo Client 3.0, but likely will not require any migration steps for most applications. While updating the relevant tests, I realized that the withWarning pattern could (and should) be eliminated.
apollographql · Mar 17, 2020 · 281d1db · 281d1db
1 parent f0fc1e7
commit 281d1db
Show file tree

Hide file tree

Showing 15 changed files with 101 additions and 163 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -38,6 +38,9 @@
 - **[BREAKING]** Apollo Client 2.x allowed `@client` fields to be passed into the `link` chain if `resolvers` were not set in the constructor. This allowed `@client` fields to be passed into Links like `apollo-link-state`. Apollo Client 3 enforces that `@client` fields are local only, meaning they are no longer passed into the `link` chain, under any circumstances.  <br/>
   [@hwillson](https://github.com/hwillson) in [#5982](https://github.com/apollographql/apollo-client/pull/5982)
 
+- **[BREAKING]** `InMemoryCache` now _throws_ when data with missing or undefined query fields is written into the cache, rather than just warning in development. <br/>
+  [@benjamn](https://github.com/benjamn) in [#6055](https://github.com/apollographql/apollo-client/pull/6055)
+
 - **[BREAKING]** `client|cache.writeData` have been fully removed. `writeData` usage is one of the easiest ways to turn faulty assumptions about how the cache represents data internally, into cache inconsistency and corruption. `client|cache.writeQuery`, `client|cache.writeFragment`, and/or `cache.modify` can be used to update the cache.  <br/>
   [@benjamn](https://github.com/benjamn) in [#5923](https://github.com/apollographql/apollo-client/pull/5923)
 

diff --git a/src/__tests__/ApolloClient.ts b/src/__tests__/ApolloClient.ts
@@ -6,7 +6,6 @@ import { ApolloLink } from '../link/core/ApolloLink';
 import { HttpLink } from '../link/http/HttpLink';
 import { InMemoryCache } from '../cache/inmemory/inMemoryCache';
 import { stripSymbols } from '../utilities/testing/stripSymbols';
-import { withWarning } from '../utilities/testing/wrap';
 import { ApolloClient } from '../';
 import { DefaultOptions } from '../ApolloClient';
 import { FetchPolicy, QueryOptions } from '../core/watchQueryOptions';
@@ -827,7 +826,7 @@ describe('ApolloClient', () => {
         }),
       });
 
-      return withWarning(() => {
+      expect(() => {
         client.writeQuery({
           data: {
             todos: [
@@ -848,7 +847,7 @@ describe('ApolloClient', () => {
             }
           `,
         });
-      }, /Missing field description/);
+      }).toThrowError(/Missing field 'description' /);
     });
   });
 
@@ -1109,7 +1108,7 @@ describe('ApolloClient', () => {
         }),
       });
 
-      return withWarning(() => {
+      expect(() => {
         client.writeFragment({
           data: { __typename: 'Bar', i: 10 },
           id: 'bar',
@@ -1120,7 +1119,7 @@ describe('ApolloClient', () => {
             }
           `,
         });
-      }, /Missing field e/);
+      }).toThrowError(/Missing field 'e' /);
     });
 
     describe('change will call observable next', () => {

diff --git a/src/__tests__/client.ts b/src/__tests__/client.ts
@@ -11,7 +11,6 @@ import { WatchQueryOptions, FetchPolicy, WatchQueryFetchPolicy } from '../core/w
 import { ApolloError } from '../errors/ApolloError';
 import { ApolloClient } from '..';
 import subscribeAndCount from '../utilities/testing/subscribeAndCount';
-import { withWarning } from '../utilities/testing/wrap';
 import { itAsync } from '../utilities/testing/itAsync';
 import { mockSingleLink } from '../utilities/testing/mocking/mockLink';
 import { NetworkStatus, ObservableQuery } from '../core';
@@ -625,41 +624,6 @@ describe('client', () => {
     });
   });
 
-  itAsync.skip('should pass a network error correctly on a query using an observable network interface with a warning', (resolve, reject) => {
-    withWarning(() => {
-      const query = gql`
-        query people {
-          allPeople(first: 1) {
-            people {
-              name
-            }
-          }
-        }
-      `;
-
-      const networkError = new Error('Some kind of network error.');
-
-      const link = ApolloLink.from([
-        () => {
-          return new Observable(_ => {
-            throw networkError;
-          });
-        },
-      ]);
-
-      const client = new ApolloClient({
-        link,
-        cache: new InMemoryCache({ addTypename: false }),
-      });
-
-      client.query({ query }).catch((error: ApolloError) => {
-        expect(error.networkError).toBeDefined();
-        expect(error.networkError!.message).toEqual(networkError.message);
-        resolve();
-      });
-    }, /deprecated/);
-  });
-
   itAsync('should pass a network error correctly on a query with apollo-link network interface', (resolve, reject) => {
     const query = gql`
       query people {
@@ -2620,9 +2584,13 @@ describe('client', () => {
       }),
     });
 
-    return withWarning(
-      () => client.query({ query }),
-      /Missing field description/,
+    return client.query({ query }).then(
+      result => {
+        fail("should have errored");
+      },
+      error => {
+        expect(error.message).toMatch(/Missing field 'description' /);
+      },
     ).then(resolve, reject);
   });
 

diff --git a/src/__tests__/mutationResults.ts b/src/__tests__/mutationResults.ts
@@ -6,7 +6,6 @@ import { ApolloLink } from '../link/core/ApolloLink';
 import { mockSingleLink } from '../utilities/testing/mocking/mockLink';
 import { ApolloClient } from '..';
 import { InMemoryCache } from '../cache/inmemory/inMemoryCache';
-import { withWarning } from '../utilities/testing/wrap';
 import { itAsync } from '../utilities/testing/itAsync';
 
 describe('mutation results', () => {
@@ -358,45 +357,50 @@ describe('mutation results', () => {
       },
     };
 
-    return withWarning(() => {
-      const { client, obsQuery } = setupObsQuery(
-        reject,
-        {
-          request: { query: queryTodos },
-          result: queryTodosResult,
-        },
-        {
-          request: { query: mutationTodo },
-          result: mutationTodoResult,
-        },
-      );
+    const { client, obsQuery } = setupObsQuery(
+      reject,
+      {
+        request: { query: queryTodos },
+        result: queryTodosResult,
+      },
+      {
+        request: { query: mutationTodo },
+        result: mutationTodoResult,
+      },
+    );
 
-      return obsQuery.result().then(() => {
-        // we have to actually subscribe to the query to be able to update it
-        return new Promise(resolve => {
-          handle = client.watchQuery({ query: queryTodos });
-          subscriptionHandle = handle.subscribe({
-            next(res: any) {
-              counter++;
-              resolve(res);
-            },
-          });
-        });
-      }).then(() => client.mutate({
-        mutation: mutationTodo,
-        updateQueries: {
-          todos: (prev, { mutationResult }) => {
-            const newTodo = (mutationResult as any).data.createTodo;
-            const newResults = {
-              todos: [...(prev as any).todos, newTodo],
-            };
-            return newResults;
+    return obsQuery.result().then(() => {
+      // we have to actually subscribe to the query to be able to update it
+      return new Promise(resolve => {
+        handle = client.watchQuery({ query: queryTodos });
+        subscriptionHandle = handle.subscribe({
+          next(res: any) {
+            counter++;
+            resolve(res);
           },
+        });
+      });
+    }).then(() => client.mutate({
+      mutation: mutationTodo,
+      updateQueries: {
+        todos: (prev, { mutationResult }) => {
+          const newTodo = (mutationResult as any).data.createTodo;
+          const newResults = {
+            todos: [...(prev as any).todos, newTodo],
+          };
+          return newResults;
         },
-      })).then(
-        () => subscriptionHandle.unsubscribe()
-      ).then(resolve, reject);
-    }, /Missing field description/);
+      },
+    })).then(
+      () => {
+        subscriptionHandle.unsubscribe();
+        fail("should have errored");
+      },
+      error => {
+        subscriptionHandle.unsubscribe();
+        expect(error.message).toMatch(/Missing field 'description' /);
+      },
+    ).then(resolve, reject);
   });
 
   describe('updateQueries', () => {

diff --git a/src/cache/inmemory/__tests__/entityStore.ts b/src/cache/inmemory/__tests__/entityStore.ts
@@ -1170,8 +1170,8 @@ describe('EntityStore', () => {
       c: 3,
     })).toBe('ABCs:{"b":2,"a":1,"c":3}');
 
-    expect(() => cache.identify(ABCs)).toThrow(
-      "Missing field b while computing key fields",
+    expect(() => cache.identify(ABCs)).toThrowError(
+      "Missing field 'b' while computing key fields",
     );
 
     expect(cache.readFragment({

diff --git a/src/cache/inmemory/__tests__/policies.ts b/src/cache/inmemory/__tests__/policies.ts
@@ -281,7 +281,7 @@ describe("type policies", function () {
           book: theInformationBookData,
         },
       });
-    }).toThrow("Missing field year while computing key fields");
+    }).toThrowError("Missing field 'year' while computing key fields");
   });
 
   describe("field policies", function () {
@@ -1859,7 +1859,7 @@ describe("type policies", function () {
             }
           `
         });
-      }).toThrow("Can't find field secret");
+      }).toThrowError("Can't find field 'secret' ");
 
       expect(secretReadAttempted).toBe(true);
     });

diff --git a/src/cache/inmemory/__tests__/readFromStore.ts b/src/cache/inmemory/__tests__/readFromStore.ts
@@ -553,7 +553,7 @@ describe('reading from the store', () => {
           }
         `,
       });
-    }).toThrowError(/Can't find field missingField on ROOT_QUERY object/);
+    }).toThrowError(/Can't find field 'missingField' on ROOT_QUERY object/);
   });
 
   it('runs a nested query where the reference is null', () => {

diff --git a/src/cache/inmemory/__tests__/roundtrip.ts b/src/cache/inmemory/__tests__/roundtrip.ts
@@ -2,7 +2,6 @@ import { DocumentNode } from 'graphql';
 import gql from 'graphql-tag';
 
 import { withError } from './diffAgainstStore';
-import { withWarning } from './writeToStore';
 import { EntityStore } from '../entityStore';
 import { StoreReader } from '../readFromStore';
 import { StoreWriter } from '../writeToStore';
@@ -311,7 +310,7 @@ describe('roundtrip', () => {
     // XXX this test is weird because it assumes the server returned an incorrect result
     // However, the user may have written this result with client.writeQuery.
     it('should throw an error on two of the same inline fragment types', () => {
-      return withWarning(() => expect(() => {
+      expect(() => {
         storeRoundtrip(
           gql`
             query {
@@ -337,7 +336,7 @@ describe('roundtrip', () => {
             ],
           },
         );
-      }).toThrowError(/Can\'t find field rank on object/));
+      }).toThrowError(/Missing field 'rank' /);
     });
 
     it('should resolve fields it can on interface with non matching inline fragments', () => {
@@ -451,7 +450,7 @@ describe('roundtrip', () => {
     });
 
     it('should throw on error on two of the same spread fragment types', () => {
-      withWarning(() => expect(() => {
+      expect(() => {
         storeRoundtrip(
           gql`
             fragment jediSide on Jedi {
@@ -481,7 +480,7 @@ describe('roundtrip', () => {
             ],
           },
         );
-      }).toThrowError(/Can\'t find field rank on object/));
+      }).toThrowError(/Missing field 'rank' /);
     });
 
     it('should resolve on @include and @skip with inline fragments', () => {

diff --git a/src/cache/inmemory/__tests__/writeToStore.ts b/src/cache/inmemory/__tests__/writeToStore.ts
@@ -20,21 +20,6 @@ import { defaultNormalizedCacheFactory } from '../entityStore';
 import { InMemoryCache } from '../inMemoryCache';
 import { Policies } from '../policies';
 
-export function withWarning(func: Function, regex?: RegExp) {
-  let message: string = null as never;
-  const oldWarn = console.warn;
-
-  console.warn = (m: string) => (message = m);
-
-  return Promise.resolve(func()).then(val => {
-    if (regex) {
-      expect(message).toMatch(regex);
-    }
-    console.warn = oldWarn;
-    return val;
-  });
-}
-
 const getIdField = ({ id }: { id: string }) => id;
 
 describe('writing to the store', () => {
@@ -1563,14 +1548,12 @@ describe('writing to the store', () => {
         }),
       });
 
-      return withWarning(() => {
-        const newStore = writer.writeQueryToStore({
+      expect(() => {
+        writer.writeQueryToStore({
           query,
           result,
         });
-
-        expect((newStore as any).lookup('1')).toEqual(result.todos[0]);
-      }, /Missing field description/);
+      }).toThrowError(/Missing field 'description' /);
     });
 
     it('should warn when it receives the wrong data inside a fragment', () => {
@@ -1617,14 +1600,12 @@ describe('writing to the store', () => {
         }),
       });
 
-      return withWarning(() => {
-        const newStore = writer.writeQueryToStore({
+      expect(() => {
+        writer.writeQueryToStore({
           query: queryWithInterface,
           result,
         });
-
-        expect((newStore as any).lookup('1')).toEqual(result.todos[0]);
-      }, /Missing field price/);
+      }).toThrowError(/Missing field 'price' /);
     });
 
     it('should warn if a result is missing __typename when required', () => {
@@ -1645,14 +1626,12 @@ describe('writing to the store', () => {
         }),
       });
 
-      return withWarning(() => {
-        const newStore = writer.writeQueryToStore({
+      expect(() => {
+        writer.writeQueryToStore({
           query: addTypenameToDocument(query),
           result,
         });
-
-        expect((newStore as any).lookup('1')).toEqual(result.todos[0]);
-      }, /Missing field __typename/);
+      }).toThrowError(/Missing field '__typename' /);
     });
 
     it('should not warn if a field is null', () => {

diff --git a/src/cache/inmemory/policies.ts b/src/cache/inmemory/policies.ts
@@ -785,7 +785,7 @@ function computeKeyObject(
       invariant(
         hasOwn.call(response, responseName),
         // TODO Make this appropriate for keyArgs as well
-        `Missing field ${responseName} while computing key fields`,
+        `Missing field '${responseName}' while computing key fields`,
       );
       keyObj[prevKey = s] = response[responseName];
     }

diff --git a/src/cache/inmemory/readFromStore.ts b/src/cache/inmemory/readFromStore.ts
@@ -242,9 +242,9 @@ export class StoreReader {
 
         if (fieldValue === void 0) {
           if (!addTypenameToDocument.added(selection)) {
-            getMissing().push(new InvariantError(`Can't find field ${
+            getMissing().push(new InvariantError(`Can't find field '${
               selection.name.value
-            } on ${
+            }' on ${
               isReference(objectOrReference)
                 ? objectOrReference.__ref + " object"
                 : "object " + JSON.stringify(objectOrReference, null, 2)