workbox icon indicating copy to clipboard operation
workbox copied to clipboard

Proposal: workbox-messages module

Open jeffposnick opened this issue 6 years ago • 7 comments

To accompany the new workbox-window library in v4, and standardize the functionality that's needed for the skipWaiting message handler proposed in #1753 as well as what's already implemented in https://github.com/GoogleChrome/workbox/blob/208850c1a670d0f7ec1b8647c0d7ce4319f9fa5d/packages/workbox-broadcast-cache-update/BroadcastCacheUpdate.mjs#L185-L207

I'm proposing a new set of methods on workbox.core:

  • registerMessageCallback({messageType, callback}) will take care of adding in a message event handler (if one doesn't already exist) that will in turn check for incoming messages whose event.data.type === messageType, and when found, run await callback(event) for each registered event. The callback mechanism will be wrapped in event.waitUntil() (for browsers that support it: https://github.com/w3c/ServiceWorker/issues/669#issuecomment-402779578)

  • unregisterMessageCallback({messageType, callback}) will do the opposite, and either unregister a specific callback, or if callback is null, unregister all callbacks for the given messageType.

I think that keeping track of messageType values can be done on a per-module basis, i.e. the constant used by workbox-broadcast-cache-update can be defined locally there.

One additional area for discussion is whether this proposal can end up powering the mechanism that's already in place in workbox.core for registering callbacks that can be run when there's a global QUOTA_EXCEEDED error. There's a lot of similarities, but invoking callbacks triggered by an exception is different from invoking callbacks triggered by a message event.

jeffposnick avatar Jan 16 '19 19:01 jeffposnick

This proposal sounds great to me. And I think it definitely makes sense for the message registrations to be handled by the individual packages, not all grouped in workbox-core.

workbox-window has a small EventTargetShim class that we could move into workbox-core. It might be easier to use this since message events are required to be added in the initial execution phase of the SW, whereas here we'd want the flexibility to add/remove listeners in callbacks.

philipwalton avatar Jan 16 '19 20:01 philipwalton

As for the format, I think something like this would be fine:

{
  type: 'SKIP_WAITING',
  meta: 'workbox-package-name',
}

We could potentially also try to standardize on using verbs for action messages and nouns for notice message, or something like that. I believe right now our only message instances are:

{
  type: 'WINDOW_READY',
  meta: 'workbox-window',
  // payload: void
}
{
  type: 'CACHE_UPDATED',
  meta: 'workbox-broadcast-cache-update',
  payload: {
    cacheName: 'the-cache-name',
    updatedURL: 'https://example.com/',
  },
}

Am I missing anything?

philipwalton avatar Jan 16 '19 20:01 philipwalton

Oh, yeah, I forgot that adding in a message event listener post-install would break, just like the other event listeners. I'll take that into account,

jeffposnick avatar Jan 16 '19 20:01 jeffposnick

I did a quick prototype and it looks like adding in this functionality would take workbox-core.prod.js from 5513 to 5924 bytes. Given that workbox-core is always loaded, those extra bytes might not be worth it for optional functionality.

This logic could live in a new workbox-messages module instead.

jeffposnick avatar Jan 16 '19 21:01 jeffposnick

I did a quick prototype and it looks like adding in this functionality would take workbox-core.prod.js from 5513 to 5924 bytes. Given that workbox-core is always loaded, those extra bytes might not be worth it for optional functionality.

In the CDN case, yes, but in the modules case it would only be loaded if a module that consumed it were loaded.

This is also true of things like the IDB wrapper, which currently gets loaded for CDN users even if they're only using precaching (or any other module that doesn't use IDB).

My preference would be to keep workbox-core as the single place for shared code, but I guess I can also see compelling reasons not to do that if the vast majority of our users consume workbox via importScripts().

philipwalton avatar Jan 16 '19 21:01 philipwalton

Moving over some thoughts from a separate conversation: if we had this module, it would be a good home for code that currently lives in workbox-broadcast-cache-update: https://github.com/GoogleChrome/workbox/blob/208850c1a670d0f7ec1b8647c0d7ce4319f9fa5d/packages/workbox-broadcast-cache-update/BroadcastCacheUpdate.mjs#L91-L127

jeffposnick avatar Jan 18 '19 18:01 jeffposnick

Yesterday I discovered that the SW spec includes a buffering mechanism (client message queue) for when a SW calls postMessage() on a window client. The default behavior is messages won't be dispatched on the window until after DOMContentLoaded.

Unfortunately, this behavior isn't yet implement in all browsers, so for the moment we'll have to continue using this WINDOW_READY strategy, but whatever we do here should be forward compatible enough that we can remove this code at some point.

However, the buffering behavior applies to postMessage() calls but not BroadcastChannel messages. This, coupled with the fact that BroadcastChannel is not even implemented in all browsers makes me wonder if we should switch to using postMessage exclusively.

philipwalton avatar Jan 20 '19 20:01 philipwalton