Throw when writing inconsistent query/data to InMemoryCache.

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)
 

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', () => {

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);
   });
 

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', () => {

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({

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);
     });

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', () => {

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', () => {

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', () => {

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];
     }

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)