workbox-precaching rewrite to avoid dependency on activate handler #1820
Conversation
| googleAnalytics: 'googleAnalytics', | ||
| precache: 'precache', | ||
| precache: 'precache-v4', |
There was a problem hiding this comment.
Not crazy about this, but I suppose we need to choose another name, correct?
There was a problem hiding this comment.
Right. Another option is 'precache-v2', implying that it's the second precache version, rather than using the version number associated with Workbox.
Would you prefer that?
There was a problem hiding this comment.
Hmmm, yeah, I think I do like that better. Because most major version bumps will not require a cache name change.
On a similar note, did you test the behavior in the case of someone who's providing their own cache name?
There was a problem hiding this comment.
If you're following the guidance at https://developers.google.com/web/tools/workbox/guides/configure-workbox#configure_cache_names and changing prefix, then you'll automatically get the new 'precache-v2' as part of your cache name.
If you're bypassing that cache name mechanism entirely (which... I'm assuming not too many people are, since you have to use PrecacheController directly vs. using workbox.precaching) then your cache name will not end up changing. As for how bad a problem that is... I think the worst case scenario is ending up with extra entries in the named cache that would never be cleaned up, because the new workbox-precaching code isn't tracking them.
There was a problem hiding this comment.
Cool. Probably worth mentioning in the upgrade notes that anyone in that latter case should probably choose a new name for v4.
| event.waitUntil(cleanupOutdatedCaches(cacheName).then((cachesDeleted) => { | ||
| if (cachesDeleted.length > 0) { | ||
| logger.log(`The following out-of-date precaches were cleaned up ` + | ||
| `automatically ${cachesDeleted}`); |
There was a problem hiding this comment.
Can this be multiple caches? Should we log it as-is rather than converting to a string?
There was a problem hiding this comment.
I'll remove the implicit string conversion.
| /** | ||
| * Call this method from a service work install event to start | ||
| * precaching assets. | ||
| * Call this method from the service worker install event to precache new and |
There was a problem hiding this comment.
I'd keep the method description first and then add the usage recommendation second.
| * @param {Event} [options.event] The install event (if needed). | ||
| * @param {Array<Object>} [options.plugins] Plugins to be used for fetching | ||
| * and caching during install. | ||
| * and caching during install. |
There was a problem hiding this comment.
Nit: these should be indented 4 spaces per Google style. We haven't done it consistently throughout, so maybe it makes sense to keep it consistent within each file, but at some point we should make it consistent for all of Workbox.
There was a problem hiding this comment.
There are enough instances that would need to be changed that I'm keeping as-is. (And our JSDoc linter doesn't complain.)
There was a problem hiding this comment.
SG. We can make them consistent in another PR sometime in the future.
| `file${alreadyPrecachedCount === 1 ? ' is' : 's are'} already cached.`; | ||
| } | ||
|
|
||
| logger.groupCollapsed(message); |
There was a problem hiding this comment.
Should this all be inside a NODE_ENV !== 'production' conditional?
There was a problem hiding this comment.
The code that calls this module is inside a NODE_ENV check.
There was a problem hiding this comment.
But uglify may not be able to figure that out. Can you confirm that it doesn't appear in the minified file?
There was a problem hiding this comment.
http://storage.googleapis.com/workbox-cdn/releases/4.0.0-beta.0/workbox-precaching.prod.js suggests that it's not in the prod bundle, due to what I guess is tree-shaking.
This PR changes some of the logging logic in this file, but shouldn't lead to changes in how things are bundled up.
| * @private | ||
| * @memberof module:workbox-precaching | ||
| */ | ||
| export default function(urlObject, ignoreUrlParametersMatching) { |
There was a problem hiding this comment.
Same comment as above, can we make this a named export?
| @@ -0,0 +1,44 @@ | |||
| /* | |||
| Copyright 2018 Google LLC | |||
| @@ -0,0 +1,98 @@ | |||
| /* | |||
| Copyright 2018 Google LLC | |||
| @@ -0,0 +1,22 @@ | |||
| /* | |||
| Copyright 2018 Google LLC | |||
PR-Bot Size PluginChanged File Sizes
New Files
All File SizesView Table
Workbox Aggregate Size Plugin9.38KB gzip'ed (63% of limit) |
R: @philipwalton
Fixes #1793, fixes #1783
This is a pretty significant rewrite of the
workbox-precachingcore behavior. The goal is to ensure that only cleanup work is done in theactivateevent. Previously, theactivateevent was used to move precachedResponses over from a-temp-cache into the active cache. Now, we just add allResponses directly in the activate cache in theinstallhandler, using an extra URL query parameter to store anyrevisionmetadata that's provided. (This is the same model thatsw-precachepreviously used.)Moving to this model means that we no longer have a dependency on IndexedDB within
workbox-precaching, which should reduce the overall bundle size and lead to what's hopefully less complex code.In addition to that change, there are two new functions exposed on
workbox.precachingas part of this PR:workbox.precaching.getCacheKeyForUrl(url), which will return the key that corresponds to the cache entry forurl. This takes into account the fact that the key might have an extra URL query parameter appended to it. It's useful if you need to manually callcache.match(key)to retrieve a precachedResponse, and you need to know the specifickeyto use.workbox.precaching.cleanupOutdatedCaches()is not called by default, but developers can try it out by explicitly calling it. It will iterate through all of the caches in scope for the current service worker, and attempt to identify and delete "out of date" precaches. Developers upgrading from earlier versions of Workbox to Workbox v4 might find this useful, since we have a breaking change in precache compatibility in v4, and a brand-new precache is created as part of the upgrade. Callingworkbox.precaching.cleanupOutdatedCaches()means that the old, Workbox v1-v3 precached entries will be deleted after the new Workbox v4 service worker activates.