diff --git a/app/util/fetchUtils.js b/app/util/fetchUtils.js index f96f9f85d2..3726c72d81 100644 --- a/app/util/fetchUtils.js +++ b/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 }, + }, + ); }; diff --git a/app/util/xhrPromise.js b/app/util/xhrPromise.js index 43d29ce2d8..fc9234ae91 100644 --- a/app/util/xhrPromise.js +++ b/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) : ''), { diff --git a/test/unit/util/fetchUtil.test.js b/test/unit/util/fetchUtil.test.js index 74b1698c60..53f6cd957f 100644 --- a/test/unit/util/fetchUtil.test.js +++ b/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); }); });