Issue with cacheDidUpdate plugin in workbox-precaching when other callbacks are also used

[budarin]

In my application, I display a splash screen and wait for complete the installation of the service worker and only then display the content, because the service worker must manipulate the csp token for several tabs for all requests in the API.

On a slow connection, the installation of a service worker can take up to a minute. I would like to keep the user busy with the progress of caching elements at this time.

I read #2406 and wrote a custom plugin for cacheDidUpdate:

export const cacheDidUpdate = async ({ cacheName, request, old Response, new Response, event, state }) => {
    const { log } = console;
    log('cacheDidUpdate', cacheName, state);
};

I used it in the service worker

addPlugins([cacheDidUpdate ]);

but I didn't see anything in the console.
What am I doing wrong?

[jeffposnick]

Can you try it with

addPlugins([
  {cacheDidUpdate},
]);

A plugin is an object that has callback functions with properties like cacheDidUpdate. (There might be multiple callbacks in a single plugin object.) Your code looks like it's just passing in a function named cacheDidUpdate as if it were a plugin object.

Hopefully that works. Otherwise, we can reopen the issue./

[budarin]

@jeffposnick

I'm sorry - it was my typo here - in fact, my code is as follows

const myPlugins = {
   handlerDidError,
   cacheDidUpdate
};

addPlugins([myPlugins]);

but the result is the same

[budarin]

I've found the problem:
If I use to

addPlugins([
  {cacheDidUpdate},
]);

Everything works as expected
when I add some plugings

addPlugins([{
    cacheDidUpdate,
    handlerDidError,
    cachedResponseWillBeUsed,
}]);

cacheDidUpdate doesnt work

Here is the code of the other plugins

export const handlerDidError = ({ request }) => {
    logError(request);

    return null;
};
export const cachedResponseWillBeUsed = async ({ cacheName, request, cachedResponse }) => {
    if (cachedResponse) {
        return cachedResponse;
    }

    const cache = await caches.open(cacheName);
    const response = await fetch(request);

    if (!response.ok) {
        logError(request, 'Error loading asset');
        return null;
    }
    await cache.put(request, response.clone());
    return response;
};

[jeffposnick]

Hmmm... let me look into that more closely, as in general, you should be able to use various plugin callbacks together.

I'm glad you have a workaround for the time being, though.

[jeffposnick]

I'm revisiting this, and just to clarify, are you saying that cacheDidUpdate never fires, or that cacheDidUpdate fires sometimes, but not when your cachedResponseWillBeUsed function does the cache.put()?

If you mean the latter, then that's excepted, as cacheDidUpdate only fires as part of a cache update performed by the cachePut() method of a StrategyHandler. It won't fire for arbitrary cache.put() calls that are made elsewhere in the code.

[budarin]

if I use both plugins: cacheDidUpdate and cachedResponseWillBeUsed - cacheDidUpdate never fires

cacheDidUpdate fires only if cachedResponseWillBeUsed is missing

but I need both of them and to have an ability to watch for installation progress and an ability to "repair" app cache

[jeffposnick]

I'm still unclear on exactly what's going on.

Taking a step back, #2858 tracks another request to "repair" precached assets outside of the normal service worker lifecycle.

If we added that functionality in an upcoming release (but only for precache manifest entries that had an integrity property), could that replace your "repair" plugins?

[jeffposnick]

See #2921 for the proposed "repair" logic.

[budarin]

If we added that functionality in an upcoming release (but only for precache manifest entries that had an integrity property), could that replace your "repair" plugins?

This would definitely help us get rid of our "recovery" plugin!

[jeffposnick]

Okay, that PR should be included in the v6.3.0 release. We're looking at pushing that out in about a week.

[jeffposnick]

The precaching repair functionality is now live in https://github.com/GoogleChrome/workbox/releases/tag/v6.3.0

[budarin]

The precaching repair functionality is now live in https://github.com/GoogleChrome/workbox/releases/tag/v6.3.0

Thanks!

[budarin]

@jeffposnick
Unfortunately, I have one option when integrity property does not save -/ root route in the case of SSR.

