Fix issue where subscriptions had no error handling (#3800)

* Fix issue where subscriptions had no error handling:
   Explicitly handle the error case similar to how it is done for
   mutations.
* Slight code tweaks; Adjust test to verify multiple observers
* Adjust subscription error handling for backwards compatibility

CHANGELOG.md

@@ -19,6 +19,10 @@
 - Fix typo in `console.warn` regarding fragment matching error message.  <br/>
   [@combizs](https://github.com/combizs) in [#3701](https://github.com/apollographql/apollo-client/pull/3701)
 
+### Apollo Client (vNext)
+
+- Add proper error handling for subscriptions <br/>
+  [@clayne11](https://github.com/clayne11) in [#3800](https://github.com/apollographql/apollo-client/pull/3800)
 
 ## 2.3.8 (August 9, 2018)
 

packages/apollo-client/src/__tests__/__snapshots__/graphqlSubscriptions.ts.snap

@@ -0,0 +1,5 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP
+
+exports[`GraphQL Subscriptions should throw an error if the result has errors on it 1`] = `[Error: GraphQL error: This is an error]`;
+
+exports[`GraphQL Subscriptions should throw an error if the result has errors on it 2`] = `[Error: GraphQL error: This is an error]`;

packages/apollo-client/src/__tests__/graphqlSubscriptions.ts

@@ -209,4 +209,53 @@ describe('GraphQL Subscriptions', () => {
 
     link.simulateResult(results[0]);
   });
+
+  it('should throw an error if the result has errors on it', () => {
+    const link = mockObservableLink(sub1);
+    const queryManager = new QueryManager({
+      link,
+      store: new DataStore(new InMemoryCache({ addTypename: false })),
+    });
+
+    const obs = queryManager.startGraphQLSubscription(options);
+
+    const promises = [];
+    for (let i = 0; i < 2; i += 1) {
+      promises.push(
+        new Promise((resolve, reject) => {
+          obs.subscribe({
+            next(result) {
+              fail('Should have hit the error block');
+              reject();
+            },
+            error(error) {
+              expect(error).toMatchSnapshot();
+              resolve();
+            },
+          });
+        }),
+      );
+    }
+
+    const errorResult = {
+      result: {
+        data: null,
+        errors: [
+          {
+            message: 'This is an error',
+            locations: [
+              {
+                column: 3,
+                line: 2,
+              },
+            ],
+            path: ['result'],
+          },
+        ],
+      },
+    };
+
+    link.simulateResult(errorResult);
+    return Promise.all(promises);
+  });
 });

packages/apollo-client/src/core/QueryManager.ts

@@ -818,11 +818,11 @@ export class QueryManager<TStore> {
     const queryName = definition.name ? definition.name.value : null;
     this.setQuery(queryId, () => ({ observableQuery: null }));
     if (queryName) {
-      this.queryIdsByName[queryName] = this.queryIdsByName[
-        queryName
-      ].filter(val => {
-        return !(observableQuery.queryId === val);
-      });
+      this.queryIdsByName[queryName] = this.queryIdsByName[queryName].filter(
+        val => {
+          return !(observableQuery.queryId === val);
+        },
+      );
     }
   }
 
@@ -929,17 +929,29 @@ export class QueryManager<TStore> {
               this.broadcastQueries();
             }
 
-            // It's slightly awkward that the data for subscriptions doesn't
-            // come from the store.
             observers.forEach(obs => {
-              // XXX I'd prefer a different way to handle errors for
-              // subscriptions.
-              if (obs.next) obs.next(result);
+              // If an error exists and a `error` handler has been defined on
+              // the observer, call that `error` handler and make sure the
+              // `next` handler is skipped. If no `error` handler exists, we're
+              // still passing any errors that might occur into the `next`
+              // handler, to give that handler a chance to deal with the
+              // error (we're doing this for backwards compatibilty).
+              if (graphQLResultHasError(result) && obs.error) {
+                obs.error(
+                  new ApolloError({
+                    graphQLErrors: result.errors,
+                  }),
+                );
+              } else if (obs.next) {
+                obs.next(result);
+              }
             });
           },
           error: (error: Error) => {
             observers.forEach(obs => {
-              if (obs.error) obs.error(error);
+              if (obs.error) {
+                obs.error(error);
+              }
             });
           },
         };