Router.handleRequest() Proposal

[gauntface]

At the moment the Router is fairly all encompassing meaning that if developers wanted to use the routing logic with logic before and / or after the routers log, they couldn't.

NOTE: This is largely a suggestion from @jakearchibalds proposal from #700 .

Add explicit "handleRequest()" method to Router class

At the moment the Router class itself adds a fetch handler.

We should alter it to take a similar approach where the "instance" of the module (i.e. the friendlier / 90% use case of the classes) creates the fetch handler and under the hood it calls a "handleRequest()" method.

So instead of in the Router code:

const router = new Router();
router.registerRoute(...);

We'd have to do the following when using the Router directly:

const router = new Router();
router.registerRoute(...);

self.addEventListener('fetch', (event) => {
  event.waitUntil(async () => {
      const response = await router.handleRequest(event);
      if (response) {
          await event.respondWith(response);
      }
  });
});

The end result for most users is that they'd use an instance of the router module that would handle this on behalf of the developer (i.e. they'd never write this code).

Pros:

• Provides flexibility to developers who want to manipulate requests before or after router behaviour
• Matches the precache model in terms of the lowest level code creates no events.

Cons:

• Unclear of the immediate use cases, but can imagine scenarios where other libraries could be used with Workbox by being able to manipulate requests / responses around the router.

[surma]

Note that you can still have a convenience function to have the “old” behavior:

const router = new Router();
router.registerFetchHandler();

The point is to give developers a choice.

[jakearchibald]

I think handleRequest would need to take the whole fetch event. It might need the extra data there, but also it may need to call waitUntil.

[jeffposnick]

A few thoughts:

• We currently support a handleFetch boolean option that can be passed through when constructing a new Router that defaults to true, but can be set to false if you don't want to install a fetch handler. The intended use case is doing something like const handleFetch = process.env.NODE_ENV === 'production' so that you could automatically disable service worker interference on a development server, but it also could satisfy your use case. My vote would be to stick with the handleFetch option, defaulting to true, and then exposing an additional handleRequest() interface that could be called explicitly from your own fetch handler if you choose to disable the implicit handler.

• That aside, I'd argue that using the Router class isn't the best fit for this use case. Instead of trying to use a neutered Router, you can instead use a Route and Handler subclass directly from your own fetch handler, accomplishing the same thing:

E.g. Instead of

const router = new Router();
const handler = new CacheFirst({cacheName: 'images'});
const route = new RegExpRoute(/\.jpg$/, handler);
router.registerRoute({route});

self.addEventListener('fetch', (event) => {
  // Oops, I don't have access to the request/responses for .jpgs here.
});

you could just not use the Router:

const handler = new CacheFirst({cacheName: 'images'});
const route = new RegExpRoute(/\.jpg$/, handler);

