Address more review feedback

.eslintrc.js

@@ -41,10 +41,13 @@ module.exports = {
     },
   }, {
     files: [
+      'infra/testing/webdriver/executeAsyncAndCatch.js',
+      'infra/utils/log-helper.js',
       'packages/workbox-core/_private/logger.mjs',
       'packages/workbox-sw/_default.mjs',
-      'infra/utils/log-helper.js',
       'packages/workbox-cli/src/lib/logger.js',
+      'test/workbox-window/integration/test.js',
+      'test/workbox-window/unit/test-Workbox.mjs',
     ],
     rules: {
       'no-console': 0,
@@ -115,13 +118,5 @@ module.exports = {
     rules: {
       'header/header': [2, 'block', {pattern: 'Copyright \\d{4} Google LLC'}]
     }
-  }, {
-    files: [
-      'test/workbox-window/integration/test.js',
-      'test/workbox-window/unit/test-Workbox.mjs',
-    ],
-    rules: {
-      'no-console': 0,
-    }
   }],
 };

gulp-tasks/utils/build-browser-bundle.js

@@ -178,10 +178,7 @@ module.exports = (packagePath, buildType) => {
     },
   })
       .on('error', (err) => {
-        const args = [];
-        Object.keys(err).forEach((key) => {
-          args.push(`${key}: ${err[key]}`);
-        });
+        const args = Object.keys(err).map((key) => `${key}: ${err[key]}`);
         logHelper.error(err, `\n\n${args.join('\n')}`);
         throw err;
       })

gulp-tasks/utils/build-window-bundle.js

@@ -73,14 +73,12 @@ module.exports = (packagePath, buildType) => {
         throw new Error(`Unhandled Rollup Warning: ${warning}`);
       }
     },
-  }).on('error', (err) => {
-    const args = [];
-    Object.keys(err).forEach((key) => {
-      args.push(`${key}: ${err[key]}`);
-    });
-    logHelper.error(err, `\n\n${args.join('\n')}`);
-    throw err;
   })
+      .on('error', (err) => {
+        const args = Object.keys(err).map((key) => `${key}: ${err[key]}`);
+        logHelper.error(err, `\n\n${args.join('\n')}`);
+        throw err;
+      })
   // We must give the generated stream the same name as the entry file
   // for the sourcemaps to work correctly
       .pipe(source(moduleBrowserPath))

infra/testing/activate-and-control.js

@@ -8,6 +8,7 @@
 
 const activateSWSafari = require('./activate-sw-safari');
 
