WIP: Fix reject on refetch

CHANGELOG.md

@@ -17,6 +17,7 @@ Expect active development and potentially significant breaking changes in the `0
 - **Refactor**: removed circular dependency in data/store.ts [Issue #731](https://github.com/apollostack/apollo-client/issues/731) [PR #778](https://github.com/apollostack/apollo-client/pull/778)
 - added "ApolloClient" to the named exports to make it compatible with Angular2 AOT compile [Issue #758](https://github.com/apollostack/apollo-client/issues/758) [PR #778](https://github.com/apollostack/apollo-client/pull/778)
 - Fix: moved dev @types to devDependencies otherwise they potentially brake projects that are importing apollo-client [Issue #713](https://github.com/apollostack/apollo-client/issues/713) [PR #778](https://github.com/apollostack/apollo-client/pull/778)
+- Fix rejecting promises on `refetch` and similar methods. Also improve error handling and stop using `ApolloError` internally. [Failing test in PR #524](https://github.com/apollostack/apollo-client/pull/524) [PR #781](https://github.com/apollostack/apollo-client/pull/781)
 
 ### v0.4.20
 - Fix: Warn but do not fail when refetchQueries includes an unknown query name [PR #700](https://github.com/apollostack/apollo-client/pull/700)

src/core/QueryManager.ts

@@ -378,7 +378,9 @@ export class QueryManager {
             }
           } catch (error) {
             if (observer.error) {
-              observer.error(error);
+              observer.error(new ApolloError({
+                networkError: error,
+              }));
             }
           }
         }
@@ -432,7 +434,82 @@ export class QueryManager {
   }
 
   public fetchQuery(queryId: string, options: WatchQueryOptions): Promise<ApolloQueryResult> {
-    return this.fetchQueryOverInterface(queryId, options);
+    const {
+      variables,
+      forceFetch = false,
+      returnPartialData = false,
+      noFetch = false,
+    } = options;
+
+    const {
+      queryDoc,
+    } = this.transformQueryDocument(options);
+
+    const queryString = print(queryDoc);
+
+    let storeResult: any;
+    let needToFetch: boolean = forceFetch;
+
+    // If this is not a force fetch, we want to diff the query against the
+    // store before we fetch it from the network interface.
+    if (!forceFetch) {
+      const { isMissing, result } = diffQueryAgainstStore({
+        query: queryDoc,
+        store: this.reduxRootSelector(this.store.getState()).data,
+        returnPartialData: true,
+        variables,
+      });
+
+      // If we're in here, only fetch if we have missing fields
+      needToFetch = isMissing;
+
+      storeResult = result;
+    }
+
+    const requestId = this.generateRequestId();
+    const shouldFetch = needToFetch && !noFetch;
+
+    // Initialize query in store with unique requestId
+    this.queryDocuments[queryId] = queryDoc;
+    this.store.dispatch({
+      type: 'APOLLO_QUERY_INIT',
+      queryString,
+      document: queryDoc,
+      variables,
+      forceFetch,
+      returnPartialData: returnPartialData || noFetch,
+      queryId,
+      requestId,
+      // we store the old variables in order to trigger "loading new variables"
+      // state if we know we will go to the server
+      storePreviousVariables: shouldFetch,
+    });
+
+    // If there is no part of the query we need to fetch from the server (or,
+    // noFetch is turned on), we just write the store result as the final result.
+    if (!shouldFetch || returnPartialData) {
+      this.store.dispatch({
+        type: 'APOLLO_QUERY_RESULT_CLIENT',
+        result: { data: storeResult },
+        variables,
+        document: queryDoc,
+        complete: !shouldFetch,
+        queryId,
+      });
+    }
+
+    if (shouldFetch) {
+      return this.fetchRequest({
+        requestId,
+        queryId,
+        document: queryDoc,
+        options,
+      });
+    }
+
+    // If we have no query to send to the server, we should return the result
+    // found within the store.
+    return Promise.resolve({ data: storeResult });
   }
 
   public generateQueryId() {
@@ -825,7 +902,7 @@ export class QueryManager {
     const retPromise = new Promise<ApolloQueryResult>((resolve, reject) => {
       this.addFetchQueryPromise(requestId, retPromise, resolve, reject);
 
-      return this.networkInterface.query(request)
+      this.networkInterface.query(request)
         .then((result: GraphQLResult) => {
 
           const extraReducers = this.getExtraReducers();
@@ -842,6 +919,14 @@ export class QueryManager {
           });
 
           this.removeFetchQueryPromise(requestId);
+
+          // XXX this duplicates some logic in the store about identifying errors
+          if (result.errors) {
+            throw new ApolloError({
+              graphQLErrors: result.errors,
+            });
+          }
+
           return result;
         }).then(() => {
 
@@ -865,99 +950,28 @@ export class QueryManager {
           this.removeFetchQueryPromise(requestId);
           resolve({ data: resultFromStore, loading: false });
         }).catch((error: Error) => {
-          this.store.dispatch({
-            type: 'APOLLO_QUERY_ERROR',
-            error,
-            queryId,
-            requestId,
-          });
-
-          this.removeFetchQueryPromise(requestId);
-        });
-    });
-    return retPromise;
-  }
-
-  private fetchQueryOverInterface(
-    queryId: string,
-    options: WatchQueryOptions
-  ): Promise<ApolloQueryResult> {
-    const {
-      variables,
-      forceFetch = false,
-      returnPartialData = false,
-      noFetch = false,
-    } = options;
-
-    const {
-      queryDoc,
-    } = this.transformQueryDocument(options);
-
-    const queryString = print(queryDoc);
-
-    let storeResult: any;
-    let needToFetch: boolean = forceFetch;
-
-    // If this is not a force fetch, we want to diff the query against the
-    // store before we fetch it from the network interface.
-    if (!forceFetch) {
-      const { isMissing, result } = diffQueryAgainstStore({
-        query: queryDoc,
-        store: this.reduxRootSelector(this.store.getState()).data,
-        returnPartialData: true,
-        variables,
-      });
-
-      // If we're in here, only fetch if we have missing fields
-      needToFetch = isMissing;
-
-      storeResult = result;
-    }
+          // This is for the benefit of `refetch` promises, which currently don't get their errors
+          // through the store like watchQuery observers do
+          if (error instanceof ApolloError) {
+            reject(error);
+          } else {
+            this.store.dispatch({
+              type: 'APOLLO_QUERY_ERROR',
+              error,
+              queryId,
+              requestId,
+            });
 
-    const requestId = this.generateRequestId();
-    const shouldFetch = needToFetch && !noFetch;
+            this.removeFetchQueryPromise(requestId);
 
-    // Initialize query in store with unique requestId
-    this.queryDocuments[queryId] = queryDoc;
-    this.store.dispatch({
-      type: 'APOLLO_QUERY_INIT',
-      queryString,
-      document: queryDoc,
-      variables,
-      forceFetch,
-      returnPartialData: returnPartialData || noFetch,
-      queryId,
-      requestId,
-      // we store the old variables in order to trigger "loading new variables"
-      // state if we know we will go to the server
-      storePreviousVariables: shouldFetch,
+            reject(new ApolloError({
+              networkError: error,
+            }));
+          }
+        });
     });
 
-    // If there is no part of the query we need to fetch from the server (or,
-    // noFetch is turned on), we just write the store result as the final result.
-    if (!shouldFetch || returnPartialData) {
-      this.store.dispatch({
-        type: 'APOLLO_QUERY_RESULT_CLIENT',
-        result: { data: storeResult },
-        variables,
-        document: queryDoc,
-        complete: !shouldFetch,
-        queryId,
-      });
-    }
-
-    if (shouldFetch) {
-      return this.fetchRequest({
-        requestId,
-        queryId,
-        document: queryDoc,
-        options,
-      });
-    }
-
-    // If we have no query to send to the server, we should return the result
-    // found within the store.
-    return Promise.resolve({ data: storeResult });
+    return retPromise;
   }
 
   // Refetches a query given that query's name. Refetches

src/data/readFromStore.ts

@@ -17,10 +17,6 @@ import {
   storeKeyNameFromFieldNameAndArgs,
 } from './storeUtils';
 
-import {
-  ApolloError,
-} from '../errors/ApolloError';
-
 export type DiffResult = {
   result?: any;
   isMissing?: boolean;
@@ -88,6 +84,8 @@ const fragmentMatcher: FragmentMatcher = (
       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.`);
