core(start-url): reorganize start_url gatherer (#4838)

GoogleChrome · Mar 21, 2018 · 146812f · 146812f
1 parent cf70143
commit 146812f
Show file tree

Hide file tree

Showing 2 changed files with 76 additions and 60 deletions.
diff --git a/lighthouse-core/gather/gatherers/start-url.js b/lighthouse-core/gather/gatherers/start-url.js
@@ -9,76 +9,92 @@ const Gatherer = require('./gatherer');
 const manifestParser = require('../../lib/manifest-parser');
 
 class StartUrl extends Gatherer {
-  executeFetchRequest(driver, url) {
-    return driver.evaluateAsync(
-      `fetch('${url}')`
-    );
-  }
-
+  /**
+   * Grab the manifest, extract it's start_url, attempt to `fetch()` it while offline
+   * @param {*} options
+   * @return {{statusCode: number, debugString?: string}}
+   */
   afterPass(options) {
-    return options.driver.goOnline(options)
-      .then(() => options.driver.getAppManifest())
+    const driver = options.driver;
+    return driver.goOnline(options)
+      .then(() => driver.getAppManifest())
+      .then(response => driver.goOffline(options).then(() => response))
       .then(response => response && manifestParser(response.data, response.url, options.url))
       .then(manifest => {
-        if (!manifest || !manifest.value) {
-          const detailedMsg = manifest && manifest.debugString;
-
-          if (detailedMsg) {
-            const debugString = `Error fetching web app manifest: ${detailedMsg}`;
-            return {
-              statusCode: -1,
-              debugString,
-            };
-          } else {
-            const debugString = `No usable web app manifest found on page ${options.url}`;
-            return {
-              statusCode: -1,
-              debugString,
-            };
-          }
+        const {isReadFailure, reason, startUrl} = this._readManifestStartUrl(manifest);
+        if (isReadFailure) {
+          return {statusCode: -1, debugString: reason};
         }
 
-        if (manifest.value.start_url.debugString) {
-          // Even if the start URL had an error, the browser will still supply a fallback URL.
-          // Therefore, we only set the debugString here and continue with the fetch.
-          return {
-            statusCode: -1,
-            debugString: manifest.value.start_url.debugString,
-          };
-        }
+        return this._attemptManifestFetch(options.driver, startUrl);
+      }).catch(() => {
+        return {statusCode: -1, debugString: 'Unable to fetch start URL via service worker'};
+      });
+  }
 
-        const startUrl = manifest.value.start_url.value;
+  /**
+   * Read the parsed manifest and return failure reasons or the startUrl
+   * @param {Manifest} manifest
+   * @return {{isReadFailure: true, reason: string}|{isReadFailure: false, startUrl: string}}
+   */
+  _readManifestStartUrl(manifest) {
+    if (!manifest || !manifest.value) {
+      const detailedMsg = manifest && manifest.debugString;
 
-        return (new Promise(resolve => {
-          options.driver.on('Network.responseReceived', function responseReceived({response}) {
-            if (response.url === startUrl) {
-              options.driver.off('Network.responseReceived', responseReceived);
+      if (detailedMsg) {
+        return {isReadFailure: true, reason: `Error fetching web app manifest: ${detailedMsg}`};
+      } else {
+        return {isReadFailure: true, reason: `No usable web app manifest found on page`};
+      }
+    }
 
-              if (!response.fromServiceWorker) {
-                return resolve({
-                  statusCode: -1,
-                  debugString: 'Unable to fetch start URL via service worker',
-                });
-              }
+    // Even if the start URL had an error, the browser will still supply a fallback URL.
+    // Therefore, we only set the debugString here and continue with the fetch.
+    if (manifest.value.start_url.debugString) {
+      return {isReadFailure: true, reason: manifest.value.start_url.debugString};
+    }
 
-              return resolve({
-                statusCode: response.status,
-                debugString: '',
-              });
-            }
+    return {isReadFailure: false, startUrl: manifest.value.start_url.value};
+  }
+
+  /**
+   * Try to `fetch(start_url)`, return true if fetched by SW
+   * Resolves when we have a matched network request
+   * @param {!Driver} driver
+   * @param {!string} startUrl
+   * @return {Promise<{statusCode: ?number, debugString: ?string}>}
+   */
+  _attemptManifestFetch(driver, startUrl) {
+    // Wait up to 3s to get a matched network request from the fetch() to work
+    const timeoutPromise = new Promise(resolve =>
+      setTimeout(
+        () => resolve({statusCode: -1, debugString: 'Timed out waiting for fetched start_url'}),
+        3000
+      )
+    );
+
+    const fetchPromise = new Promise(resolve => {
+      driver.on('Network.responseReceived', onResponseReceived);
+
+      function onResponseReceived({response}) {
+        // ignore mismatched URLs
+        if (response.url !== startUrl) return;
+        driver.off('Network.responseReceived', onResponseReceived);
+
+        if (!response.fromServiceWorker) {
+          return resolve({
+            statusCode: -1,
+            debugString: 'Unable to fetch start URL via service worker',
           });
+        }
+        // Successful SW-served fetch of the start_URL
+        return resolve({statusCode: response.status});
+      }
+    });
 
-          options.driver.goOffline(options)
-            .then(() => this.executeFetchRequest(options.driver, startUrl))
-            .then(() => options.driver.goOnline(options))
-            .catch(() => {
-              resolve({
-                statusCode: -1,
-                debugString: 'Unable to fetch start URL via service worker',
-              });
-            });
-        }));
-      });
+    return driver
+      .evaluateAsync(`fetch('${startUrl}')`)
+      .then(() => Promise.race([fetchPromise, timeoutPromise]));
   }
 }
 

diff --git a/lighthouse-core/test/gather/gatherers/start-url-test.js b/lighthouse-core/test/gather/gatherers/start-url-test.js
@@ -120,7 +120,7 @@ describe('Start-url gatherer', () => {
     return startUrlGatherer.afterPass(options, tracingData)
       .then(artifact => {
         assert.equal(artifact.debugString,
-          `No usable web app manifest found on page ${options.url}`);
+          `No usable web app manifest found on page`);
       });
   });