Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposal: workbox-messages module #1848

Closed
jeffposnick opened this issue Jan 16, 2019 · 8 comments
Closed

Proposal: workbox-messages module #1848

jeffposnick opened this issue Jan 16, 2019 · 8 comments
Labels
Discuss An open question, where input from the community would be appreciated. New Project Idea Ideas for a new, standalone module.

Comments

@jeffposnick
Copy link
Contributor

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

_initWindowReadyDeferreds() {
// A mapping between navigation events and their deferreds.
this._navigationEventsDeferreds = new Map();
// The message listener needs to be added in the initial run of the
// service worker, but since we don't actually need to be listening for
// messages until the cache updates, we only invoke the callback if set.
self.addEventListener('message', (event) => {
if (event.data.type === 'WINDOW_READY' &&
event.data.meta === 'workbox-window' &&
this._navigationEventsDeferreds.size > 0) {
if (process.env.NODE_ENV !== 'production') {
logger.debug(`Received WINDOW_READY event: `, event);
}
// Resolve any pending deferreds.
for (const deferred of this._navigationEventsDeferreds.values()) {
deferred.resolve();
}
this._navigationEventsDeferreds.clear();
}
});
}
}

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: MessageEvent within the SW global should have waitUntil() w3c/ServiceWorker#669 (comment))

  • 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 jeffposnick added Discuss An open question, where input from the community would be appreciated. workbox-core labels Jan 16, 2019
@philipwalton
Copy link
Member

philipwalton commented Jan 16, 2019

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
Copy link
Member

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?

@jeffposnick
Copy link
Contributor Author

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
Copy link
Contributor Author

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 jeffposnick added the New Project Idea Ideas for a new, standalone module. label Jan 16, 2019
@jeffposnick jeffposnick changed the title Add a common 'message' event listener to workbox.core Jan 16, 2019
@philipwalton
Copy link
Member

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().

@jeffposnick
Copy link
Contributor Author

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:

notifyIfUpdated({oldResponse, newResponse, url, cacheName, event}) {
if (!responsesAreSame(oldResponse, newResponse, this._headersToCheck)) {
if (process.env.NODE_ENV !== 'production') {
logger.log(`Newer response found (and cached) for:`, url);
}
const sendUpdate = async () => {
// In the case of a navigation request, the requesting page will likely
// not have loaded its JavaScript in time to recevied the update
// notification, so we defer it until ready (or we timeout waiting).
if (event && event.request.mode === 'navigate') {
if (process.env.NODE_ENV !== 'production') {
logger.debug(`Original request was a navigation request, ` +
`waiting for a ready message from the window`, event.request);
}
await this._windowReadyOrTimeout(event);
}
await broadcastUpdate({channel: this._getChannel(), cacheName, url});
};
// Send the update and ensure the SW stays alive until it's sent.
const done = sendUpdate();
if (event) {
try {
event.waitUntil(done);
} catch (error) {
if (process.env.NODE_ENV !== 'production') {
logger.warn(`Unable to ensure service worker stays alive ` +
`when broadcasting cache update for ` +
`${getFriendlyURL(event.request.url)}'.`);
}
}
}
return done;
}
}

@philipwalton
Copy link
Member

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.

@tomayac
Copy link
Member

tomayac commented Apr 25, 2024

Hi there,

Workbox is moving to a new engineering team within Google. As part of this move, we're declaring a partial bug bankruptcy to allow the new team to start fresh. We realize this isn't optimal, but realistically, this is the only way we see it working. For transparency, here're the criteria we applied:

Thanks, and we hope for your understanding!
The Workbox team

@tomayac tomayac closed this as completed Apr 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discuss An open question, where input from the community would be appreciated. New Project Idea Ideas for a new, standalone module.
3 participants