+
+      /* istanbul ignore if */
       if (process.env.NODE_ENV !== 'test') {
         // When running tests, we want to print the warning every time
         haveWarned = true;
@@ -123,13 +121,8 @@ const readStoreResolver: Resolver = (
 
   if (typeof fieldValue === 'undefined') {
     if (! context.returnPartialData) {
-      throw new ApolloError({
-        errorMessage: `Can't find field ${storeKeyName} on object (${objId}) ${JSON.stringify(obj, null, 2)}.
-Perhaps you want to use the \`returnPartialData\` option?`,
-        extraInfo: {
-          isFieldError: true,
-        },
-      });
+      throw new Error(`Can't find field ${storeKeyName} on object (${objId}) ${JSON.stringify(obj, null, 2)}.
+Perhaps you want to use the \`returnPartialData\` option?`);
     }
 
     context.hasMissingField = true;

src/data/writeToStore.ts

@@ -42,10 +42,6 @@ import {
   shouldInclude,
 } from '../queries/directives';
 
-import {
-  ApolloError,
-} from '../errors/ApolloError';
-
 /**
  * Writes the result of a query to the store.
  *
@@ -351,10 +347,8 @@ function writeFieldToStore({
       // are dealing with is generated, we throw an error.
       if (isIdValue(storeValue) && storeValue.generated
           && isIdValue(escapedId) && !escapedId.generated) {
-        throw new ApolloError({
-          errorMessage: `Store error: the application attempted to write an object with no provided id` +
-            ` but the store already contains an id of ${escapedId.id} for this object.`,
-        });
+        throw new Error(`Store error: the application attempted to write an object with no provided id` +
+            ` but the store already contains an id of ${escapedId.id} for this object.`);
       }
 
       if (isIdValue(escapedId) && escapedId.generated) {

test/QueryManager.ts

@@ -1580,15 +1580,20 @@ describe('QueryManager', () => {
       pollInterval: 50,
     });
 
-    return observableToPromise({
+    const { promise, subscription } = observableToPromiseAndSubscription({
         observable,
         wait: 60,
         errorCallbacks: [
-          (error) => assert.include(error.message, 'Network error'),
+          (error) => {
+            assert.include(error.message, 'Network error');
+            subscription.unsubscribe();
+          },
         ],
       },
       (result) => assert.deepEqual(result.data, data1)
     );
+
+    return promise;
   });
 
   it('exposes a way to start a polling query', () => {
@@ -2543,13 +2548,18 @@ describe('QueryManager', () => {
       observableToPromise({
           observable: observable1,
           errorCallbacks: [
-            (error) => assert.deepEqual((error as any).extraInfo, { isFieldError: true }),
+            // This isn't the best error message, but at least people will know they are missing
+            // data in the store.
+            (error: ApolloError) => assert.include(error.networkError.message, 'find field'),
           ],
           wait: 60,
         },
         (result) => assert.deepEqual(result.data, data1)
       ),
-      observableToPromise({ observable: observable2, wait: 60 },
+      observableToPromise({
+          observable: observable2,
+          wait: 60,
+        },
         (result) => assert.deepEqual(result.data, data2)
       ),
     ]);
@@ -3128,4 +3138,50 @@ describe('QueryManager', () => {
 
   });
 
+  it('exposes errors on a refetch as a rejection', (done) => {
+    const request = {
+      query: gql`
+      {
+        people_one(id: 1) {
+          name
+        }
+      }`,
+    };
+    const firstResult = {
+      data: {
+        people_one: {
+          name: 'Luke Skywalker',
+        },
+      },
+    };
+    const secondResult = {
+      errors: [
+        {
+          name: 'PeopleError',
+          message: 'This is not the person you are looking for.',
+        },
+      ],
+    };
+
+    const queryManager = mockRefetch({ request, firstResult, secondResult });
+
+    const handle = queryManager.watchQuery(request);
+
+    handle.subscribe({
+      error: () => { /* nothing */ },
+    });
+
+    handle.refetch().catch((error) => {
+      assert.deepEqual(error.graphQLErrors, [
+        {
+          name: 'PeopleError',
+          message: 'This is not the person you are looking for.',
+        },
+      ]);
+      done();
+    });
+
+    // We have an unhandled error warning from the `subscribe` above, which has no `error` cb
+  });
+
 });