Fix fetch-with-cache (#7083)

The `fetch-with-cache` utility was failing to actually cache anything.
It would cache an object with cache time and URL, and would return that
instead of a valid response. This resulted in the error:
`TypeError: fourByteResponse.json is not a function`

The utility was updated to call `.json()` within itself, and cache the
JSON response. The function signature was updated as well, to expect an
options object instead of just the `cacheRefreshTime` option. The
timeout was added to this options object, which helped with testing.

The utility method now also handles unsuccessfull responses, and
incompatible `fetch` options.

ui/app/helpers/utils/fetch-with-cache.js

@@ -2,27 +2,52 @@ import {
   loadLocalStorageData,
   saveLocalStorageData,
 } from '../../../lib/local-storage-helpers'
-import http from './fetch'
+import fetchWithTimeout from './fetch'
 
-const fetch = http({
-  timeout: 30000,
-})
+const fetchWithCache = async (url, fetchOptions = {}, { cacheRefreshTime = 360000, timeout = 30000 } = {}) => {
+  if (fetchOptions.body || (fetchOptions.method && fetchOptions.method !== 'GET')) {
+    throw new Error('fetchWithCache only supports GET requests')
+  }
+  if (!(fetchOptions.headers instanceof Headers)) {
+    fetchOptions.headers = new Headers(fetchOptions.headers)
+  }
+  if (
+    fetchOptions.headers &&
+    fetchOptions.headers.has('Content-Type') &&
+    fetchOptions.headers.get('Content-Type') !== 'application/json'
+  ) {
+    throw new Error('fetchWithCache only supports JSON responses')
+  }
 
-export default function fetchWithCache (url, opts, cacheRefreshTime = 360000) {
   const currentTime = Date.now()
   const cachedFetch = loadLocalStorageData('cachedFetch') || {}
-  const { cachedUrl, cachedTime } = cachedFetch[url] || {}
-  if (cachedUrl && currentTime - cachedTime < cacheRefreshTime) {
-    return cachedFetch[url]
+  const { cachedResponse, cachedTime } = cachedFetch[url] || {}
+  if (cachedResponse && currentTime - cachedTime < cacheRefreshTime) {
+    return cachedResponse
   } else {
-    cachedFetch[url] = { cachedUrl: url, cachedTime: currentTime }
-    saveLocalStorageData(cachedFetch, 'cachedFetch')
-    return fetch(url, {
+    fetchOptions.headers.set('Content-Type', 'application/json')
+    const _fetch = timeout ?
+      fetchWithTimeout({ timeout }) :
+      fetch
+    const response = await _fetch(url, {
       referrerPolicy: 'no-referrer-when-downgrade',
       body: null,
       method: 'GET',
       mode: 'cors',
-      ...opts,
+      ...fetchOptions,
     })
+    if (!response.ok) {
+      throw new Error(`Fetch failed with status '${response.status}': '${response.statusText}'`)
+    }
+    const responseJson = await response.json()
+    const cacheEntry = {
+      cachedResponse: responseJson,
+      cachedTime: currentTime,
+    }
+    cachedFetch[url] = cacheEntry
+    saveLocalStorageData(cachedFetch, 'cachedFetch')
+    return responseJson
   }
 }
+
+export default fetchWithCache

ui/app/helpers/utils/fetch-with-cache.test.js

@@ -0,0 +1,133 @@
+import assert from 'assert'
+import nock from 'nock'
+import sinon from 'sinon'
+import proxyquire from 'proxyquire'
+
+const fakeLocalStorageHelpers = {}
+const fetchWithCache = proxyquire('./fetch-with-cache', {
+  '../../../lib/local-storage-helpers': fakeLocalStorageHelpers,
+}).default
+
+describe('Fetch with cache', () => {
+  beforeEach(() => {
+    fakeLocalStorageHelpers.loadLocalStorageData = sinon.stub()
+    fakeLocalStorageHelpers.saveLocalStorageData = sinon.stub()
+  })
+  afterEach(() => {
+    sinon.restore()
+    nock.cleanAll()
+  })
+
+  it('fetches a url', async () => {
+    nock('https://fetchwithcache.metamask.io')
+      .get('/price')
+      .reply(200, '{"average": 1}')
+
+    const response = await fetchWithCache('https://fetchwithcache.metamask.io/price')
+    assert.deepEqual(response, {
+      average: 1,
+    })
+  })
+
+  it('returns cached response', async () => {
+    nock('https://fetchwithcache.metamask.io')
+      .get('/price')
+      .reply(200, '{"average": 2}')
+
+    fakeLocalStorageHelpers.loadLocalStorageData.returns({
+      'https://fetchwithcache.metamask.io/price': {
+        cachedResponse: { average: 1 },
+        cachedTime: Date.now(),
+      },
+    })
+
+    const response = await fetchWithCache('https://fetchwithcache.metamask.io/price')
+    assert.deepEqual(response, {
+      average: 1,
+    })
+  })
+
+  it('fetches URL again after cache refresh time has passed', async () => {
+    nock('https://fetchwithcache.metamask.io')
+      .get('/price')
+      .reply(200, '{"average": 3}')
+
+    fakeLocalStorageHelpers.loadLocalStorageData.returns({
+      'https://fetchwithcache.metamask.io/cached': {
+        cachedResponse: { average: 1 },
+        cachedTime: Date.now() - 1000,
+      },
+    })
+
+    const response = await fetchWithCache('https://fetchwithcache.metamask.io/price', {}, { cacheRefreshTime: 123 })
+    assert.deepEqual(response, {
+      average: 3,
+    })
+  })
+
+  it('should abort the request when the custom timeout is hit', async () => {
+    nock('https://fetchwithcache.metamask.io')
+      .get('/price')
+      .delay(100)
+      .reply(200, '{"average": 4}')
+
+    try {
+      await fetchWithCache('https://fetchwithcache.metamask.io/price', {}, { timeout: 20 })
+      assert.fail('Request should be aborted')
+    } catch (e) {
+      assert.deepEqual(e.message, 'Aborted')
+    }
+  })
+
+  it('throws when the response is unsuccessful', async () => {
+    nock('https://fetchwithcache.metamask.io')
+      .get('/price')
+      .reply(500, '{"average": 6}')
+
+    try {
+      await fetchWithCache('https://fetchwithcache.metamask.io/price')
+      assert.fail('Request should throw')
+    } catch (e) {
+      assert.ok(e)
+    }
+  })
+
+  it('throws when a POST request is attempted', async () => {
+    nock('https://fetchwithcache.metamask.io')
+      .post('/price')
+      .reply(200, '{"average": 7}')
+
+    try {
+      await fetchWithCache('https://fetchwithcache.metamask.io/price', { method: 'POST' })
+      assert.fail('Request should throw')
+    } catch (e) {
+      assert.ok(e)
+    }
+  })
+
+  it('throws when the request has a truthy body', async () => {
+    nock('https://fetchwithcache.metamask.io')
+      .get('/price')
+      .reply(200, '{"average": 8}')
+
+    try {
+      await fetch('https://fetchwithcache.metamask.io/price', { body: 1 })
+      assert.fail('Request should throw')
+    } catch (e) {
+      assert.ok(e)
+    }
+  })
+
+  it('throws when the request has an invalid Content-Type header', async () => {
+    nock('https://fetchwithcache.metamask.io')
+      .get('/price')
+      .reply(200, '{"average": 9}')
+
+    try {
+      await fetch('https://fetchwithcache.metamask.io/price', { headers: { 'Content-Type': 'text/plain' } })
+      assert.fail('Request should throw')
+    } catch (e) {
+      assert.ok(e)
+    }
+  })
+})

ui/app/helpers/utils/transactions.util.js

@@ -41,10 +41,8 @@ async function getMethodFrom4Byte (fourBytePrefix) {
     mode: 'cors',
   }))
 
-  const fourByteJSON = await fourByteResponse.json()
-
-  if (fourByteJSON.count === 1) {
-    return fourByteJSON.results[0].text_signature
+  if (fourByteResponse.count === 1) {
+    return fourByteResponse.results[0].text_signature
   } else {
     return null
   }