This is not a static element - it's dynamically generated depending on the browser, for each client, and depending on many other parameters - so it is not possible to use integrity property for it, but this element must be present in the cache and must be "repair" if it is not in the cache

[jeffposnick]

I don't feel comfortable adding in automatic precache repair functionality unless integrity is also being used.

If I had a dynamically generated HTML page that was heavily customized for each client, I don't think I would use precaching for that to begin with, though, since it's probably very difficult to generate a meaningful hash of the contents at build time.

Using a StaleWhileRevalidate, CacheFirst, or even NetworkFirst runtime caching route for those types of documents makes more sense to me. You can ensure that it's added to the cache during installation via a custom install handler.

// Don't include and entry for `'/'` in your precache manifest.
precacheAndRoute(self.__WB_MANIFEST);

self.addEventListener('install', (event) => {
  event.waitUntil(
    caches.open(INDEX_CACHE)
      .then((cache) => cache.add('/'))
  );
});

registerRoute(
  // If you need to support /index.html as well,
  // this criteria would need to be tweaked and you'd
  // want to add a cacheKeyWillBeUsed plugin to normalize.
  ({url}) => url.pathname === '/',
  new StaleWhileRevalidate({
    cacheName: INDEX_CACHE,
  })
);

[budarin]

This solves the problem, but breaks the uniformity and simplicity - a developer will have to be informed about this case and should not forget to exclude the ` / ' route from the manifest and add the code to the worker service.

The solution with integrity, it seems to me, complicates the process - it forces us to write additional code for generating hash (this can be solved by an option in the config, and by the code in the library), although we have either revision or its hash / version is contained in the resource name itself in the manifest

My opinion about restoring pre-cache elements is that they can and should be re-requested uniformly without writing a code and all the information for this is already in the manifest.
In the case of the ' / ' route (and other resources generated dynamically by the server), you should not need to request a resource with integrity info.

[jeffposnick]

This solves the problem, but breaks the uniformity and simplicity - a developer will have to be informed about this case and should not forget to exclude the ` / ' route from the manifest and add the code to the worker service.

