Throw when writing inconsistent query/data to InMemoryCache.

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 TODO 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 16, 2020 · 05c5fee · 05c5fee
1 parent f0fc1e7
commit 05c5fee
Show file tree

Hide file tree

Showing 9 changed files with 89 additions and 154 deletions.
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__/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/writeToStore.ts b/src/cache/inmemory/writeToStore.ts
@@ -1,5 +1,5 @@
 import { SelectionSetNode, FieldNode, DocumentNode } from 'graphql';
-import { invariant } from 'ts-invariant';
+import { invariant, InvariantError } from 'ts-invariant';
 
 import {
   createFragmentMap,
@@ -22,7 +22,7 @@ import {
   StoreObject,
 } from '../../utilities/graphql/storeUtils';
 
-import { shouldInclude } from '../../utilities/graphql/directives';
+import { shouldInclude, hasDirectives } from '../../utilities/graphql/directives';
 import { cloneDeep } from '../../utilities/common/cloneDeep';
 
 import { Policies, ReadMergeContext } from './policies';
@@ -229,18 +229,9 @@ export class StoreWriter {
 
         } else if (
           policies.usingPossibleTypes &&
-          !(
-            selection.directives &&
-            selection.directives.some(
-              ({ name }) =>
-                name && (name.value === 'defer' || name.value === 'client'),
-            )
-          )
+          !hasDirectives(["defer", "client"], selection)
         ) {
-          // XXX We'd like to throw an error, but for backwards compatibility's sake
-          // we just print a warning for the time being.
-          //throw new WriteError(`Missing field ${resultFieldKey} in ${JSON.stringify(result, null, 2).substring(0, 100)}`);
-          invariant.warn(
+          throw new InvariantError(
             `Missing field ${resultFieldKey} in ${JSON.stringify(
               result,
               null,

diff --git a/src/core/QueryManager.ts b/src/core/QueryManager.ts
@@ -239,14 +239,21 @@ export class QueryManager<TStore> {
           self.mutationStore.markMutationResult(mutationId);
 
           if (fetchPolicy !== 'no-cache') {
-            markMutationResult({
-              mutationId,
-              result,
-              document: mutation,
-              variables,
-              queryUpdatersById: generateUpdateQueriesInfo(),
-              update: updateWithProxyFn,
-            }, self.cache);
+            try {
+              markMutationResult({
+                mutationId,
+                result,
+                document: mutation,
+                variables,
+                queryUpdatersById: generateUpdateQueriesInfo(),
+                update: updateWithProxyFn,
+              }, self.cache);
+            } catch (e) {
+              error = new ApolloError({
+                networkError: e,
+              });
+              return;
+            }
           }
 
           storeResult = result as FetchResult<T>;