Repository: GoogleChrome/sw-precache (js)

Issue:
Don't include the cache-busting URL parameter in the cache key
Right now, we include the cache-busting URL parameter in the key name for the cache entries. This works fine with the logic `sw-precache` has in place for doing cache lookups, but if we excluded the cache-busting URL parameter, we would play nicely with service worker code outside of `sw-precache` that uses `caches.match()`.

Pull request:
Naming/logic cleanup around cache keys
R: @gauntface @wibblymat @addyosmani 

This PR's primary purpose is to use asset URLs without cache-busting parameters as the key names in the caches that `sw-precache` maintains. This should make them play nicer with third-party service worker code that calls `caches.match(assetUrl)`, since `assetUrl` obviously won't have those cache-busting parameters.

In the course of updating that logic, I also updated some comments in the `fetch` handler. That led me to simplify the logic that handles an edge case where someone's deleted an entry from one of the caches, i.e. when the cache key is missing.

Closes #48

Patch:
diff --git a/lib/functions.js b/lib/functions.js
index 622b3a7..0417588 100644
--- a/lib/functions.js
+++ b/lib/functions.js
@@ -64,5 +64,14 @@ module.exports = {
       url.pathname += index;
     }
     return url.toString();
+  },
+
+  getCacheBustedUrl: function(url, now) {
+    now = now || Date.now();
+
+    var urlWithCacheBusting = new URL(url);
+    urlWithCacheBusting.search += (urlWithCacheBusting.search ? '&' : '') + 'sw-precache=' + now;
+
+    return urlWithCacheBusting.toString();
   }
 };
diff --git a/service-worker.tmpl b/service-worker.tmpl
index 933aa81..ae59ac2 100644
--- a/service-worker.tmpl
+++ b/service-worker.tmpl
@@ -64,19 +64,14 @@ self.addEventListener('install', function(event) {
         Object.keys(CurrentCacheNamesToAbsoluteUrl).filter(function(cacheName) {
           return allCacheNames.indexOf(cacheName) === -1;
         }).map(function(cacheName) {
-          var url = new URL(CurrentCacheNamesToAbsoluteUrl[cacheName]);
-          // Put in a cache-busting parameter to ensure we're caching a fresh response.
-          if (url.search) {
-            url.search += '&';
-          }
-          url.search += 'sw-precache=' + now;
-          var urlWithCacheBusting = url.toString();
+          var urlWithCacheBusting = getCacheBustedUrl(CurrentCacheNamesToAbsoluteUrl[cacheName],
+            now);
 
           return caches.open(cacheName).then(function(cache) {
             var request = new Request(urlWithCacheBusting, {credentials: 'same-origin'});
-            return fetch(request.clone()).then(function(response) {
+            return fetch(request).then(function(response) {
               if (response.ok) {
-                return cache.put(request, response);
+                return cache.put(CurrentCacheNamesToAbsoluteUrl[cacheName], response);
               }
 
               console.error('Request for %s returned a response with status %d, so not attempting to cache it.',
@@ -154,19 +149,20 @@ self.addEventListener('fetch', function(event) {
 
     if (cacheName) {
       event.respondWith(
-        // We can't call cache.match(event.request) since the entry in the cache will contain the
-        // cache-busting parameter. Instead, rely on the fact that each cache should only have one
-        // entry, and return that.
+        // Rely on the fact that each cache we manage should only have one entry, and return that.
         caches.open(cacheName).then(function(cache) {
           return cache.keys().then(function(keys) {
             return cache.match(keys[0]).then(function(response) {
-              return response || fetch(event.request).catch(function(e) {
-                console.error('Fetch for "%s" failed: %O', urlWithoutIgnoredParameters, e);
-              });
+              if (response) {
+                return response;
+              }
+              // If for some reason the response was deleted from the cache,
+              // raise and exception and fall back to the fetch() triggered in the catch().
+              throw Error('The cache ' + cacheName + ' is empty.');
             });
           });
         }).catch(function(e) {
-          console.error('Couldn\'t serve response for "%s" from cache: %O', urlWithoutIgnoredParameters, e);
+          console.warn('Couldn\'t serve response for "%s" from cache: %O', event.request.url, e);
           return fetch(event.request);
         })
       );


Test patch:
diff --git a/test/test.js b/test/test.js
index da0f768..308794b 100644
--- a/test/test.js
+++ b/test/test.js
@@ -388,3 +388,40 @@ describe('addDirectoryIndex', function() {
     done();
   });
 });
+
+describe('getCacheBustedUrl', function() {
+  it('should append the cache-busting parameter', function(done) {
+    var url = 'http://example.com/';
+    var cacheBustedUrl = externalFunctions.getCacheBustedUrl(url);
+    assert.notStrictEqual(url, cacheBustedUrl);
+    done();
+  });
+
+  it('should produce the same output when called with the same "now" parameter', function(done) {
+    var url = 'http://example.com/';
+    var now = 123;
+    var cacheBustedUrl = externalFunctions.getCacheBustedUrl(url, now);
+    var cacheBustedUrlPrime = externalFunctions.getCacheBustedUrl(url, now);
+    assert.strictEqual(cacheBustedUrl, cacheBustedUrlPrime);
+    done();
+  });
+
+  it('should produce different output when called twice, with a delay in between', function(done) {
+    var url = 'http://example.com/';
+    var cacheBustedUrl = externalFunctions.getCacheBustedUrl(url);
+    setTimeout(function() {
+      var cacheBustedUrlPrime = externalFunctions.getCacheBustedUrl(url);
+      assert.notStrictEqual(cacheBustedUrl, cacheBustedUrlPrime);
+      done();
+    }, 2);
+  });
+
+  it('should append the URL parameter without modifying any existing parameters', function(done) {
+    var url = 'http://example.com/?one=two';
+    var cacheBustedUrl = externalFunctions.getCacheBustedUrl(url);
+    // The cache-busted URL should consist of the basic URL, followed by '&' and then the
+    // cached-busting parameters, in order to keep the same semantics.
+    assert(cacheBustedUrl.indexOf(url + '&') === 0);
+    done();
+  });
+});