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

Feature/recipes precache and normalize #2718

Merged

Conversation

Snugug
Copy link
Contributor

@Snugug Snugug commented Jan 8, 2021

R: @jeffposnick @philipwalton

  • Adds a new Workbox Recipe: warmStrategyCache, which takes an array of paths and a Workbox strategy and warms that strategy's cache with those paths at service worker install time.
  • Adds warmCache option to page, image, and static resource recipes to allow users to warm those caches
  • Adds plugins option to page, image, and static resource recipes to allow users to pass additional plugins to those recipes, allowing a user, for instance, to add a URL normalization plugin to the page recipe
  • Removes dependency on precaching from offlineFallback recipe. If precaching is used, it will still attempt to pull from there, but if not, it will pull from its own cache.

Snugug added 4 commits January 8, 2021 15:21
Takes an array of items and a Workbox Strategy and warms the strategy’s cache with those items at install. Useful for getting specific items into cache but allowing them to be updated outside of the Service Worker install cycle
Give users of page/image/resource recipes the ability to warm those caches using warmStrategyCache functionality. Also allows them to include plugins if desired (for instance, to normalize URLs coming into the cache)
While precache will still work, and that will be tried first, the files will be saved in a workbox-offline-fallbacks cache and the cache will try to be retrieved from there if precache fails.
@Snugug Snugug requested a review from jeffposnick January 8, 2021 20:33
@Snugug
Copy link
Contributor Author

Snugug commented Jan 8, 2021

I'm not sure what the error in the Mac test suite is, and why the Windows one didn't fail. If someone can point me to what I need to fix I'd be more than happy to fix it.

Copy link
Contributor

@jeffposnick jeffposnick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

I still need to give some of the modified files a more complete review, but here's some feedback on the new file.

packages/workbox-recipes/src/warmStrategyCache.ts Outdated Show resolved Hide resolved
*/
function warmStrategyCache(options: WarmStrategyCacheOptions): void {
self.addEventListener('install', event => {
const warm = options.paths.map(path => options.strategy.handleAll({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think done (or donePromises) would be a more descriptive variable name than warm, especially since it's not entirely obvious from reading the code that the [1] index represents a done promise.

import './_version.js';

export interface WarmStrategyCacheOptions {
paths: Array<string>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you switch from calling this paths to urls?

(As far as I can tell, you should be able to pass in any absolute or relative URL inside this array, right?)

Snugug and others added 3 commits January 14, 2021 15:59
Co-authored-by: Jeffrey Posnick <jeffy@google.com>
Improves clarity of each, pre PR review request
@Snugug
Copy link
Contributor Author

Snugug commented Jan 15, 2021

@jeffposnick Looks like test are passing now. Here's the Workbox Recipes docs update to go along w/this
google/WebFundamentals#9210

@jeffposnick jeffposnick merged commit 3151c3a into GoogleChrome:v6 Jan 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants