Merge pull request #1635 from apollographql/fm-usability

FragmentMatcher warning

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)

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

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

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

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,

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

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 },
             },
           },
         },