Updated review

GoogleChrome · Jan 9, 2017 · b466513 · b466513
1 parent 5aeabec
commit b466513
Show file tree

Hide file tree

Showing 3 changed files with 24 additions and 16 deletions.
diff --git a/lighthouse-core/gather/gatherers/offline.js b/lighthouse-core/gather/gatherers/offline.js
@@ -26,7 +26,7 @@ class Offline extends Gatherer {
 
   afterPass(options, tracingData) {
     const navigationRecord = tracingData.networkRecords.filter(record => {
-      return cleanHash(new URL(record._url)) === cleanHash(new URL(options.url)) &&
+      return new URL(record._url).stripHash().href === new URL(options.url).stripHash().href &&
         record._fetchedViaServiceWorker;
     }).pop(); // Take the last record that matches.
 
@@ -36,8 +36,4 @@ class Offline extends Gatherer {
   }
 }
 
-function cleanHash(url) {
-  return url.href.removeURLFragment();
-}
-
 module.exports = Offline;
diff --git a/lighthouse-core/lib/url-shim.js b/lighthouse-core/lib/url-shim.js
@@ -57,4 +57,14 @@ URL.hostsMatch = function hostsMatch(urlA, urlB) {
   }
 };
 
+/**
+ * Removes hash from URL
+ * @return {URL} Url object chaining
+ */
+URL.prototype.stripHash = function stripHash() {
+  this.hash = '';
+
+  return this;
+};
+
 module.exports = URL;
diff --git a/lighthouse-core/test/gather/gatherers/offline-test.js b/lighthouse-core/test/gather/gatherers/offline-test.js
@@ -37,9 +37,19 @@ describe('Offline gatherer', () => {
       url: 'https://do-not-match.com/',
       driver: mockDriver
     };
-    return offlineGather.afterPass(options, tracingData).then(artifact => {
-      assert.strictEqual(artifact, -1);
-    });
+    const optionsWithQueryString = {
+      url: 'https://ifixit-pwa.appspot.com/?history',
+      driver: mockDriver
+    };
+
+    return Promise.all([
+      offlineGather.afterPass(options, tracingData).then(artifact => {
+        assert.strictEqual(artifact, -1);
+      }),
+      offlineGather.afterPass(optionsWithQueryString, tracingData).then(artifact => {
+        assert.strictEqual(artifact, -1);
+      }),
+    ]);
   });
 
   it('returns an artifact set to 200 when offline loading succeeds', () => {
@@ -52,21 +62,13 @@ describe('Offline gatherer', () => {
       url: 'https://ifixit-pwa.appspot.com/#/history',
       driver: mockDriver
     };
-    const optionsWithQueryString = {
-      url: 'https://ifixit-pwa.appspot.com/?history',
-      driver: mockDriver
-    };
-
     return Promise.all([
       offlineGather.afterPass(options, tracingData).then(artifact => {
         assert.strictEqual(artifact, 200);
       }),
       offlineGather.afterPass(optionsWithFragment, tracingData).then(artifact => {
         assert.strictEqual(artifact, 200);
       }),
-      offlineGather.afterPass(optionsWithQueryString, tracingData).then(artifact => {
-        assert.strictEqual(artifact, -1);
-      }),
     ]);
   });
 });