Merge pull request #352 from apollostack/unify_errors

[Breaking change] Unify error handling

CHANGELOG.md

@@ -5,6 +5,7 @@ Expect active development and potentially significant breaking changes in the `0
 ### vNEXT
 
 - **Breaking change** Moved refetch(), startPolling(), and stopPolling() methods from QuerySubscription to ObservableQuery. This shouldn't affect anyone using `react-apollo`, but if you were calling those methods on the subscription directly, you need to call them on the query handle/observable instead. The benefit of this is that developers that want to use RxJS for their observable handling can now have access to these methods. [Issue #194] (https://github.com/apollostack/apollo-client/issues/194) and [PR #362] (https://github.com/apollostack/apollo-client/pull/362)
+- **Breaking change** Unified error handling for GraphQL errors and network errors. Both now result in rejected promises and passed as errors on observables through a new `ApolloError` type. This is a significant departure from the previous method of error handling which passed GraphQL errors in resolvers and `next` methods on subscriptions. [PR #352](https://github.com/apollostack/apollo-client/pull/352)
 
 ### v0.3.30
 

src/QueryManager.ts

@@ -3,7 +3,6 @@ import {
   Request,
 } from './networkInterface';
 
-
 import forOwn = require('lodash.forown');
 import assign = require('lodash.assign');
 import isEqual = require('lodash.isequal');
@@ -66,16 +65,24 @@ import {
   QueryScheduler,
 } from './scheduler';
 
+import {
+  ApolloQueryResult,
+} from './index';
+
 import { Observable, Observer, Subscription, SubscriberFunction } from './util/Observable';
 
-export class ObservableQuery extends Observable<GraphQLResult> {
-  public refetch: (variables?: any) => Promise<GraphQLResult>;
+import {
+  ApolloError,
+} from './errors';
+
+export class ObservableQuery extends Observable<ApolloQueryResult> {
+  public refetch: (variables?: any) => Promise<ApolloQueryResult>;
   public stopPolling: () => void;
   public startPolling: (p: number) => void;
 
   constructor(options: {
-    subscriberFunction: SubscriberFunction<GraphQLResult>,
-    refetch: (variables?: any) => Promise<GraphQLResult>,
+    subscriberFunction: SubscriberFunction<ApolloQueryResult>,
+    refetch: (variables?: any) => Promise<ApolloQueryResult>,
     stopPolling: () => void,
     startPolling: (p: number) => void
   }) {
@@ -85,11 +92,11 @@ export class ObservableQuery extends Observable<GraphQLResult> {
     this.startPolling = options.startPolling;
   }
 
-  public subscribe(observer: Observer<GraphQLResult>): Subscription {
+  public subscribe(observer: Observer<ApolloQueryResult>): Subscription {
     return super.subscribe(observer);
   }
 
-  public result(): Promise<GraphQLResult> {
+  public result(): Promise<ApolloQueryResult> {
     return new Promise((resolve, reject) => {
       const subscription = this.subscribe({
         next(result) {
@@ -136,8 +143,8 @@ export class QueryManager {
   // track of queries that are inflight and reject them in case some
   // destabalizing action occurs (e.g. reset of the Apollo store).
   private fetchQueryPromises: { [requestId: string]: {
-    promise: Promise<GraphQLResult>;
-    resolve: (result: GraphQLResult) => void;
+    promise: Promise<ApolloQueryResult>;
+    resolve: (result: ApolloQueryResult) => void;
     reject: (error: Error) => void;
   } };
 
@@ -216,7 +223,7 @@ export class QueryManager {
     variables?: Object,
     resultBehaviors?: MutationBehavior[],
     fragments?: FragmentDefinition[],
-  }): Promise<GraphQLResult> {
+  }): Promise<ApolloQueryResult> {
     const mutationId = this.generateQueryId();
 
     let mutationDef = getMutationDefinition(mutation);
@@ -269,24 +276,30 @@ export class QueryManager {
   // results (or lack thereof) for a particular query.
   public queryListenerForObserver(
     options: WatchQueryOptions,
-    observer: Observer<GraphQLResult>
+    observer: Observer<ApolloQueryResult>
   ): QueryListener {
     return (queryStoreValue: QueryStoreValue) => {
+      // The query store value can be undefined in the event of a store
+      // reset.
+      if (!queryStoreValue) {
+        return;
+      }
+
       if (!queryStoreValue.loading || queryStoreValue.returnPartialData) {
         // XXX Currently, returning errors and data is exclusive because we
         // don't handle partial results
-        if (queryStoreValue.graphQLErrors) {
-          if (observer.next) {
-            observer.next({ errors: queryStoreValue.graphQLErrors });
-          }
-        } else if (queryStoreValue.networkError) {
-          // XXX we might not want to re-broadcast the same error over and over if it didn't change
+
+        // If we have either a GraphQL error or a network error, we create
+        // an error and tell the observer about it.
+        if (queryStoreValue.graphQLErrors || queryStoreValue.networkError) {
+          const apolloError = new ApolloError({
+            graphQLErrors: queryStoreValue.graphQLErrors,
+            networkError: queryStoreValue.networkError,
+          });
           if (observer.error) {
-            observer.error(queryStoreValue.networkError);
+            observer.error(apolloError);
           } else {
-            console.error('Unhandled network error',
-                          queryStoreValue.networkError,
-                          queryStoreValue.networkError.stack);
+            console.error('Unhandled error', apolloError, apolloError.stack);
           }
         } else {
           const resultFromStore = readSelectionSetFromStore({
@@ -319,11 +332,12 @@ export class QueryManager {
   public watchQuery(options: WatchQueryOptions, shouldSubscribe = true): ObservableQuery {
     // Call just to get errors synchronously
     getQueryDefinition(options.query);
+
     const queryId = this.generateQueryId();
 
     let observableQuery;
 
-    const subscriberFunction = (observer) => {
+    const subscriberFunction = (observer: Observer<ApolloQueryResult>) => {
       const retQuerySubscription = {
         unsubscribe: () => {
           this.stopQuery(queryId);
@@ -335,46 +349,12 @@ export class QueryManager {
         this.addQuerySubscription(queryId, retQuerySubscription);
       }
 
-      this.startQuery(queryId, options, (queryStoreValue: QueryStoreValue) => {
-        // we could get back an empty store value if the store was reset while this
-        // query was still in flight. In this circumstance, we are no longer concerned
-        // with the return value of that particular instance of the query.
-        if (!queryStoreValue) {
-          return;
-        }
-
-        if (!queryStoreValue.loading || queryStoreValue.returnPartialData) {
-          // XXX Currently, returning errors and data is exclusive because we
-          // don't handle partial results
-          if (queryStoreValue.graphQLErrors) {
-            if (observer.next) {
-              observer.next({ errors: queryStoreValue.graphQLErrors });
-            }
-          } else if (queryStoreValue.networkError) {
-            // XXX we might not want to re-broadcast the same error over and over if it didn't change
-            if (observer.error) {
-              observer.error(queryStoreValue.networkError);
-            } else {
-              console.error('Unhandled network error',
-                queryStoreValue.networkError,
-                queryStoreValue.networkError.stack);
-            }
-          } else {
-            const resultFromStore = readSelectionSetFromStore({
-              store: this.getApolloState().data,
-              rootId: queryStoreValue.query.id,
-              selectionSet: queryStoreValue.query.selectionSet,
-              variables: queryStoreValue.variables,
-              returnPartialData: options.returnPartialData,
-              fragmentMap: queryStoreValue.fragmentMap,
-            });
+      this.startQuery(
+        queryId,
+        options,
+        this.queryListenerForObserver(options, observer)
+      );
 
-            if (observer.next) {
-              observer.next({ data: resultFromStore });
-            }
-          }
-        }
-      });
       return retQuerySubscription;
     };
 
@@ -414,7 +394,7 @@ export class QueryManager {
     return observableQuery;
   }
 
-  public query(options: WatchQueryOptions): Promise<GraphQLResult> {
+  public query(options: WatchQueryOptions): Promise<ApolloQueryResult> {
     if (options.returnPartialData) {
       throw new Error('returnPartialData option only supported on watchQuery.');
     }
@@ -439,7 +419,7 @@ export class QueryManager {
     return resPromise;
   }
 
-  public fetchQuery(queryId: string, options: WatchQueryOptions): Promise<GraphQLResult> {
+  public fetchQuery(queryId: string, options: WatchQueryOptions): Promise<ApolloQueryResult> {
     return this.fetchQueryOverInterface(queryId, options, this.networkInterface);
   }
 
@@ -469,12 +449,13 @@ export class QueryManager {
   }
 
   // Adds a promise to this.fetchQueryPromises for a given request ID.
-  public addFetchQueryPromise(requestId: number, promise: Promise<GraphQLResult>,
-    resolve: (result: GraphQLResult) => void,
+  public addFetchQueryPromise(requestId: number, promise: Promise<ApolloQueryResult>,
+    resolve: (result: ApolloQueryResult) => void,
     reject: (error: Error) => void) {
     this.fetchQueryPromises[requestId.toString()] = { promise, resolve, reject };
   }
 
+
   // Removes the promise in this.fetchQueryPromises for a particular request ID.
   public removeFetchQueryPromise(requestId: number) {
     delete this.fetchQueryPromises[requestId.toString()];
@@ -533,7 +514,7 @@ export class QueryManager {
     queryId: string,
     options: WatchQueryOptions,
     network: NetworkInterface
-  ): Promise<GraphQLResult> {
+  ): Promise<ApolloQueryResult> {
     const {
       query,
       variables,
@@ -652,12 +633,12 @@ export class QueryManager {
         operationName: request.operationName,
       };
 
-      const retPromise = new Promise<GraphQLResult>((resolve, reject) => {
+      const retPromise = new Promise<ApolloQueryResult>((resolve, reject) => {
         this.addFetchQueryPromise(requestId, retPromise, resolve, reject);
 
         return this.batcher.enqueueRequest(fetchRequest)
           .then((result: GraphQLResult) => {
-            // XXX handle multiple GraphQLResults
+            // XXX handle multiple ApolloQueryResults
             this.store.dispatch({
               type: 'APOLLO_QUERY_RESULT',
               result,
@@ -666,6 +647,11 @@ export class QueryManager {
             });
 
             this.removeFetchQueryPromise(requestId);
+            if (result.errors) {
+              reject(new ApolloError({
+                graphQLErrors: result.errors,
+              }));
+            }
             return result;
           }).then(() => {
 
@@ -699,7 +685,9 @@ export class QueryManager {
             });
 
             this.removeFetchQueryPromise(requestId);
-            return error;
+            reject(new ApolloError({
+              networkError: error,
+            }));
           });
       });
       return retPromise;

src/errors.ts

@@ -0,0 +1,57 @@
+import { GraphQLError } from 'graphql';
+
+export class ApolloError extends Error {
+  public message: string;
+  public graphQLErrors: GraphQLError[];
+  public networkError: Error;
+
+  // Constructs an instance of ApolloError given a GraphQLError
+  // or a network error. Note that one of these has to be a valid
+  // value or the constructed error will be meaningless.
+  constructor({
+    graphQLErrors,
+    networkError,
+    errorMessage,
+  }: {
+    graphQLErrors?: GraphQLError[],
+    networkError?: Error,
+    errorMessage?: string,
+  }) {
+    super(errorMessage);
+    this.graphQLErrors = graphQLErrors;
+    this.networkError = networkError;
+
+    if (!errorMessage) {
+      this.generateErrorMessage();
+    } else {
+      this.message = errorMessage;
+    }
+  }
+
+  // Sets the error message on this error according to the
+  // the GraphQL and network errors that are present.
+  // If the error message has already been set through the
+  // constructor or otherwise, this function is a nop.
+  private generateErrorMessage() {
+    if (typeof this.message !== 'undefined' &&
+       this.message !== '') {
+      return;
+    }
+
+    let message = '';
+    // If we have GraphQL errors present, add that to the error message.
+    if (Array.isArray(this.graphQLErrors) && this.graphQLErrors.length !== 0) {
+      this.graphQLErrors.forEach((graphQLError) => {
+        message += 'GraphQL error: ' + graphQLError.message + '\n';
+      });
+    }
+
+    if (this.networkError) {
+      message += 'Network error: ' + this.networkError.message + '\n';
+    }
+
+    // strip newline from the end of the message
+    message = message.replace(/\n$/, '');
+    this.message = message;
+  }
+}

src/index.ts

@@ -5,7 +5,6 @@ import {
 } from './networkInterface';
 
 import {
-  GraphQLResult,
   Document,
   FragmentDefinition,
 } from 'graphql';
@@ -77,6 +76,12 @@ export {
   print as printAST,
 };
 
+export type ApolloQueryResult = {
+  data: any;
+  // Right now only has one property, but will later include loading state, and possibly other info
+  // This is different from the GraphQLResult type because it doesn't include errors - those are
+  // thrown via the standard promise/observer catch mechanism
+}
 
 // A map going from the name of a fragment to that fragment's definition.
 // The point is to keep track of fragments that exist and print a warning if we encounter two
@@ -204,7 +209,7 @@ export default class ApolloClient {
     return this.queryManager.watchQuery(options);
   };
 
-  public query = (options: WatchQueryOptions): Promise<GraphQLResult> => {
+  public query = (options: WatchQueryOptions): Promise<ApolloQueryResult> => {
     this.initStore();
 
     if (!this.shouldForceFetch && options.forceFetch) {
@@ -226,7 +231,7 @@ export default class ApolloClient {
     resultBehaviors?: MutationBehavior[],
     variables?: Object,
     fragments?: FragmentDefinition[],
-  }): Promise<GraphQLResult> => {
+  }): Promise<ApolloQueryResult> => {
     this.initStore();
     return this.queryManager.mutate(options);
   };

src/scheduler.ts

@@ -96,6 +96,7 @@ export class QueryScheduler {
     if (!options.pollInterval) {
       throw new Error('Tried to register a non-polling query with the scheduler.');
     }
+
     const queryId = this.queryManager.generateQueryId();
 
     const subscriberFunction = (observer) => {