Sw lib caching strategies #108

gauntface · 2017-01-06T22:49:47Z

This adds caching strategies to sw-lib as well as adding some docs.

addyosmani

Left some minor comments.

addyosmani · 2017-01-09T16:48:02Z

packages/sw-lib/src/index.js

+ * importScripts('/<Path to Module>/build/sw-lib.min.js');
+ *
+ * goog.swlib.cacheRevisionedAssets([
+ *     '/styles/main.1234.css',


I think what you're trying to show here is that cacheRevisionedAssets accepts both filename paths with revisions in there or an object with the raw path and a separate revision number. Is that the case?

It might be worth splitting these into two examples (one showing caching a few files with one approach (e.g `/styles/main.1234.css', '/js/main.1234.js' etc) and then the object variation. Might make it more easy to read through.

addyosmani · 2017-01-09T16:48:19Z

packages/sw-lib/src/index.js

+ *     '/scripts/main.js',
+ *     new Request('/images/logo.png')
+ * ]);
+ *


Same comment as above.

addyosmani · 2017-01-09T16:50:41Z

packages/sw-lib/test/browser-unit/sw-unit/caching-strategies.js

+});
+
+describe('Test swlib.cacheOnly', function() {
+  it('should be accessible goog.swlib.cacheOnly', function() {


We talked about why these functions aren't just using arrow syntax offline. I think your reason about Mocha breaking decorated it bindings etc if arrows are used was fine. I did see https://github.com/skozin/arrow-mocha in case we wanted to explore how to work around this in future.

I'm probably going to stick with the function name for now. Ideally Mocha would change to have a more explicit approach of declaring the timeout and retries of a unit test.

gauntface · 2017-01-09T19:19:56Z

@addyosmani happy for this to land?

jeffposnick · 2017-01-09T19:24:13Z

I'm taking a look right now, too.

jeffposnick · 2017-01-09T19:24:57Z

packages/sw-lib/src/index.js

+ * ]);
+ *
+ * // If you have assets that aren't revisioned, you can cache them
+ * // during the installation of you service work using warmRuntimeCache()


"service work" ➡️ "service worker"

jeffposnick · 2017-01-09T19:28:04Z

packages/sw-lib/src/index.js

+ *     }
+ * ]);
+ *
+ * // If you have assets that aren't revisioned, you can cache them


Suggested rephrasing:

If you have assets that aren't revisioned, but which you plan to reference using a runtime caching strategy, you can add them to your runtime cache

At the moment the precaching module doesn't configure fetch routes, so technically both require "run time caching strategies".

jeffposnick · 2017-01-09T19:37:40Z

packages/sw-lib/src/lib/sw-lib.js

   */
  constructor() {
    this._router = new RouterWrapper();
    this._precacheManager = new PrecacheManager();
+    this._strategies = {


When we talked about this last week, I misunderstood and thought that your plan was to export the class definitions—basically, aliasing them under the sw-lib module namespace. I didn't realize that your suggestion was to create instances of each class and include those in each SWLib instance.

These strategies are most useful when you have some control over their behavior via the RequestWrapper class, but there's no way to configure these instances. The default behavior will work, but in the back of my head I worry that people won't realize that they can do things like change the cache name or configure cache expiration if they're only familiar with using these unconfigured instances.

Do you all share those concerns?

Yes and no.

This API matches that of sw-toolbox, that's all I was aiming for with this as it does cover a simple use case.

There are three 1 options:

This approach and expect developers to use the smaller modules is desired (not ideal).

This approach + expose the classes as well, i.e. allow new goog.swlib.strategies.<Name of Strategy>(). Simple use case plus allows configuration.

Just include the class strategies. Only con is that is requires some boilerplate for basic use.

The sw-toolbox-y way of doing this was to pass in the configuration as the third parameter.

I guess we could take inspiration from that and allow developers to pass in a RequestWrapper as an optional third parameter.

For that approach to work, the second parameter should be the an uninstantiated reference to a sw-runtime-caching class, and then SWLib would construct the class at runtime, passing along the RequestWrapper to the constructor if it's provided.

In toolbox I would always go:

toolbox.router.all('/', toolbox.fastest);

The options generally would be global rather than per route.

Would it be worth me removing the strategies from this PR, landing the doc changes and then discussing the strategies in an issue?

Yep, yanking them out of this PR and discussing the approach off-thread would make sense.

There are lots of common use cases for per-route configs that we'd need to support, like one route that used a dedicated image cache with a specific cache expiration policy, and a different route that used a dedicated api-response cache with a different policy.

jeffposnick · 2017-01-09T19:39:51Z

packages/sw-lib/src/lib/sw-lib.js

-   * aren't revisioned, you can cache them here.
-   * @param {Array<String>} unrevisionedFiles A set of urls to cache when the
-   * service worker is installed.
+   * Any assets you wish to cache which can't be revisioned should be


How about:

Any assets you wish to cache ahead of time which can't be revisioned should be...

Since in many of the cases for unversioned resources, just populating the caches via a runtime caching strategy is desirable.

gauntface · 2017-01-09T22:38:03Z

@jeffposnick the strategies are now removed - any further changes you'd like to comments?

jeffposnick · 2017-01-10T14:53:18Z

There's a few extra bits to remove to make the linting happy, but once Travis is green, feel free to merge.

/home/travis/build/GoogleChrome/sw-helpers/packages/sw-lib/src/lib/sw-lib.js
  22:3   error  'CacheFirst' is defined but never used            no-unused-vars
  22:15  error  'CacheOnly' is defined but never used             no-unused-vars
  22:26  error  'NetworkFirst' is defined but never used          no-unused-vars
  22:40  error  'NetworkOnly' is defined but never used           no-unused-vars
  22:53  error  'StaleWhileRevalidate' is defined but never used  no-unused-vars

Matt Gaunt added 2 commits January 6, 2017 14:03

Adding docs and cache strategies

7ad1b69

Changing sw-cli name

a245226

addyosmani reviewed Jan 9, 2017

View reviewed changes

Fixing PR comments

0f490e1

jeffposnick suggested changes Jan 9, 2017

View reviewed changes

Matt Gaunt added 3 commits January 9, 2017 13:41

Changing comments

6b7ea24

Minor tweaks to comments

9e11ac2

Removing strategies

fe2613b

jeffposnick approved these changes Jan 10, 2017

View reviewed changes

Matt Gaunt added 4 commits January 10, 2017 08:12

Fixing lint errors

1a75be4

Removing tests that are no longer relevant

efbf3e8

Adding better logging for failing tests

73bd04f

undeleting router code

f1396cb

gauntface merged commit ed29d60 into master Jan 10, 2017

gauntface deleted the sw-lib-caching-strategies branch January 10, 2017 18:28

snyk-bot mentioned this pull request May 30, 2021

[Snyk] Security upgrade meow from 3.7.0 to 6.0.0 Jeremip11/workbox#9

Open

thexdesk mentioned this pull request Nov 27, 2023

[Snyk] Fix for 6 vulnerabilities thexdesk/workbox#27

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sw lib caching strategies #108

Sw lib caching strategies #108

gauntface commented Jan 6, 2017

addyosmani left a comment

addyosmani Jan 9, 2017

gauntface Jan 9, 2017

addyosmani Jan 9, 2017

gauntface Jan 9, 2017

addyosmani Jan 9, 2017

gauntface Jan 9, 2017

gauntface commented Jan 9, 2017

jeffposnick commented Jan 9, 2017

jeffposnick Jan 9, 2017

gauntface Jan 9, 2017

jeffposnick Jan 9, 2017

gauntface Jan 9, 2017

jeffposnick Jan 9, 2017

gauntface Jan 9, 2017

jeffposnick Jan 9, 2017

gauntface Jan 9, 2017

jeffposnick Jan 9, 2017

jeffposnick Jan 9, 2017

gauntface Jan 9, 2017

gauntface commented Jan 9, 2017

jeffposnick commented Jan 10, 2017

Sw lib caching strategies #108

Sw lib caching strategies #108

Conversation

gauntface commented Jan 6, 2017

addyosmani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gauntface commented Jan 9, 2017

jeffposnick commented Jan 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gauntface commented Jan 9, 2017

jeffposnick commented Jan 10, 2017