+// TODO(philipwalton): remove this in favor of using workbox-window.
 module.exports = async (swUrl) => {
   if (global.__workbox.seleniumBrowser.getId() === 'safari') {
     return activateSWSafari(swUrl);

infra/testing/activate-sw-safari.js

@@ -6,6 +6,7 @@
   https://opensource.org/licenses/MIT.
 */
 
+// TODO(philipwalton): remove this in favor of using workbox-window.
 module.exports = async (swUrl) => {
   // First step: Wait for the page to activate
   let error = await global.__workbox.webdriver.executeAsyncScript((swUrl, cb) => {

infra/testing/webdriver/executeAsyncAndCatch.js

@@ -0,0 +1,31 @@
+/*
+  Copyright 2019 Google LLC
+
+  Use of this source code is governed by an MIT-style
+  license that can be found in the LICENSE file or at
+  https://opensource.org/licenses/MIT.
+*/
+
+
+// Store local references of these globals.
+const {webdriver} = global.__workbox;
+
+/**
+ * Executes the passed function (and args) async and logs any errors that
+ * occur. Errors are assumed to be passed to the callback as an object
+ * with the `error` property.
+ *
+ * @param {...*} args
+ * @return {*}
+ */
+const executeAsyncAndCatch = async (...args) => {
+  const result = await webdriver.executeAsyncScript(...args);
+
+  if (result && result.error) {
+    console.error(result.error);
+    throw new Error('Error executing async script');
+  }
+  return result;
+};
+
+module.exports = {executeAsyncAndCatch};

infra/testing/webdriver/getLastWindowHandle.js

@@ -0,0 +1,23 @@
+/*
+  Copyright 2019 Google LLC
+
+  Use of this source code is governed by an MIT-style
+  license that can be found in the LICENSE file or at
+  https://opensource.org/licenses/MIT.
+*/
+
+
+// Store local references of these globals.
+const {webdriver} = global.__workbox;
+
+/**
+ * Gets the window handle of the last openned tab.
+ *
+ * @return {string}
+ */
+const getLastWindowHandle = async () => {
+  const allHandles = await webdriver.getAllWindowHandles();
+  return allHandles[allHandles.length - 1];
+};
+
+module.exports = {getLastWindowHandle};

infra/testing/webdriver/openNewTab.js

@@ -0,0 +1,36 @@
+/*
+  Copyright 2019 Google LLC
+
+  Use of this source code is governed by an MIT-style
+  license that can be found in the LICENSE file or at
+  https://opensource.org/licenses/MIT.
+*/
+
+
+const {getLastWindowHandle} = require('./getLastWindowHandle');
+
+// Store local references of these globals.
+const {webdriver} = global.__workbox;
+
+/**
+ * Opens a new window for the passed URL. If no URL is passed, a blank page
+ * is opened.
+ *
+ * @param {string} url
+ * @param {Object} options
+ * @return {string}
+ */
+const openNewTab = async (url) => {
+  await webdriver.executeAsyncScript((url, cb) => {
+    window.open(url);
+    cb();
+  }, url);
+
+  const lastHandle = await getLastWindowHandle();
+  await webdriver.switchTo().window(lastHandle);
+
+  // Return the handle of the window that was just opened.
+  return lastHandle;
+};
+
+module.exports = {openNewTab};

infra/testing/webdriver/unregisterAllSws.js

@@ -0,0 +1,30 @@
+/*
+  Copyright 2019 Google LLC
+
+  Use of this source code is governed by an MIT-style
+  license that can be found in the LICENSE file or at
+  https://opensource.org/licenses/MIT.
+*/
+
+
+const {executeAsyncAndCatch} = require('./executeAsyncAndCatch');
+
+/**
+ * Unregisters any active SWs so the next page load can start clean.
+ * Note: a new page load is needed before controlling SWs stop being active.
+ */
+const unregisterAllSws = async () => {
+  await executeAsyncAndCatch(async (cb) => {
+    try {
+      const regs = await navigator.serviceWorker.getRegistrations();
+      for (const reg of regs) {
+        await reg.unregister();
+      }
+      cb();
+    } catch (error) {
+      cb({error: error.stack});
+    }
+  });
+};
+
+module.exports = {unregisterAllSws};

infra/testing/webdriver/windowLoaded.js

@@ -0,0 +1,30 @@
+/*
+  Copyright 2019 Google LLC
+
+  Use of this source code is governed by an MIT-style
+  license that can be found in the LICENSE file or at
+  https://opensource.org/licenses/MIT.
+*/
+
+
+const {executeAsyncAndCatch} = require('./executeAsyncAndCatch');
+
+/**
+ * Waits for the current window to load if it's not already loaded.
+ */
+const windowLoaded = async () => {
+  // Wait for the window to load, so the `Workbox` global is available.
+  await executeAsyncAndCatch(async (cb) => {
+    try {
+      if (document.readyState === 'complete') {
+        cb();
+      } else {
+        addEventListener('load', () => cb());
+      }
+    } catch (error) {
+      cb({error: error.stack});
+    }
+  });
+};
+
+module.exports = {windowLoaded};

packages/workbox-window/Workbox.mjs

@@ -45,24 +45,28 @@ export class Workbox extends EventTargetShim {
    *
    * @param {string} scriptUrl The service worker script associated with this
    *     instance.
-   * @param {Object} options The service worker options associated with this
+   * @param {Object} [options] The service worker options associated with this
    *     instance.
    */
   constructor(scriptUrl, options = {}) {
     super();
 
     this._scriptUrl = scriptUrl;
     this._swOptions = options;
-
-    // this._registration = null;
-    // this._sw = null;
-    // this._externalSw = null;
-    // this._controllingSw = null;
     this._updateFoundCount = 0;
 
     // A promise that will be resolved once we have a SW reference.
     this._swPromise = new Promise((res) => this._swPromiseResolver = res);
 
+    // Instance variables initially not set.
+    // this._broadcastChannel;
+    // this._controllingSw;
+    // this._externalSw;
+    // this._registration;
+    // this._registrationTime;
+    // this._sw;
+    // this._waitingTimeout;
+
     // Bind event handler callbacks.
     this._onMessage = this._onMessage.bind(this);
     this._onStateChange = this._onStateChange.bind(this);
@@ -141,7 +145,8 @@ export class Workbox extends EventTargetShim {
         } else {
           logger.debug('A service worker with a different script URL is ' +
               'currently controlling the page.');
-          logger.debug('Fetching the new script...');
+          logger.debug('The browser is now fetching the new ' +
+              'service worker script...');
         }
       }
 

test/workbox-window/integration/test.js

@@ -10,95 +10,15 @@
 const {expect} = require('chai');
 const templateData = require('../../../infra/testing/server/template-data');
 const waitUntil = require('../../../infra/testing/wait-until');
+const {executeAsyncAndCatch} = require('../../../infra/testing/webdriver/executeAsyncAndCatch');
+const {getLastWindowHandle} = require('../../../infra/testing/webdriver/getLastWindowHandle');
+const {openNewTab} = require('../../../infra/testing/webdriver/openNewTab');
+const {unregisterAllSws} = require('../../../infra/testing/webdriver/unregisterAllSws');
+const {windowLoaded} = require('../../../infra/testing/webdriver/windowLoaded');
 
 // Store local references of these globals.
 const {webdriver, server} = global.__workbox;
 
-/**
- * Executes the passed function (and args) async and logs any errors that
- * occur. Errors are assumed to be passed to the callback as an object
- * with the `error` property.
- *
- * @param {...*} args
- * @return {*}
- */
-const executeAsyncAndCatch = async (...args) => {
-  const result = await webdriver.executeAsyncScript(...args);
-
-  if (result && result.error) {
-    console.error(result.error);
-    throw new Error('Error executing async script');
-  }
-  return result;
-};
-
-/**
- * Gets the window handle of the last openned tab.
- *
- * @return {string}
- */
-const getLastWindowHandle = async () => {
-  const allHandles = await webdriver.getAllWindowHandles();
-  return allHandles[allHandles.length - 1];
-};
-
-/**
- * Opens a new window for the passed URL. If no URL is passed, a blank page
- * is opened.
- *
- * @param {string} url
- * @param {Object} options
- * @return {string}
- */
-const openNewTab = async (url) => {
-  await webdriver.executeAsyncScript((url, cb) => {
-    window.open(url);
-    cb();
-  }, url);
-
-  const lastHandle = await getLastWindowHandle();
-  await webdriver.switchTo().window(lastHandle);
-
-  // Return the handle of the window that was just opened.
-  return lastHandle;
-};
-
-/**
- * Waits for the current window to load if it's not already loaded.
- */
-const windowLoaded = async () => {
-  // Wait for the window to load, so the `Workbox` global is available.
-  await executeAsyncAndCatch(async (cb) => {
-    try {
-      if (document.readyState === 'complete') {
-        cb();
-      } else {
-        addEventListener('load', () => cb());
-      }
-    } catch (error) {
-      cb({error: error.stack});
-    }
-  });
-};
-
-/**
- * Unregisters any active SWs so the next page load can start clean.
- * Note: a new page load is needed before controlling SWs stop being active.
- */
-const unregisterAllSws = async () => {
-  await executeAsyncAndCatch(async (cb) => {
-    try {
-      const regs = await navigator.serviceWorker.getRegistrations();
-      for (const reg of regs) {
-        await reg.unregister();
-      }
-      cb();
-    } catch (error) {
-      cb({error: error.stack});
-    }
-  });
-};
-
 const testServerOrigin = server.getAddress();
 const testPath = `${testServerOrigin}/test/workbox-window/static/`;
 const unitTestPath = `${testServerOrigin}/test/workbox-window/unit/`;