improve fetch error handling

app/util/fetchUtils.js

@@ -1,3 +1,19 @@
+export class FetchError extends Error {}
+FetchError.prototype.name = 'FetchError';
+
+export const fetchWithErrors = async (url, options = {}) => {
+  const res = await fetch(url, options);
+  if (!res.ok) {
+    const error = new FetchError(`${res.url}: ${res.status} ${res.statusText}`);
+    error.reqUrl = url;
+    error.reqOptions = options;
+    error.res = res;
+    throw error;
+  }
+
+  return res;
+};
+
 const delay = ms =>
   new Promise(resolve => {
     setTimeout(() => {
@@ -34,41 +50,46 @@ const addLocaleParam = (url, lang) => {
 
 // Tries to fetch 1 + retryCount times until 200 is returned.
 // Uses retryDelay (ms) between requests. url and options are normal fetch parameters
-export const retryFetch = (
+export const retryFetch = async (
   URL,
-  options = {},
+  _options = {},
   retryCount,
   retryDelay,
   config = {},
 ) => {
-  return new Promise((resolve, reject) => {
-    const retry = retriesLeft => {
-      fetch(URL, {
-        ...options,
-        headers: addSubscriptionHeader(options.headers, config),
-      })
-        .then(res => {
-          if (res.ok) {
-            resolve(res);
-            // Don't retry if user is not logged in
-          } else if (res.status === 401) {
-            throw res.status;
-          } else {
-            // eslint-disable-next-line no-throw-literal
-            throw `${URL}: ${res.statusText}`;
-          }
-        })
-        .catch(async err => {
-          if (retriesLeft > 0 && err !== 401) {
-            await delay(retryDelay);
-            retry(retriesLeft - 1);
-          } else {
-            reject(err);
-          }
-        });
-    };
-    retry(retryCount);
-  });
+  const options = {
+    ..._options,
+    headers: addSubscriptionHeader(_options.headers, config),
+  };
+
+  let retriesLeft = retryCount;
+  while (retriesLeft >= 0) {
+    try {
+      // eslint-disable-next-line no-await-in-loop
+      return await fetchWithErrors(URL, options);
+    } catch (error) {
+      if (!(error instanceof FetchError)) {
+        // Throwing unrelated errors (e.g. TypeError) allows us to catch bugs.
+        throw error;
+      }
+
+      if (error.res.status === 401) {
+        // todo: throw `error` instead of a literal (breaking change)
+        // eslint-disable-next-line no-throw-literal
+        throw 401;
+      }
+      if (retriesLeft === 0) {
+        // todo: throw `error` instead of a literal (breaking change)
+        // eslint-disable-next-line no-throw-literal
+        throw `${URL}: ${error.res.statusText}`;
+      }
+      retriesLeft -= 1;
+      // eslint-disable-next-line no-await-in-loop
+      await delay(retryDelay);
+    }
+  }
+  // This should be unreachable, but the linter demands a consistent return.
+  return null;
 };
 
 /**
@@ -84,7 +105,10 @@ export const retryFetch = (
  */
 
 export const fetchWithLanguageAndSubscription = (URL, config, lang) => {
-  return fetch(addSubscriptionParam(addLocaleParam(URL, lang), config), {
-    headers: { 'Accept-Language': lang },
-  });
+  return fetchWithErrors(
+    addSubscriptionParam(addLocaleParam(URL, lang), config),
+    {
+      headers: { 'Accept-Language': lang },
+    },
+  );
 };

app/util/xhrPromise.js

@@ -1,3 +1,5 @@
+import { fetchWithErrors } from './fetchUtils';
+
 function serialize(obj, prefix) {
   if (!obj) {
     return '';
@@ -17,7 +19,7 @@ function serialize(obj, prefix) {
 
 // Return Promise for a url json get request
 export function getJson(url, params) {
-  return fetch(
+  return fetchWithErrors(
     encodeURI(url) +
       (params ? (url.search(/\?/) === -1 ? '?' : '&') + serialize(params) : ''),
     {
@@ -33,7 +35,7 @@ export function getJson(url, params) {
 
 // Return Promise for a json post request
 export function postJson(url, params, payload) {
-  return fetch(
+  return fetchWithErrors(
     encodeURI(url) +
       (params ? (url.search(/\?/) === -1 ? '?' : '&') + serialize(params) : ''),
     {

test/unit/util/fetchUtil.test.js

@@ -1,7 +1,11 @@
 import { expect, assert } from 'chai';
 import { describe, it } from 'mocha';
 import fetchMock from 'fetch-mock';
-import { retryFetch } from '../../../app/util/fetchUtils';
+import {
+  FetchError,
+  fetchWithErrors,
+  retryFetch,
+} from '../../../app/util/fetchUtils';
 
 // retryFetch retries fetch requests (url, options, retry count, delay) where total number or calls is initial request + retry count
 
@@ -10,6 +14,48 @@ const testUrl =
 
 const testJSONResponse = '{"test": 3}';
 
+describe('fetchWithErrors', () => {
+  it('on 201, should resolve with the Response', async () => {
+    fetchMock.get(testUrl, {
+      status: 201,
+      body: testJSONResponse,
+    });
+    try {
+      const res = await fetchWithErrors(testUrl);
+      expect(res.ok).to.equal(true);
+      expect(res.status).to.equal(201);
+      expect(res.url).to.equal(testUrl);
+      const body = await res.text();
+      expect(body).to.equal(testJSONResponse);
+    } finally {
+      fetchMock.restore();
+    }
+  });
+
+  it('on 500, should reject with a FetchError', async () => {
+    fetchMock.get(testUrl, {
+      status: 500,
+      body: 'nope',
+      headers: { foo: 'bar' },
+    });
+    try {
+      await fetchWithErrors(testUrl);
+      expect.fail('should have rejected');
+    } catch (err) {
+      expect(err).to.be.an.instanceof(FetchError);
+      expect(err.name).to.equal('FetchError');
+      expect(err.reqUrl).to.equal(testUrl);
+      expect(err.res.ok).to.equal(false);
+      expect(err.res.status).to.equal(500);
+      expect(err.res.url).to.equal(testUrl);
+      const body = await err.res.text();
+      expect(body).to.equal('nope');
+    } finally {
+      fetchMock.restore();
+    }
+  });
+});
+
 describe('retryFetch', () => {
   /* eslint-disable no-unused-vars */
   it('fetching something that does not exist with 5 retries should give Not Found error and 6 requests in total should be made ', done => {
@@ -30,7 +76,8 @@ describe('retryFetch', () => {
           fetchMock.restore();
           done();
         },
-      );
+      )
+      .catch(done);
   });
 
   it('fetch with larger fetch timeout should take longer', done => {
@@ -75,42 +122,35 @@ describe('retryFetch', () => {
               done();
             },
           );
-      });
+      })
+      .catch(done);
   });
 
   it('fetch that gives 200 should not be retried', done => {
     fetchMock.get(testUrl, testJSONResponse);
     retryFetch(testUrl, {}, 5, 10)
       .then(res => res.json())
-      .then(
-        result => {
-          // calls has array of requests made to given URL
-          const calls = fetchMock.calls(
-            'https://dev-api.digitransit.fi/timetables/v1/hsl/routes/routes.json',
-          );
-          expect(calls.length).to.equal(1);
-          fetchMock.restore();
-          done();
-        },
-        err => {
-          assert.fail('No error should have been thrown');
-        },
-      );
+      .then(result => {
+        // calls has array of requests made to given URL
+        const calls = fetchMock.calls(
+          'https://dev-api.digitransit.fi/timetables/v1/hsl/routes/routes.json',
+        );
+        expect(calls.length).to.equal(1);
+        fetchMock.restore();
+        done();
+      })
+      .catch(done);
   });
 
   it('fetch that gives 200 should have correct result data', done => {
     fetchMock.get(testUrl, testJSONResponse);
     retryFetch(testUrl, {}, 5, 10)
       .then(res => res.json())
-      .then(
-        result => {
-          expect(result.test).to.equal(3);
-          fetchMock.restore();
-          done();
-        },
-        err => {
-          assert.fail('No error should have been thrown');
-        },
-      );
+      .then(result => {
+        expect(result.test).to.equal(3);
+        fetchMock.restore();
+        done();
+      })
+      .catch(done);
   });
 });