All of the Workbox tools that are used to generate a manifest, by default, will look at actual assets on disk (or in a webpack asset pipeline, if you're using webpack), and create a revision parameter based on the actual static contents of what it finds. If the contents of / are generated or customized dynamically, on the server, then it stands to reason that they don't uniquely depend on static assets. I'm not sure how you ended up with an entry for / in your precache manifest, unless you used the templatedURLs option (which generates a compound revision hash from multiple static dependencies), or if you manually added a hard-coded entry for /. In either of those cases, the developer needs to go out of their way to add / to the manifest, so it's not extra work to exclude it.

My opinion about restoring pre-cache elements is that they can and should be re-requested uniformly without writing a code and all the information for this is already in the manifest.
In the case of the ' / ' route (and other resources generated dynamically by the server), you should not need to request a resource with integrity info.

To repeat what I mentioned above—I think the fundamental issue is that you shouldn't have / in your precache manifest, if its contents vary based on a dynamic server response. This is what runtime caching is meant to deal with.

CC: @malchata, who is working on revamping our docs, in case this is something that we can clarify there.

[budarin]

To repeat what I mentioned above—I think the fundamental issue is that you shouldn't have / in your precache manifest, if its contents vary based on a dynamic server response. This is what runtime caching is meant to deal with.

Why not ? I'm sorry, but I don't understand why we can't have / in the precache manifest - it's just a resource that, like other resources, must be in the precache manifest for normal offline operation.

I agree that it is not static and generating a revision for it does not make sense, but it is possible that there may be several such resources in the project and it is possible to add a flag to the manifest configuration that this is a dynamic resource and it just needs to be loaded

If we had an option in the workbox configuration that the resource is dynamic , there would be no need to generate integrity property (and how about the idea of putting integrity generation (if it is still needed) under the hood of the workbox if there is a flag in the configuration?)

[jeffposnick]

Why not ? I'm sorry, but I don't understand why we can't have / in the precache manifest - it's just a resource that, like other resources, must be in the precache manifest for normal offline operation.

It's just that workbox-precaching is not designed to handle the caching lifecycle of this sort of resource. I apologize if it's not clearer in the documentation, and it's obviously possible to make workbox-precaching do things that it's not intended to do, but your service worker is going to be more resilient over all if you go with, and not against, the intended design.

Workbox's solution for caching dynamic resources is to use a runtime route to handle serving what's already in the cache and updating it with the most appropriate strategy based on your knowledge of your architecture. It might be stale-while-revalidate, or network-first, or something else. If you need to guarantee that the resource is in the cache during service worker installation, writing a custom install handler, as in the example above, will give you that guarantee.

If you're asking about how to configure our build tools to generate this sort of runtime caching route, you can do that with the runtimeCaching configuration option.

[budarin]

It's just that workbox-precaching is not designed to handle the caching lifecycle of this sort of resource. I apologize if it's not clearer in the documentation, and it's obviously possible to make workbox-precaching do things that it's not intended to do, but your service worker is going to be more resilient over all if you go with, and not against, the intended design.

The library should simplify the life of a developer. The simplicity of its use lies in a simple declaration of what he would like to pre-cache - and this is a simple list of resources.

Considering your suggestions and objections, the developer should be very well versed in the nuances of the library when working with static and dynamic resources in precache list, although he does not need it and complicates the use of the library.
He would like to declare that the library should cache, and you suggest that he actually return to manually writing the logic of the service worker - then why do we need a library?

I apologize if my statements are harsh, but I think like a library user and in this form I see chaos: in one place I have to use the library with it's declarations, and in other places I have to write native code for the implementation of the service worker. So isn't it easier then to write everything with hands?

the ' / ' route is one of the elements of the precache list - why should I process it separately from the other elements of this list? This only complicates the use of the library and introduces confusion

If you're asking about how to configure our build tools to generate this sort of runtime caching route, you can do that with the runtimeCaching configuration option.

I'm talking about writing by hands integrityManifestTransform for generating integrity property - it should be automated with workbox with an option in config

[jeffposnick]

Thanks for continuing to engage with us here—I do want to figure out exactly how you've approached Workbox, and learn from that had we could better guide folks towards better outcomes in our documentation (or add in new features when appropropriate).

The library should simplify the life of a developer. The simplicity of its use lies in a simple declaration of what he would like to pre-cache - and this is a simple list of resources.

So I wanted to push back a bit here, because while you're able to pass in whatever list of precache entries you want to precacheAndRoute(), the expectation and documented behavior is that updates will only happen if you use a revision property for each entry that reflects the corresponding URL's contents. We give the following warning in the docs: Never hardcode revision info into a "hand written" manifest, as precached URLs will not be kept up to date unless the revision info reflects the URL's contents!

Based on what you're describing, there's an entry for / in your precache list, but I don't understand how its being added, or what its revision property is being set to. Is one of the Workbox build tools generating the entry for /? Or are you adding it in manually outside of the build process, and if so, how are you guaranteeing at build time that its revision parameter uniquely fingerprints the dynamic HTML that's returned from the server?

I apologize if my statements are harsh, but I think like a library user and in this form I see chaos: in one place I have to use the library with it's declarations, and in other places I have to write native code for the implementation of the service worker. So isn't it easier then to write everything with hands?

That's a fair point, and the custom install handler that I described above could be written using the warmStrategyCache() method of workbox-recipes instead. (workbox-recipes is new-ish, and my instinct is sometimes just to go with the underlying service worker API when coming up with quick examples.)

import {warmStrategyCache} from 'workbox-recipes';
import {registerRoute} from 'workbox-routing';
import {StaleWhileRevalidate} from 'workbox-strategies';

const indexStrategy = new StaleWhileRevalidate({cacheName: INDEX_CACHE});

registerRoute(
  ({url}) => url.pathname === '/',
  indexStrategy
);

warmStrategyCache({
  strategy: indexStrategy,
  urls: ['/'],
});

I'm talking about writing by hands integrityManifestTransform for generating integrity property - it should be automated with workbox with an option in config

This goes back to my earlier point, since the same complication around generating a revision property applies to generating a integrity value: I don't see how a theoretical integrityManifestTransform could be written (either by you, or as part of Workbox) that would work in this use case, because the HTML response from the server is dynamic, and you don't know what the SHA-256 hash of it will be ahead of time. That's why I'm strongly suggesting that you shouldn't go out of your way trying to force workbox-precaching to handle that URL, and instead should use runtime caching with the workbox-strategy class that makes the most sense for the freshness guarantees you want.

[budarin]

Ok, I'll tell you how our service worker is arranged and how we would like it: we generate the manifest using the InjectManifest plugin.
In the service worker, we add the ' / ' route to the manifest

const updatedManifest = [
    ...self.__WB_MANIFEST,
    {
        url: '/',
        revision: packageJson.version,
    },
];

Webpack generates us resource names with contenthash. This is quite enough to have a consistent precashe for offline.

Given that other developers may not have the contenthash, you suggest generating an integrity attribute for them.
In order to make it clear to workbox that it is necessary to generate an integrity attribute for static resources under the hood, I would add the generateIntegrityAttribute flag to InjectManifest,

        new InjectManifest({
            swSrc: '...',
            swDest: 'sw.js',
            ...
            generateIntegrityAttribute: true
        }),

a dynamically generated resources (resources without the integrity attribute) I would upload it as it is - they do not require any verification.

Now I hope everything is clear and transparent, and when" repairing" the damaged precache, you can safely download all the resources specified in the manifest.

[jeffposnick]

Thanks for sharing that.

I think we'll add in an addIntegrity configuration option in a future Workbox release, most likely defaulted to false, to make using integrity easier in the future. (Some folks end up loading items in their precache manifest from cross-origin CDNs, and they don't always have CORS enabled, which won't work with subresource integrity, so I don't know that we'll ever default to adding in those attributes.)

Getting back to your specific usage, apart from all other aspects of this discussion, I would recommend that you don't add any entries to the precache manifest that is generated by the build tools unless you're sure that the revision field uniquely identifies the actual content associated with the URL. Based on what you've described, the contents of / are dynamically generated and don't correspond 1:1 to a value known at build time like packageJson.version. Using runtime caching for /, as described above, sounds like a much safer option for your use case. The "repair" step, where the network is consulted when there's a cache miss, is already implemented in runtime caching strategies like NetworkFirst or StaleWhileRevalidate.

I am at a loss for how the Workbox libraries could prevent you from doing what you're doing, so I don't think that this is a problem that could just be solved by improving the developer experience. We currently have warnings enabled if you just manually added an entry with a url and without a revision property, but stopping someone from explicitly passing in an entry that has a revision property which doesn't correspond to the URL's contents is a different problem.

workbox/packages/workbox-precaching/src/PrecacheController.ts

Lines 158 to 169 in 03055e6

	if (urlsToWarnAbout.length > 0) {
	const warningMessage = `Workbox is precaching URLs without revision ` +
	`info: ${urlsToWarnAbout.join(', ')}\nThis is generally NOT safe. ` +
	`Learn more at https://bit.ly/wb-precache`;
	if (process.env.NODE_ENV === 'production') {
	// Use console directly to display this warning without bloating
	// bundle sizes by pulling in all of the logger codebase in prod.
	console.warn(warningMessage);
	} else {
	logger.warn(warningMessage);
	}
	}

[budarin]

Getting back to your specific usage, apart from all other aspects of this discussion, I would recommend that you don't add any entries to the precache manifest that is generated by the build tools unless you're sure that the revision field uniquely identifies the actual content associated with the URL. ...

This approach greatly complicates the life of the developer and disorients beginners - static resources and a /?source=pwa router make up the precache list for the application to work offline. You suggest dividing this list and processing its elements in different ways.

If I were developing a workbox, I would implement such a contract

new InjectManifest({
    swSrc: '...',
    swDest: 'sw.js',
    include: [
       { 
          url: '/?source=pwa',  // static resource is generated by server
          revision: <revision>,
       },
       { 
          url: '/anotherStaticResourceFromServer',
          revision: <revision>,
       },
       { url: '*.(js|json)' }, 
       { url: '*.css' }, 
    ],
    addIntegrity: true
}),

And under the hood I would get the integrity attribute for static resources and for static resources from server (with revision) I would do nothing - simply put them as is into manifest assets array.

Thus, the developer would only have to focus on working with the rest of the resources that are not included in the precache list.

P.S: I want to emphasize once again that in this case /?source=pwa and /anotherStaticResourceFromServer are static resources that must be only generated by the product server with its environment and cannot be generated during the build process

for example, '/?source=pwa' route - is an app-shell for the pwa, which is rendered by the server at the request of the client from components that are build and tested during the build process

[khmyznikov]

@jeffposnick Maybe we should make something like precache routes or runtime precache to cover SSR cases like this. I've got similar questions from the community guys who implement PWA in SSR.

[budarin]

To summarize all of the above - we have two types of precache assets in SSR

• some are available at the build stage in files on a disk
• others are available exclusively in runtime as network static assets as they are generated by the production server and there is no way to get them and their revisions before deploying this build in production

[FluorescentHallucinogen]

@jeffposnick This is a question about DX (developer experience).

From what I can see currently Workbox is more focused on CSR (client-side rendering) PWA.

In CSR PWA the app shell are always files. And these files are static and always available at build time (at service worker generation time). You can easily calculate file checksums (hashes/revisions). And these app shell files are identical for all users.

SSR PWA is a different story. Some of app shell files can be generated in runtime and be different for various users. But they are still a part of app shell. And should be precached (like the rest of static files). The content of these files is unavailable at build time, so you can't calculate checksums. All that developer know is URL.

And as far as I understand, Workbox doesn't actually validate (recalculate) the checksum of files from cache. Right?

The current goal of revision field is to just indicate that some file has changed or not and should be downloaded from network or not. If the revision field is null, the file is always downloads from network.

If so, I like the idea of allowing declaring dynamic routes in the include array since they are part of app shell and should be precached.

[FluorescentHallucinogen]

In other words, the idea is to declare all the things should be precached in one single place — in InjectManifest's include array in declarative way instead of writing imperative custom install handler by hand or using warmStrategyCache and so on in various places. This should improve DX.

[FluorescentHallucinogen]

@budarin

UPD: you can use optional additionalManifestEntries array. According to docs, this is A list of entries to be precached, in addition to any entries that are generated as part of the build configuration.. 😉

[budarin]

@jeffposnick

The issue with the declaration of resources from the server in injectManifest was solved using the configuration property additionalmanifestentries.

new InjectManifest({
    swSrc: '...',
    swDest: 'sw.js',
    include: [
       { url: '*.(js|json)' }, 
       { url: '*.css' }, 
    ],
    additionalManifestEntries: [
       { 
          url: '/?source=pwa', 
          revision: <revision>,
       },
       { 
          url: '/anotherStaticResourceFromServer',
          revision: <revision>,
       },    
    ],
    addIntegrity: true
}),

There is no need to change anything - the plugin parameters have all the possibilities for flexible description of all types of resources

@FluorescentHallucinogen , thank you for your help in solving the issue

[FluorescentHallucinogen]

I think we need to improve Workbox docs by adding samples for additionalManifestEntries use cases and SSR recipes generally.

[jeffposnick]

Hey all—the existence of the additionalManifestEntries configuration option does not mean that it's the correct solution for this use case. Using additionalManifestEntries is no different than hardcoding something like:

const moreEntries = [
  {url: '/?source=pwa', revision: '<somethingHardcoded>'},
  {url: '/anotherStaticResourceFromServer', revision: '<somethingElseHardcoded>'},
];
precacheAndRoute([...self.__WB_MANIFEST, ...moreEntries]);

in your swSrc file's code.

It is really not a good idea to precache any URL whose content might change outside of your build process, i.e. any HTML that's server-rendered. I've done my best to explain upthread why this is the case, and why runtime caching is the right approach. But to summarize, the logic and data sources that your server uses to generate that SSR response may change, and the initially precached entries won't end up updated, because the revision field hardcoded in the service worker script will remain the same.

I'm sorry that the DX might feel a bit different than what you'd prefer, and there is likely some ability to extend the options in our GenerateSW mode so that it could, e.g., call warmStrategyCache for you based on a given configuration option. That is not going to be added to the InjectManifest plugin, though, because as the name suggests, the purpose of that plugin is to generate a precache manifest and then inject it into your swSrc file, replacing the "magic" symbol self.__WB_MANIFEST. When you use InjectManfiest, as opposed to GenerateSW, the expectation is that you're okay with writing the rest of your service worker file by hand.

Finding the right way to explain this nuance in our documentation is definitely a challenge, and again, I apologize that this is not currently clearer. @malchata continues to work on a revamp of all of our existing documentation, and this discussion has informed some of the guidance he'll be writing.

In the meantime, here are some less formal thoughts that I've written up on precaching vs. runtime caching, and one approach to coordinating SSR logic with service worker code.

[budarin]

@jeffposnick

I'm sorry, but you misunderstand the purpose of /?source=pwa - this is the main resource of the precache - this is the pwa-shell, which must be in the precache and not loaded dynamically.

We use it as navigation route

   registerRoute(
       new NavigationRoute(createHandlerBoundToURL('/?source=pwa')),
   );

revision - in our case, not hardcoded, but the version of the application on which the server rendering of this resource depends
It uniquely identifies the content of the resource.

[jeffposnick]

Hello @budarin—if you have a mechanism in place to ensure that the <revision> field of {url: '/?source=pwa', revision: '<revision>'} inside of your deployed service worker code is updated whenever the server-rendered response for /?source=pwa might change, then 👍 . It's what I'm suggesting you'd need to have in order for updates to work as expected.

(Going back to what the original question was about, the precaching "repair" can't be handled by Workbox unless there's also an integrity field, though.)

[budarin]

Going back to what the original question was about, the precaching "repair" can't be handled by Workbox unless there's also an integrity field, though.

Unfortunately, it is not possible to get the integrity value at the build stage.
I think that in our case revision is enough - because it uniquely identifies the content of this resource

now the process for tasks that used to be simple has become very complicated - what I used to do without even thinking about it now looks either very difficult or not possible :(

PS: I believe that using integrity checks during repair should be used only when the enableintegrity flag is enabled in the configuration otherwise rely on the resource name + revision

[jeffposnick]

In the interest of closing out this issue, I've revisited the original complaint, which is that when you add your cachedResponseWillBeUsed plugin to workbox-precaching, your cacheDidUpdate plugin isn't called anymore.

I went through a reproduction steps, and identified the following issue: your cachedResponseWillBeUsed plugin executes its "cache repair" logic any time the cachedResponse parameter is undefined.

During the install event, when processing a precache entry that's never been seen before, cachedResponseWillBeUsed will be passed an undefined cachedResponse, but this isn't an error condition—it's the expected behavior. Therefore, you should just return early from your cachedResponseWillBeUsed plugin if its called during install. This will allow your cacheDidUpdate to get called once workbox-precaching adds the new entry to the cache—as opposed to your "cache repair" logic adding the entry to the cache.

In other words, your code should change to:

const cachedResponseWillBeUsed = async ({cacheName, request, cachedResponse, event}) => {
  // Return early if cachedResponse is already set, or if this is an install event,
  // where cachedResponse being undefined is not an error.
  if (cachedResponse || event.type === 'install') {
    return cachedResponse;
  }

  // Your "cache repair" code goes here.
}

Hopefully this change in your cachedResponseWillBeUsed logic unblocks you from implementing the specific logic you believe is needed in your service worker.

I don't have much more to add beyond what I've already mentioned upthread regarding the other topics.

[budarin]

@jeffposnick thanks!

[jeffposnick]

(I've updated the code sample to use event.type === 'install' instead of event instanceof InstallEvent, since the latter apparently won't work in Safari, where InstallEvent is not defined in the ServiceWorkerGlobalScope.)