diff --git a/CHANGELOG.md b/CHANGELOG.md index 11d8d7668b0..207672c6402 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,7 +1,7 @@ # Change log ### vNEXT - +- Feature: Print a warning when heuristically matching fragments on interface/union [PR #1635](https://github.com/apollographql/apollo-client/pull/1635) ### 1.1.1 - Fix: Remove ability to set default fetchPolicy, which broke polling queries [PR #1630](https://github.com/apollographql/apollo-client/pull/1630) diff --git a/src/data/fragmentMatcher.ts b/src/data/fragmentMatcher.ts index c830c931cd5..9df503de820 100644 --- a/src/data/fragmentMatcher.ts +++ b/src/data/fragmentMatcher.ts @@ -10,6 +10,10 @@ import { isTest, } from '../util/environment'; +import { + warnOnce, +} from '../util/warnOnce'; + export interface FragmentMatcherInterface { match(idValue: IdValue, typeCondition: string, context: ReadStoreContext): boolean; } @@ -122,6 +126,8 @@ export class HeuristicFragmentMatcher implements FragmentMatcherInterface { console.warn(`You're using fragments in your queries, but don't have the addTypename: true option set in Apollo Client. Please turn on that option so that we can accurately match fragments.`); + console.warn(`DEPRECATION WARNING: using fragments without __typename is unsupported behavior ` + + `and will be removed in future versions of Apollo client. You should fix this and set addTypename to true now.`); /* istanbul ignore if */ if (!isTest()) { @@ -142,7 +148,12 @@ export class HeuristicFragmentMatcher implements FragmentMatcherInterface { // 1. A fragment on a non-matching concrete type or interface or union // 2. A fragment on a matching interface or union // If it's 1, we don't want to return anything, if it's 2 we want to match. We can't tell the - // difference, so for now, we just do our best to resolve the fragment but turn on partial data + // difference, so we warn the user, but still try to match it (backcompat). + warnOnce(`You are using the simple (heuristic) fragment matcher, but your queries contain union or interface types. + Apollo Client will not be able to able to accurately map fragments.` + + `To make this error go away, use the IntrospectionFragmentMatcher as described in the docs: ` + + `http://dev.apollodata.com/react/initialization.html#fragment-matcher`, 'error'); + context.returnPartialData = true; return true; } diff --git a/src/data/writeToStore.ts b/src/data/writeToStore.ts index fe70d6893bd..bb40b478dcf 100644 --- a/src/data/writeToStore.ts +++ b/src/data/writeToStore.ts @@ -152,13 +152,15 @@ export function writeSelectionSetToStore({ const resultFieldKey: string = resultKeyNameFromField(selection); const value: any = result[resultFieldKey]; - if (value !== undefined) { - writeFieldToStore({ - dataId, - value, - field: selection, - context, - }); + if (included) { + if (typeof value !== 'undefined') { + writeFieldToStore({ + dataId, + value, + field: selection, + context, + }); + } } } else if (isInlineFragment(selection)) { if (included) { diff --git a/src/util/warnOnce.ts b/src/util/warnOnce.ts new file mode 100644 index 00000000000..e1ff3f7f59c --- /dev/null +++ b/src/util/warnOnce.ts @@ -0,0 +1,15 @@ + +const haveWarned = Object.create({}); + +export function warnOnce(msg: string, type = 'warn') { + if (!haveWarned[msg]) { + haveWarned[msg] = true; + switch (type) { + case 'error': + console.error(msg); + break; + default: + console.warn(msg); + } + } +} diff --git a/test/ApolloClient.ts b/test/ApolloClient.ts index f9d896cb1ff..9805dd799af 100644 --- a/test/ApolloClient.ts +++ b/test/ApolloClient.ts @@ -504,13 +504,14 @@ describe('ApolloClient', () => { }); client.writeFragment({ - data: { e: 4, h: { id: 'bar', i: 7 } }, + data: { __typename: 'Foo', e: 4, h: { id: 'bar', i: 7 } }, id: 'foo', fragment: gql`fragment fragmentFoo on Foo { e h { i } }`, }); assert.deepEqual(client.store.getState().apollo.data, { 'foo': { + __typename: 'Foo', e: 4, h: { type: 'id', @@ -531,6 +532,7 @@ describe('ApolloClient', () => { assert.deepEqual(client.store.getState().apollo.data, { 'foo': { + __typename: 'Foo', e: 4, f: 5, g: 6, @@ -555,6 +557,7 @@ describe('ApolloClient', () => { assert.deepEqual(client.store.getState().apollo.data, { 'foo': { + __typename: 'Foo', e: 4, f: 5, g: 6, @@ -579,6 +582,7 @@ describe('ApolloClient', () => { assert.deepEqual(client.store.getState().apollo.data, { 'foo': { + __typename: 'Foo', e: 4, f: 5, g: 6, @@ -604,6 +608,7 @@ describe('ApolloClient', () => { assert.deepEqual(client.store.getState().apollo.data, { 'foo': { + __typename: 'Foo', e: 4, f: 5, g: 6, @@ -629,6 +634,7 @@ describe('ApolloClient', () => { assert.deepEqual(client.store.getState().apollo.data, { 'foo': { + __typename: 'Foo', e: 4, f: 5, g: 6, diff --git a/test/diffAgainstStore.ts b/test/diffAgainstStore.ts index e7de51ac88d..b362e035b54 100644 --- a/test/diffAgainstStore.ts +++ b/test/diffAgainstStore.ts @@ -8,6 +8,9 @@ import { import { writeQueryToStore } from '../src/data/writeToStore'; import gql from 'graphql-tag'; +import { + withError, +} from './util/wrap'; import { HeuristicFragmentMatcher, @@ -87,76 +90,82 @@ describe('diffing queries against the store', () => { assert.deepEqual(store['1'], result.people_one); }); - it('does not swallow errors other than field errors', () => { - const firstQuery = gql` - query { - person { - powers - } - }`; - const firstResult = { - person: { - powers: 'the force', - }, - }; - const store = writeQueryToStore({ - result: firstResult, - query: firstQuery, - }); - const unionQuery = gql` - query { - ...notARealFragment - }`; - assert.throws(() => { - diffQueryAgainstStore({ - store, - query: unionQuery, + it('does not swallow errors other than field errors', (done) => { + withError( () => { + const firstQuery = gql` + query { + person { + powers + } + }`; + const firstResult = { + person: { + powers: 'the force', + }, + }; + const store = writeQueryToStore({ + result: firstResult, + query: firstQuery, }); - }, /No fragment/); + const unionQuery = gql` + query { + ...notARealFragment + }`; + assert.throws(() => { + diffQueryAgainstStore({ + store, + query: unionQuery, + }); + }, /No fragment/); + done(); + }, /IntrospectionFragmentMatcher/); }); - it('does not error on a correct query with union typed fragments', () => { - const firstQuery = gql` - query { - person { - __typename - firstName - lastName - } - }`; - const firstResult = { - person: { - __typename: 'Author', - firstName: 'John', - lastName: 'Smith', - }, - }; - const store = writeQueryToStore({ - result: firstResult, - query: firstQuery, - }); - const unionQuery = gql` - query { - person { - __typename - ... on Author { + it('does not error on a correct query with union typed fragments', (done) => { + withError(() => { + const firstQuery = gql` + query { + person { + __typename firstName lastName } - - ... on Jedi { - powers + }`; + const firstResult = { + person: { + __typename: 'Author', + firstName: 'John', + lastName: 'Smith', + }, + }; + const store = writeQueryToStore({ + result: firstResult, + query: firstQuery, + }); + const unionQuery = gql` + query { + person { + __typename + ... on Author { + firstName + lastName + } + + ... on Jedi { + powers + } } - } - }`; - const { isMissing } = diffQueryAgainstStore({ - store, - query: unionQuery, - returnPartialData: false, - fragmentMatcherFunction, - }); + }`; + const { isMissing } = diffQueryAgainstStore({ + store, + query: unionQuery, + returnPartialData: false, + fragmentMatcherFunction, + }); - assert.isTrue(isMissing); + assert.isTrue(isMissing); + done(); + }, /IntrospectionFragmentMatcher/); }); it('does not error on a query with fields missing from all but one named fragment', () => { diff --git a/test/proxy.ts b/test/proxy.ts index 4540ea045c3..8329ebb9d03 100644 --- a/test/proxy.ts +++ b/test/proxy.ts @@ -203,7 +203,7 @@ describe('ReduxDataProxy', () => { }, }, 'foo': { - __typename: 'Type2', + __typename: 'Foo', e: 4, f: 5, g: 6, @@ -214,7 +214,7 @@ describe('ReduxDataProxy', () => { }, }, 'bar': { - __typename: 'Type3', + __typename: 'Bar', i: 7, j: 8, k: 9, @@ -264,7 +264,7 @@ describe('ReduxDataProxy', () => { apollo: { data: { 'foo': { - __typename: 'Type1', + __typename: 'Foo', 'field({"literal":true,"value":42})': 1, 'field({"literal":false,"value":42})': 2, }, @@ -294,7 +294,7 @@ describe('ReduxDataProxy', () => { initialState: { apollo: { data: { - 'bar': { __typename: 'Type1', a: 1, b: 2, c: 3 }, + 'bar': { __typename: 'Bar', a: 1, b: 2, c: 3 }, }, }, }, @@ -303,7 +303,7 @@ describe('ReduxDataProxy', () => { initialState: { apollo: { data: { - 'foo': { __typename: 'Type1', a: 1, b: 2, c: 3 }, + 'foo': { __typename: 'Foo', a: 1, b: 2, c: 3 }, }, }, }, diff --git a/test/readFromStore.ts b/test/readFromStore.ts index f53af31e960..13fc1db75fb 100644 --- a/test/readFromStore.ts +++ b/test/readFromStore.ts @@ -5,6 +5,10 @@ import { readQueryFromStore, } from '../src/data/readFromStore'; +import { + withError, +} from './util/wrap'; + import { NormalizedCache, StoreObject, @@ -295,51 +299,55 @@ describe('reading from the store', () => { }); }); - it('runs a nested query with proper fragment fields in arrays', () => { - const store = { - 'ROOT_QUERY': { - __typename: 'Query', - nestedObj: { type: 'id', id: 'abcde', generated: false }, - } as StoreObject, - abcde: { - id: 'abcde', - innerArray: [{ type: 'id', generated: true, id: 'abcde.innerArray.0' } as any], - } as StoreObject, - 'abcde.innerArray.0': { - id: 'abcdef', - someField: 3, - } as StoreObject, - } as NormalizedCache; + it('runs a nested query with proper fragment fields in arrays', (done) => { + withError(() => { + const store = { + 'ROOT_QUERY': { + __typename: 'Query', + nestedObj: { type: 'id', id: 'abcde', generated: false }, + } as StoreObject, + abcde: { + id: 'abcde', + innerArray: [{ type: 'id', generated: true, id: 'abcde.innerArray.0' } as any], + } as StoreObject, + 'abcde.innerArray.0': { + id: 'abcdef', + someField: 3, + } as StoreObject, + } as NormalizedCache; - const queryResult = readQueryFromStore({ - store, - query: gql` - { - ... on DummyQuery { - nestedObj { - innerArray { id otherField } + const queryResult = readQueryFromStore({ + store, + query: gql` + { + ... on DummyQuery { + nestedObj { + innerArray { id otherField } + } } - } - ... on Query { - nestedObj { - innerArray { id someField } + ... on Query { + nestedObj { + innerArray { id someField } + } } - } - ... on DummyQuery2 { - nestedObj { - innerArray { id otherField2 } + ... on DummyQuery2 { + nestedObj { + innerArray { id otherField2 } + } } } - } - `, - fragmentMatcherFunction, - }); + `, + fragmentMatcherFunction, + }); + + assert.deepEqual(queryResult, { + nestedObj: { + innerArray: [{id: 'abcdef', someField: 3}], + }, + }); + done(); + }, /IntrospectionFragmentMatcher/); - assert.deepEqual(queryResult, { - nestedObj: { - innerArray: [{id: 'abcdef', someField: 3}], - }, - }); }); it('runs a nested query with an array without IDs', () => { diff --git a/test/roundtrip.ts b/test/roundtrip.ts index 657818da424..06654767d43 100644 --- a/test/roundtrip.ts +++ b/test/roundtrip.ts @@ -13,7 +13,7 @@ import { import gql from 'graphql-tag'; -import { withWarning } from './util/wrap'; +import { withWarning, withError } from './util/wrap'; import { HeuristicFragmentMatcher, @@ -218,121 +218,134 @@ describe('roundtrip', () => { }); // XXX this test is weird because it assumes the server returned an incorrect result - it('should throw an error on two of the same inline fragment types', () => { - assert.throws(() => { + // However, the user may have written this result with client.writeQuery. + it('should throw an error on two of the same inline fragment types', (done) => { + withError(() => { + assert.throws(() => { + storeRoundtrip(gql` + query { + all_people { + __typename + name + ... on Jedi { + side + } + ... on Jedi { + rank + } + } + }`, { + all_people: [ + { + __typename: 'Jedi', + name: 'Luke Skywalker', + side: 'bright', + }, + ], + }); + }, /Can\'t find field rank on object/); + done(); + }, /IntrospectionFragmentMatcher/); + }); + + it('should resolve fields it can on interface with non matching inline fragments', (done) => { + withError(() => { storeRoundtrip(gql` query { - all_people { + dark_forces { __typename name - ... on Jedi { - side - } - ... on Jedi { - rank + ... on Droid { + model } } }`, { + dark_forces: [ + { + __typename: 'Droid', + name: '8t88', + model: '88', + }, + { + __typename: 'Darth', + name: 'Anakin Skywalker', + }, + ], + }); + done(); + }, /IntrospectionFragmentMatcher/); + }); + + it('should resolve on union types with spread fragments', (done) => { + withError(() => { + storeRoundtrip(gql` + fragment jediFragment on Jedi { + side + } + + fragment droidFragment on Droid { + model + } + + query { + all_people { + __typename + name + ...jediFragment + ...droidFragment + } + }`, { all_people: [ { __typename: 'Jedi', name: 'Luke Skywalker', side: 'bright', }, + { + __typename: 'Droid', + name: 'R2D2', + model: 'astromech', + }, ], - }); - }, /Can\'t find field rank on object/); + }); + done(); + }, /IntrospectionFragmentMatcher/); }); - it('should resolve fields it can on interface with non matching inline fragments', () => { - storeRoundtrip(gql` - query { - dark_forces { - __typename - name - ... on Droid { - model - } + it('should work with a fragment on the actual interface or union', (done) => { + withError(() => { + storeRoundtrip(gql` + fragment jediFragment on Character { + side } - }`, { - dark_forces: [ - { - __typename: 'Droid', - name: '8t88', - model: '88', - }, - { - __typename: 'Darth', - name: 'Anakin Skywalker', - }, - ], - }); - }); - - it('should resolve on union types with spread fragments', () => { - storeRoundtrip(gql` - fragment jediFragment on Jedi { - side - } - - fragment droidFragment on Droid { - model - } - query { - all_people { - __typename - name - ...jediFragment - ...droidFragment + fragment droidFragment on Droid { + model } - }`, { - all_people: [ - { - __typename: 'Jedi', - name: 'Luke Skywalker', - side: 'bright', - }, - { - __typename: 'Droid', - name: 'R2D2', - model: 'astromech', - }, - ], - }); - }); - - it('should work with a fragment on the actual interface or union', () => { - storeRoundtrip(gql` - fragment jediFragment on Character { - side - } - - fragment droidFragment on Droid { - model - } - query { - all_people { - name - __typename - ...jediFragment - ...droidFragment - } - }`, { - all_people: [ - { - __typename: 'Jedi', - name: 'Luke Skywalker', - side: 'bright', - }, - { - __typename: 'Droid', - name: 'R2D2', - model: 'astromech', - }, - ], - }); + query { + all_people { + name + __typename + ...jediFragment + ...droidFragment + } + }`, { + all_people: [ + { + __typename: 'Jedi', + name: 'Luke Skywalker', + side: 'bright', + }, + { + __typename: 'Droid', + name: 'R2D2', + model: 'astromech', + }, + ], + }); + done(); + }, /IntrospectionFragmentMatcher/); }); it('should throw on error on two of the same spread fragment types', () => { diff --git a/test/tests.ts b/test/tests.ts index 49bdb5e5892..425d81f741d 100644 --- a/test/tests.ts +++ b/test/tests.ts @@ -25,6 +25,7 @@ console.warn = console.error = (...messages: string[]) => { process.on('unhandledRejection', () => {}); +import './warnOnce'; import './fragmentMatcher'; import './writeToStore'; import './readFromStore'; diff --git a/test/util/wrap.ts b/test/util/wrap.ts index 3acfefdbb57..91317323be5 100644 --- a/test/util/wrap.ts +++ b/test/util/wrap.ts @@ -26,3 +26,19 @@ export function withWarning(func: Function, regex: RegExp) { console.warn = oldWarn; } } + +export function withError(func: Function, regex: RegExp) { + let message: string = null as never; + const oldError = console.error; + + console.error = (m: string) => message = m; + + try { + const result = func(); + assert.match(message, regex); + return result; + + } finally { + console.error = oldError; + } +} diff --git a/test/warnOnce.ts b/test/warnOnce.ts new file mode 100644 index 00000000000..7140f88bb38 --- /dev/null +++ b/test/warnOnce.ts @@ -0,0 +1,38 @@ +import { assert, expect } from 'chai'; +import { warnOnce } from '../src/util/warnOnce'; + +let lastWarning: string | null; +let numCalls = 0; +let oldConsoleWarn: any; + +describe('warnOnce', () => { + beforeEach( () => { + numCalls = 0; + lastWarning = null; + oldConsoleWarn = console.warn; + console.warn = (msg: any) => { numCalls++; lastWarning = msg; }; + }); + afterEach( () => { + console.warn = oldConsoleWarn; + }); + it('actually warns', () => { + warnOnce('hi'); + assert(lastWarning === 'hi'); + expect(numCalls).to.equal(1); + }); + + it('does not warn twice', () => { + warnOnce('ho'); + warnOnce('ho'); + expect(lastWarning).to.equal('ho'); + expect(numCalls).to.equal(1); + }); + + it('warns two different things once', () => { + warnOnce('slow'); + expect(lastWarning).to.equal('slow'); + warnOnce('mo'); + expect(lastWarning).to.equal('mo'); + expect(numCalls).to.equal(2); + }); +});