No longer register a fetch handler for each Router by default #752

jeffposnick · 2017-08-17T21:53:53Z

R: @addyosmani @gauntface
CC: @jakearchibald @surma

Fixes #721

This changes the default behavior of Router, as discussed in the proposal.

gauntface · 2017-08-17T23:23:57Z

packages/workbox-routing/src/lib/router.js

 *
 * It also allows you to define a "default" handler that applies to any requests
 * that don't explicitly match a `Route`, and a "catch" handler that responds
 * to any requests that throw an exception while being routed.
 *
+ * By default, the `Router` class won't register a `fetch` event handler,


Nit: Could we rephrase to:

You can use the handleRequest() method to pass a fetch event through the router and ultimately get a "Routed response" back. If you'd like this to be handled automatically for you, calling addFetchListener() will cause the Router to respond to requests

gauntface · 2017-08-17T23:25:25Z

packages/workbox-routing/src/lib/router.js

+   * router.registerRoute({route: imageRoute});
+   * router.addFetchListener();
+   */
+  addFetchListener() {


We should check to see if this method has already been called and if so, display a warning and return early.

Out of interest, what happens with this implementation if addFetchListener is called multiple times?

Sorry, I left that bit out—we agreed on returning a boolean based on whether that call actually registered the listener, and to prevent the listener from being registered multiple times. Will adjust.

gauntface · 2017-08-17T23:27:19Z

packages/workbox-routing/src/lib/router.js

+   */
+  addFetchListener() {
+    self.addEventListener('fetch', (event) => {
+      const responsePromise = this.handleRequest({event});


Q: Do you need to call waitUntil at all? or does a Promise that resolves to a Response cause the service worker to wait and respond correctly?

The latter.

waitUntil is only needed if there are multiple asynchronous things that are part of separate promise chains going on inside the event handler. When there's just a single promise chain that resolves with the Response, it can be passed directly to respondWith().

gauntface · 2017-08-17T23:30:02Z

packages/workbox-routing/src/lib/router.js

    }
  }

  /**
-   * This method will actually add the fetch event listener.
+   * Checks the incoming even.request against the registered routes, and if


Nit: minor typo event.request

gauntface · 2017-08-17T23:31:06Z

packages/workbox-routing/src/lib/router.js

+   *
+   * @param {FetchEvent} input.event
+   * @param {URL} input.url
+   * @return {Object} Returns an object with `handler` and `params` properties


Nit: Can drop the "set to appropriate values".

👍 Matt's suggestion. If I might suggest one further edit "Returns an object with handler and params properties. They are populated if a matching route was found or undefined otherwise".

Done, with @addyosmani's language.

gauntface · 2017-08-17T23:31:30Z

packages/workbox-routing/src/lib/router.js

+   * @param {URL} input.url
+   * @return {Object} Returns an object with `handler` and `params` properties
+   * set to appropriate values if there was a match, and set to `undefined` if
+   * there was not matching route.


Nit: Change "not" to "no"

gauntface · 2017-08-17T23:33:59Z

packages/workbox-routing/test/sw/router.js

@@ -73,4 +49,32 @@ describe(`Test of the Router class`, function() {
    expect(router._routes.get('GET')).to.have.members([getRoute1]);
    expect(router._routes.get('PUT')).to.have.members([putRoute1]);
  });
+
+  it(`should call self.addEventListener('fetch') when addFetchListener() is called`, function() {
+    const stub = sinon.stub(self.__proto__.__proto__.__proto__, 'addEventListener');


why so many proto?

This seems super strange :) I'm curious about the proto depth here, but imagine there's a good reason.

I've added a comment explaining what's going on. If there's a better approach for stubbing out self.addEventListener() without triggering a mocha global leak warning, I'm definitely open, as I had to bang my head for a while before I was able to get even that to work.

gauntface

Only one proper change - prevent multiple callls to addEventlistener

addyosmani

Approved with a few nits along the way.

addyosmani · 2017-08-18T04:53:15Z

packages/workbox-routing/src/lib/router.js

-   * register a `fetch` handler and respond to requests. This can be set to
-   * `false` in a development environment, to prevent the service worker
-   * from responding with cached responses.
+   * Constructs a new instance, without any registered routes.


Nit: constructs a new Router instance

addyosmani · 2017-08-18T04:54:47Z

packages/workbox-routing/src/lib/router.js

+   * router.registerRoute({route: imageRoute});
+   * router.addFetchListener();
+   */
+  addFetchListener() {


Out of interest, what happens with this implementation if addFetchListener is called multiple times?

addyosmani · 2017-08-18T04:55:25Z

packages/workbox-routing/src/lib/router.js

    }
  }

  /**
-   * This method will actually add the fetch event listener.
+   * Checks the incoming even.request against the registered routes, and if


addyosmani · 2017-08-18T05:01:18Z

packages/workbox-routing/src/lib/router.js

+   *
+   * @param {FetchEvent} input.event
+   * @param {URL} input.url
+   * @return {Object} Returns an object with `handler` and `params` properties


👍 Matt's suggestion. If I might suggest one further edit "Returns an object with handler and params properties. They are populated if a matching route was found or undefined otherwise".

addyosmani · 2017-08-18T05:01:25Z

packages/workbox-routing/src/lib/router.js

+   * @param {URL} input.url
+   * @return {Object} Returns an object with `handler` and `params` properties
+   * set to appropriate values if there was a match, and set to `undefined` if
+   * there was not matching route.


addyosmani · 2017-08-18T05:01:53Z

packages/workbox-routing/test/sw/router.js

@@ -73,4 +49,32 @@ describe(`Test of the Router class`, function() {
    expect(router._routes.get('GET')).to.have.members([getRoute1]);
    expect(router._routes.get('PUT')).to.have.members([putRoute1]);
  });
+
+  it(`should call self.addEventListener('fetch') when addFetchListener() is called`, function() {
+    const stub = sinon.stub(self.__proto__.__proto__.__proto__, 'addEventListener');


This seems super strange :) I'm curious about the proto depth here, but imagine there's a good reason.

jeffposnick · 2017-08-18T14:16:43Z

PTAL.

* Adds JSDocs for the lifecycle methods, and awaits the return of each. (#736) * Move towards using 'workbox' over 'workbox-cli'. (#744) * New library to support Range requests (#710) * WIP. * Still needs JSDocs. * Renaming, bug fixes, JSDoc. * Changed a few comments. * Updated tests to use new bundling infra, allowing us to remove private exports. * Modified a warning. * Rename cacheWillMatch to cachedResponseWillBeUsed (#748) * No longer register a fetch handler for each Router by default (#752) * No longer register a fetch handler for each Router by default. * Review feedback.

No longer register a fetch handler for each Router by default.

dc4b1bb

jeffposnick requested review from addyosmani and gauntface August 17, 2017 21:53

gauntface reviewed Aug 17, 2017

View reviewed changes

gauntface suggested changes Aug 17, 2017

View reviewed changes

addyosmani approved these changes Aug 18, 2017

View reviewed changes

Review feedback.

b5ddcdc

jeffposnick added the Breaking Change Denotes a "major" semver change. label Aug 18, 2017

gauntface approved these changes Aug 18, 2017

View reviewed changes

gauntface merged commit f4f450a into v2 Aug 18, 2017

gauntface deleted the router-addFetchHandler branch August 18, 2017 23:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

No longer register a fetch handler for each Router by default #752

No longer register a fetch handler for each Router by default #752

jeffposnick commented Aug 17, 2017

gauntface Aug 17, 2017

jeffposnick Aug 18, 2017

gauntface Aug 17, 2017

addyosmani Aug 18, 2017

jeffposnick Aug 18, 2017

gauntface Aug 17, 2017

jeffposnick Aug 18, 2017

gauntface Aug 17, 2017

addyosmani Aug 18, 2017

jeffposnick Aug 18, 2017

gauntface Aug 17, 2017

addyosmani Aug 18, 2017

jeffposnick Aug 18, 2017

gauntface Aug 17, 2017

addyosmani Aug 18, 2017

gauntface Aug 17, 2017

addyosmani Aug 18, 2017

jeffposnick Aug 18, 2017

gauntface left a comment

addyosmani left a comment

addyosmani Aug 18, 2017

addyosmani Aug 18, 2017

addyosmani Aug 18, 2017

addyosmani Aug 18, 2017

addyosmani Aug 18, 2017

addyosmani Aug 18, 2017

jeffposnick commented Aug 18, 2017

No longer register a fetch handler for each Router by default #752

No longer register a fetch handler for each Router by default #752

Conversation

jeffposnick commented Aug 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gauntface left a comment

Choose a reason for hiding this comment

addyosmani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeffposnick commented Aug 18, 2017