fix: implement cache modes correctly

these changes align our cache modes with the whatwg fetch spec.
previously, 'reload' was behaving how 'no-cache' is meant to and
'no-cache' wasn't implemented at all

PR-URL: #52
Credit: @nlf
Close: #52
Reviewed-by: @wraithgar

lib/cache/entry.js

@@ -145,6 +145,12 @@ class CacheEntry {
       return
     }
 
+    // a cache mode of 'reload' means to behave as though we have no cache
+    // on the way to the network. return undefined to allow cacheFetch to
+    // create a brand new request no matter what.
+    if (options.cache === 'reload')
+      return
+
     // find the specific entry that satisfies the request
     let match
     for (const entry of matches) {

lib/cache/index.js

@@ -7,7 +7,7 @@ const cacheFetch = async (request, options) => {
   // try to find a cached entry that satisfies this request
   const entry = await CacheEntry.find(request, options)
   if (!entry) {
-    // no cached result, if the cache mode is only-if-cached that's a failure
+    // no cached result, if the cache mode is 'only-if-cached' that's a failure
     if (options.cache === 'only-if-cached')
       throw new NotCachedError(request.url)
 
@@ -17,22 +17,21 @@ const cacheFetch = async (request, options) => {
     return entry.store('miss')
   }
 
-  // we have a cached response that satisfies this request, however
-  // if the cache mode is reload the user explicitly wants us to revalidate
-  if (options.cache === 'reload')
+  // we have a cached response that satisfies this request, however if the cache
+  // mode is 'no-cache' then we send the revalidation request no matter what
+  if (options.cache === 'no-cache')
     return entry.revalidate(request, options)
 
-  // if the cache mode is either force-cache or only-if-cached we will only
-  // respond with a cached entry, even if it's stale. set the status to the
-  // appropriate value based on whether revalidation is needed and respond
-  // from the cache
+  // if the cached entry is not stale, or if the cache mode is 'force-cache' or
+  // 'only-if-cached' we can respond with the cached entry. set the status
+  // based on the result of needsRevalidation and respond
   const _needsRevalidation = entry.policy.needsRevalidation(request)
   if (options.cache === 'force-cache' ||
       options.cache === 'only-if-cached' ||
       !_needsRevalidation)
     return entry.respond(request.method, options, _needsRevalidation ? 'stale' : 'hit')
 
-  // cache entry might be stale, revalidate it and return a response
+  // if we got here, the cache entry is stale so revalidate it
   return entry.revalidate(request, options)
 }
 

test/cache.js

@@ -211,7 +211,7 @@ t.test('cache hit, no revalidation', async (t) => {
   t.ok(res.headers.has('x-local-cache-time'))
 })
 
-t.test('cache hit, cache mode reload 304', async (t) => {
+t.test('cache hit, cache mode no-cache 304', async (t) => {
   const srv = nock(HOST)
     .get('/test')
     .reply(200, CONTENT, {
@@ -233,7 +233,7 @@ t.test('cache hit, cache mode reload 304', async (t) => {
   await cacheRes.buffer() // drain it immediately so it stores to the cache
   t.ok(srv.isDone(), 'first req fulfilled')
 
-  const res = await fetch(`${HOST}/test`, { cachePath: dir, retry: false, cache: 'reload' })
+  const res = await fetch(`${HOST}/test`, { cachePath: dir, retry: false, cache: 'no-cache' })
   const buf = await res.buffer()
   t.ok(revalidateSrv.isDone(), 'second req fulfilled')
   t.same(buf, CONTENT, 'got the right content')
@@ -254,7 +254,7 @@ t.test('cache hit, cache mode reload 304', async (t) => {
   t.equal(entries.length, 2, 'should have 2 entries')
 })
 
-t.test('cache hit, cache mode reload 200', async (t) => {
+t.test('cache hit, cache mode no-cache 200', async (t) => {
   const srv = nock(HOST)
     .get('/test')
     .reply(200, CONTENT, {
@@ -279,7 +279,7 @@ t.test('cache hit, cache mode reload 200', async (t) => {
   await cacheRes.buffer() // drain it immediately so it stores to the cache
   t.ok(srv.isDone(), 'first req fulfilled')
 
-  const res = await fetch(`${HOST}/test`, { cachePath: dir, retry: false, cache: 'reload' })
+  const res = await fetch(`${HOST}/test`, { cachePath: dir, retry: false, cache: 'no-cache' })
   const buf = await res.buffer()
   t.ok(revalidateSrv.isDone(), 'second req fulfilled')
   t.same(buf, CONTENT, 'got the right content')
@@ -304,6 +304,42 @@ t.test('cache hit, cache mode reload 200', async (t) => {
   t.equal(entries[0].metadata.resHeaders.etag, '"beefcafe"', 'new etag takes priority')
 })
 
+t.test('cache mode reload', async (t) => {
+  const srv = nock(HOST)
+    .get('/test')
+    .twice()
+    .reply(200, CONTENT, {
+      ...getHeaders(CONTENT),
+      etag: '"beefc0ffee"',
+    })
+
+  const dir = t.testdir()
+  const reqKey = cacheKey(new Request(`${HOST}/test`))
+  const cacheRes = await fetch(`${HOST}/test`, { cachePath: dir, retry: false })
+  await cacheRes.buffer() // drain it immediately so it stores to the cache
+
+  // reload will always give a status of miss and send a full request
+  const res = await fetch(`${HOST}/test`, { cachePath: dir, retry: false, cache: 'reload' })
+  const buf = await res.buffer()
+  t.ok(srv.isDone(), 'both requests fulfilled')
+  t.same(buf, CONTENT, 'got the right content')
+  t.equal(res.status, 200, 'got a 200')
+  t.equal(res.url, `${HOST}/test`, 'has the right url property')
+  t.equal(res.headers.get('cache-control'), 'max-age=300', 'kept cache-control')
+  t.equal(res.headers.get('content-type'), 'application/octet-stream', 'kept content-type')
+  t.equal(res.headers.get('content-length'), `${CONTENT.length}`, 'kept content-length')
+  t.equal(res.headers.get('etag'), '"beefc0ffee"', 'kept the etag')
+  t.equal(res.headers.get('x-local-cache'), encodeURIComponent(dir), 'encoded the path')
+  t.equal(res.headers.get('x-local-cache-status'), 'miss', 'got a cache miss')
+  t.equal(res.headers.get('x-local-cache-key'), encodeURIComponent(reqKey), 'got the right cache key')
+  // just make sure x-local-cache-time is set, no need to assert its value
+  t.ok(res.headers.has('x-local-cache-time'))
+  t.notOk(res.headers.has('x-local-cache-hash'), 'does not have a hash header')
+
+  const entries = await cacache.index.compact(dir, reqKey, () => false)
+  t.equal(entries.length, 2, 'should have 2 entries')
+})
+
 t.test('cache hit, stale but mode is only-if-cached', async (t) => {
   // rewind time to make cacache put a timestamp in the past
   const now = Date.now()

test/integrity.js

@@ -138,7 +138,7 @@ t.test('checks integrity on cache fetch too', async (t) => {
   const safetch = fetch.defaults({
     cacheManager: CACHE,
     integrity: INTEGRITY,
-    cache: 'reload',
+    cache: 'no-cache',
   })
 
   const goodRes = await safetch(`${HOST}/test`)