From 7282a5b9e8bb7e39c486a96c08f3f049342398e0 Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Mon, 16 Mar 2020 15:01:00 -0400 Subject: [PATCH] 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. --- src/__tests__/ApolloClient.ts | 9 +-- src/__tests__/client.ts | 46 ++---------- src/__tests__/mutationResults.ts | 78 ++++++++++---------- src/cache/inmemory/__tests__/roundtrip.ts | 9 +-- src/cache/inmemory/__tests__/writeToStore.ts | 39 +++------- src/cache/inmemory/writeToStore.ts | 17 +---- src/core/QueryManager.ts | 23 ++++-- src/utilities/graphql/directives.ts | 9 ++- src/utilities/testing/wrap.ts | 13 ---- 9 files changed, 89 insertions(+), 154 deletions(-) diff --git a/src/__tests__/ApolloClient.ts b/src/__tests__/ApolloClient.ts index b854ee46c8b..052d51738f9 100644 --- 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 index 1517e0cbf7d..59294e1d65b 100644 --- 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 index 56ef7ad3488..1103d45781e 100644 --- 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 index ba6a75796d0..c56afc07868 100644 --- 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 index c481cc9703b..665a1721225 100644 --- 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 index de617f5dddf..8b4ea9572be 100644 --- 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 { 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 index 240021df4d4..22c4b90c929 100644 --- a/src/core/QueryManager.ts +++ b/src/core/QueryManager.ts @@ -239,14 +239,21 @@ export class QueryManager { 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; diff --git a/src/utilities/graphql/directives.ts b/src/utilities/graphql/directives.ts index 31658f1be16..9f5d10748f9 100644 --- a/src/utilities/graphql/directives.ts +++ b/src/utilities/graphql/directives.ts @@ -8,6 +8,7 @@ import { DocumentNode, ArgumentNode, ValueNode, + ASTNode, } from 'graphql'; import { visit } from 'graphql/language/visitor'; @@ -42,10 +43,10 @@ export function shouldInclude( }); } -export function getDirectiveNames(doc: DocumentNode) { +export function getDirectiveNames(root: ASTNode) { const names: string[] = []; - visit(doc, { + visit(root, { Directive(node) { names.push(node.name.value); }, @@ -54,8 +55,8 @@ export function getDirectiveNames(doc: DocumentNode) { return names; } -export function hasDirectives(names: string[], doc: DocumentNode) { - return getDirectiveNames(doc).some( +export function hasDirectives(names: string[], root: ASTNode) { + return getDirectiveNames(root).some( (name: string) => names.indexOf(name) > -1, ); } diff --git a/src/utilities/testing/wrap.ts b/src/utilities/testing/wrap.ts index 6e635a29903..6c020a3ef86 100644 --- a/src/utilities/testing/wrap.ts +++ b/src/utilities/testing/wrap.ts @@ -11,19 +11,6 @@ export default ( } }; -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 => { - expect(message).toMatch(regex); - console.warn = oldWarn; - return val; - }); -} - export function withError(func: Function, regex: RegExp) { let message: string = null as never; const oldError = console.error;