self.addEventListener('fetch', (event) => {
  // Create a URL object to pass in to route.match()
  const url = new URL(event.request.url);
  event.waitUntil(async () => {
      let response;
      if (route.match({event, url}) {
        response = await route.handle({event});
      }

      // You can now do whatever you want with the response before it's used.
      if (response) {
        await event.respondWith(response);
      }
  });
});

or even just use the Handler without using Route as well if the predicate is simple:

const handler = new CacheFirst({cacheName: 'images'});

self.addEventListener('fetch', (event) => {
  event.waitUntil(async () => {
      let response;
      if (event.request.url.endsWith('.jpg')) {
        response = await handler.handle({event});
      }

      // You can now do whatever you want with the response before it's used.
      if (response) {
        await event.respondWith(response);
      }
  });
});

Does that address your use case? It's not something that we emphasize in the workbox-sw docs, but it's fully supported for folks who just use the workbox-routing or workbox-runtime-caching libraries directly.

[jakearchibald]

respondWith still needs to be called synchronously, but I get what you mean. Being able to put code either side of route handling is definitely a plus.

[gauntface]

At a high level the lower level modules should be explicit and magic should be handled by workbox-sw.

With that in mind, we should remove the handleFetch option and make it so that given a request, we run it through our router (with any extra's that are set up) and then resolve to the response.

Using Responses or Handlers directly feels like - just don't use Workbox, yet both @surma and @jakearchibald seem to want to use these pieces (i.e. see value in it) just the API isn't allowing them to use it comfortably.

@jeffposnick how do you think this "neuters" the Router class?

@jakearchibald what extra data is in the FetchEvent that might be needed? I'm probably missing something simple here.

[jakearchibald]

what extra data is in the FetchEvent that might be needed?

The various client IDs. We don't implement them yet, but we will at some point. If you want to reimplement something like appcache, you need to know which page is making the request, and if it's a navigation, which page it'll replace.

[gauntface]

Got you. Not useful right now necessarily, but will be in the future.

Only fear - curious what you think - what happens if a developer calls event.respondWith() or other methods that mess with event for future handlers?

[jakearchibald]

I create an object that's FetchEvent-like, but not quite https://github.com/jakearchibald/sw-router/blob/master/src/index.js#L5

[gauntface]

Ignore me - we pass the FetchEvent - corrected my shiz.

[jeffposnick]

@jeffposnick how do you think this "neuters" the Router class?

Some of the value of the Router class right now is that it will respond to events. (I know that not everyone finds this valuable.) This is something that some folks might expect, and by "neutered", I meant disabling that functionality.

While my preference is to continue to support the handleFetch toggle for at least optionally enabling the fetch handler inside of the Router class (even if it defaults to false), I'll defer to consensus.

I think we're agreed that users of workbox-sw will get a fetch handler by default, via whatever mechanism makes the most sense.

Using Responses or Handlers directly feels like - just don't use Workbox, yet both @surma and @jakearchibald seem to want to use these pieces (i.e. see value in it) just the API isn't allowing them to use it comfortably.

Could you clarify what you mean by that? The alternatives that I presented—using the Route and/or Handler classes, but not using Router—are examples of how you could accomplish what they were trying to do while still using Workbox v1.

[gauntface]

Router handles the linking of Routes and Handlers, removing that functionality feels like an awful lot to lose reducing to little win. Plus the Router has the default and catch handlers which would be lost by Routes and Handlers alone.

Keeping in workbox-sw is fine, I see that as a the easiest path, but I would like the lower-levels to be the most flexible. I'd personally prefer replacing the handleFetch constructor arg to a method on the class like Surma suggested and just have workbox-sw use that.

This to me is the best of all worlds.

[jeffposnick]

Proposal based on some of the discussion we've had:

Router changes:

• The Router class will no longer register a fetch handler by default.
• The Router constructor will no longer take in a handleFetch boolean parameter.
• Instead, the Router class will expose a new addFetchListener() method that needs to be called explicitly in order to register a fetch handler. It will return a boolean: true the first time it's called and the handler is registered, and false if it's called multiple times for the same Router.

WorkboxSW changes:

• Nothing changes in the public interface or default functionality. The handleFetch constructor parameter is still supported, and still defaults to true.
• Under the hood, it will explicitly call router.handleFetch() to enable the fetch handler.

@surma @jakearchibald does that sound okay to you? We've got a v2.0.0 planned for next Wednesday, and we should be able to sneak this breaking change into that if so.

[gauntface]

@jeffposnick I preferred Surma's suggestion of registerFetchHandler() and it doesn't need to through an error on the second call, could just have a boolean and return. We could log a warning to callout it's not needed.

Otherwise LGTM

[jeffposnick]

Updated my proposal based on that.

[gauntface]

For workbox-precaching, should we add registerInstallHandler(), registerActivateHandler() and registerFetchHandler() methods as well and pull any logic out of workbox-sw?

We can then seperate the skipWaiting and clientsClaim out into seperate install and activate events in workbox-sw.

[jeffposnick]

I've moved the workbox-precaching discussion to #747

[gauntface]

From the now moved over issue:

Thinking about this, should we use the term handler here given that we have a "handler" notion already?

[jeffposnick]

That's a good point. How about Router.registerFetchListener()?

[gauntface]

+1 registerFetchListener.

[jakearchibald]

I know bikeshedding is cheap, but I used addFetchListener in https://github.com/jakearchibald/sw-router because it's addEventListener, not registerEventListener.

[jeffposnick]

addFetchListener() sounds 👍 to me.

I'll aim for a PR soon to get this into the v2 release.

[jakearchibald]

I wrote the docs for https://github.com/jakearchibald/sw-routes in case we ever want to consider a composable API. I'm not going to publish it or anything, was just exploring the types of patterns that could be done declaratively.