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

workbox-strategies will "auto" import cachable-response and broadcast-cache-update #1071

Closed
gauntface opened this issue Nov 27, 2017 · 2 comments
Labels

Comments

@gauntface
Copy link

Due to the way strategies will attempt to convert configuration into a plugin and how Rollup builds the final bundle, using a strategy will auto import the workbox plugins regardless of whether they are used or not.

Only way around this is to require developers to use the plugins directly, which would unless make it a bit clearer that Workbox has some notion of plugin support.

Thoughts?

@gauntface
Copy link
Author

To be specific, these are the versions:

Today, developers can do:

workbox.strategies.staleWhileRevalidate({
  cacheName: 'hello-world',
  cacheExpiration: {
    ...
  },
  broadcastCacheUpdate: {
    ...
  },
  cacheableResponse: {
    ...
  }
});

Regardless of the configuration, this will pull in the workbox-cache-expiration, workbox-cacheable-response and workbox-broadcast-cache-update modules, meaning the first time a developer runs workbox.strategies it'll actually be pulling three modules.

I'd like to undo this and instead require explicit plugins:

workbox.strategies.staleWhileRevalidate({
  cacheName: 'example',
  plugins: [
    new workbox.expiration.CacheExpirationPlugin({ ... }),
    new workbox.broadcastUpdate.BroadcastCacheUpdatePlugin({ ... }),
    new workbox.cacheableResponse.CacheableResponsePlugin({ ... }),
  ]
});

This removes the dependency between strategies and the three other modules and makes it more explicit on the developer.

We can do more to naming if we felt it would make the API more pallet-able:

workbox.strategies.staleWhileRevalidate({
  cacheName: 'example',
  plugins: [
    new workbox.expiration.Plugin({ ... }),
    new workbox.broadcastUpdate.Plugin({ ... }),
    new workbox.cacheableResponse.Plugin({ ... }),
  ]
});

or:

workbox.strategies.staleWhileRevalidate({
  cacheName: 'example',
  plugins: [
    workbox.expiration.plugin({ ... }),
    workbox.broadcastUpdate.plugin({ ... }),
    workbox.cacheableResponse.plugin({ ... }),
  ]
});

@jeffposnick jeffposnick added the Breaking Change Denotes a "major" semver change. label Nov 30, 2017
@jeffposnick
Copy link
Contributor

Thanks for investigating the options.

I am on board with the second proposal,

workbox.strategies.staleWhileRevalidate({
  cacheName: 'example',
  plugins: [
    new workbox.cacheExpiration.Plugin({ ... }),
    new workbox.broadcastUpdate.Plugin({ ... }),
    new workbox.cacheableResponse.Plugin({ ... }),
  ]
});

being the runtime syntax.

On a positive note, folks using the runtimeCaching config options along with workbox-build.generateSW() to create their service worker can continue to use the previous syntax, and we can just adjust runtime-caching-converter.js to spit out the modified code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants