Prefer response.status === 200 to response.ok (#1805)

GoogleChrome · Dec 20, 2018 · bebeb49 · bebeb49
1 parent b294993
commit bebeb49
Show file tree

Hide file tree

Showing 4 changed files with 35 additions and 32 deletions.
diff --git a/packages/workbox-core/_private/cacheWrapper.mjs b/packages/workbox-core/_private/cacheWrapper.mjs
@@ -161,7 +161,7 @@ const matchWrapper = async ({
 
 /**
  * This method will call cacheWillUpdate on the available plugins (or use
- * response.ok) to determine if the Response is safe and valid to cache.
+ * status === 200) to determine if the Response is safe and valid to cache.
  *
  * @param {Object} options
  * @param {Request} options.request
@@ -204,7 +204,7 @@ const _isResponseSafeToCache = async ({request, response, event, plugins}) => {
 
   if (!pluginsUsed) {
     if (process.env.NODE_ENV !== 'production') {
-      if (!responseToCache.ok) {
+      if (!responseToCache.status === 200) {
         if (responseToCache.status === 0) {
           logger.warn(`The response for '${request.url}' is an opaque ` +
             `response. The caching strategy that you're using will not ` +
@@ -216,7 +216,7 @@ const _isResponseSafeToCache = async ({request, response, event, plugins}) => {
         }
       }
     }
-    responseToCache = responseToCache.ok ? responseToCache : null;
+    responseToCache = responseToCache.status === 200 ? responseToCache : null;
   }
 
   return responseToCache ? responseToCache : null;

diff --git a/packages/workbox-strategies/plugins/cacheOkAndOpaquePlugin.mjs b/packages/workbox-strategies/plugins/cacheOkAndOpaquePlugin.mjs
@@ -10,8 +10,8 @@ import '../_version.mjs';
 
 export default {
   /**
-   * Return return a response (i.e. allow caching) if the
-   * response is ok (i.e. 200) or is opaque.
+   * Returns a valid response (to allow caching) if the status is 200 (OK) or
+   * 0 (opaque).
    *
    * @param {Object} options
    * @param {Response} options.response
@@ -20,7 +20,7 @@ export default {
    * @private
    */
   cacheWillUpdate: ({response}) => {
-    if (response.ok || response.status === 0) {
+    if (response.status === 200 || response.status === 0) {
       return response;
     }
     return null;

diff --git a/test/workbox-core/node/_private/test-cacheWrapper.mjs b/test/workbox-core/node/_private/test-cacheWrapper.mjs
@@ -54,28 +54,29 @@ describe(`workbox-core cacheWrapper`, function() {
       expect(cacheResponse).to.equal(putResponse);
     });
 
-    it(`should not cache opaque responses by default`, async function() {
-      const testCache = await caches.open('TEST-CACHE');
-      const cacheOpenStub = sandbox.stub(global.caches, 'open');
-      const cachePutStub = sandbox.stub(testCache, 'put');
-      cacheOpenStub.callsFake(async (cacheName) => {
-        return testCache;
-      });
-      const putRequest = new Request('/test/string');
-      const putResponse = new Response('Response for /test/string', {
-        // The mock doesn't allow a status of zero due to a bug
-        // so mock to 1.
-        status: 1,
-      });
-      await cacheWrapper.put({
-        cacheName: 'TODO-CHANGE-ME',
-        request: putRequest,
-        response: putResponse,
-      });
+    // This covers opaque responses (0) and partial content responses (206).
+    for (const status of [0, 206]) {
+      it(`should not cache response.status of ${status} by default`, async function() {
+        const cacheName = 'test-cache';
+        const testCache = await caches.open(cacheName);
+        const cacheOpenStub = sandbox.stub(global.caches, 'open').resolves(testCache);
+        const cachePutSpy = sandbox.spy(testCache, 'put');
+
+        const putRequest = new Request('/test/string');
+        const putResponse = new Response('', {
+          status,
+        });
 
-      expect(cacheOpenStub.callCount).to.equal(0);
-      expect(cachePutStub.callCount).to.equal(0);
-    });
+        await cacheWrapper.put({
+          cacheName,
+          request: putRequest,
+          response: putResponse,
+        });
+
+        expect(cacheOpenStub.callCount).to.equal(0);
+        expect(cachePutSpy.callCount).to.equal(0);
+      });
+    }
 
     devOnly.it(`should not cache POST responses`, async function() {
       const testCache = await caches.open('TEST-CACHE');

diff --git a/test/workbox-strategies/node/test-cacheOkAndOpaquePlugin.mjs b/test/workbox-strategies/node/test-cacheOkAndOpaquePlugin.mjs
@@ -10,12 +10,14 @@ import {expect} from 'chai';
 import plugin from '../../../packages/workbox-strategies/plugins/cacheOkAndOpaquePlugin.mjs';
 
 describe(`[workbox-strategies] cacheOkAndOpaquePlugin`, function() {
-  it(`should return null if status is not ok and status is not opaque`, function() {
-    const response = new Response('Hello', {
-      status: 404,
+  for (const status of [206, 404]) {
+    it(`should return null when status is ${status}`, function() {
+      const response = new Response('Hello', {
+        status,
+      });
+      expect(plugin.cacheWillUpdate({response})).to.equal(null);
     });
-    expect(plugin.cacheWillUpdate({response})).to.equal(null);
-  });
+  }
 
   it(`should return Response if status is opaque`, function() {
     const response = new Response('Hello', {