Re: [w3c/ServiceWorker] Proposal: Allow addEventListener/removeEventListener for 'fetch' to called at anytime (#1195)

I didn't mean to dismiss discussion on requirements, I was just afraid the proposal could get side tracked. I'm happy to discuss requirements more. I wanted to think about this a little bit so sorry for the delayed response. 

> Changing navigation preload into a general preload may solve this particular problem, but it also may solve some other issues, such as speeding up stale-while-revalidate type of behaviors.

A general preload is interesting and potentially useful to us but I think it's add a lot of complexity and doesn't address this particularly problem well:
1) It doesn't allow a running SW to stop handling `fetch` events and avoiding its overhead.
2) It doesn't solve the SW overhead of running the respondWith. The SW could be handling other fetch events in the queue, running GC, fetch response has to be routed through another process leading to extra context switches and scheduling overhead which are costly particularly if the user is CPU bound. The SW overhead will still be felt event even with the general preload response.
3) It will cause preload `fetch` to go out to network for resources that will be clearly cached consuming valuable bandwidth. This is something that HTTP2 push has not managed to solve yet i.e. avoiding pushing HTTP2 resources that are cached. Pages that opt into navigation/document preload will know that their document is not entirely cacheable but for general resources there will be a mix of fully/partially/not cached resources. So you run the risk of causing a net regression by enabling it for all resources. Canceling the preload after the fact would minimize but wouldn't bring that cost to zero.

General preload is useful and something I'd like to explore but I don't think it solves this particular problem well.

I think the root of the problem with my proposal, like you mention, is how to deal with the SW restarting and correctly deciding if a `fetch` handler should be registered or not. Ideally there could be something like this:
```javascript
// At global scope
if (self.fastSyncStorage.get('does_client_want_fetch')) {
   addEventListener('fetch', fetchHandler);
}
```

Now the problem is there's no sync storage API AFAIK and even if there was then this pattern would add a sync dependent disk read which would slow down SW startup. Another option is to have the client always register the fetch handler when the SW is restarted and as soon as async storage returns a read that indicates that the fetch handler is not required then it can be unregistered. This means that any network fetch around the time a SW is restarted will be slower. That's not a good design in my opinion.

`serviceWorker.fetch.disable()` isn't great either even if we also added `enable()`. I think it's bad practice to not deliver a `fetch` event if there's any remaining `fetch` listener(s) that have not been unregistered. It's also a bad API if we ask users to call `serviceWorker.fetch.disable()` rather then `removeEventListener`. I guess we could require that all `fetch` liseners have been unregistered or otherwise `disable()` would warn/fail/throw but that's a bit cumbersome. It also means that all the users of `fetch` have to coordinate to call `disable()` unlike other event listeners where you only have to worry about register/unregistering yourself. 

How about if we, in addition to my proposal, added an API that returns `undefined` on the first run and then true/false on subsequent restarts of the same version, if there was no registered fetch handler `serviceWorker.fetch.wasRegistered` (better naming?) when the SW was shutdown. Then we could suggest the following:
```javascript
// At global scope
if (!serviceWorker.fetch.wasUnregistered) {
   addEventListener('fetch', fetchHandler);
}
```

Implementors would be explicitly allowed to not restart the SW on a `fetch` event when the value of wasUnregistered is true meanings that the SW was shutdown with no `fetch` handler active meaning that `fetch` are not observable until some other events re-registers a `fetch` handler.

This would solve the following:
1) It would let add/removeEventListener work naturally and not require any special throwing logic in addEventListener, solving whatwg/dom#371.
2) It wouldn't temporarily re-register the `fetch` handler on startup until async storage is queried.
3) It would let implementors only pay the overhead of SW startup and `fetch` handler when required.
4) It would let the SW user decide when to pay the `fetch` overhead at any point in the SW life cycle.
4) Doesn't require independent `fetch` listeners to coordinate when they have all un-registered to call `disable()`. (I'm not aware of why you would have multiple independent`fetch` listeners and if there's any other issues around that so that point might be moot point.)


-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/w3c/ServiceWorker/issues/1195#issuecomment-333703638