Sw lib caching strategies #108

Merged
merged 10 commits into from Jan 10, 2017

Projects

None yet

3 participants

@gauntface
Contributor

R: @jeffposnick @addyosmani

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

gauntface added some commits Jan 6, 2017
@gauntface gauntface Adding docs and cache strategies
7ad1b69
@gauntface gauntface Changing sw-cli name
a245226
@addyosmani

Left some minor comments.

packages/sw-lib/src/index.js
+ * importScripts('/<Path to Module>/build/sw-lib.min.js');
+ *
+ * goog.swlib.cacheRevisionedAssets([
+ * '/styles/main.1234.css',
@addyosmani
addyosmani Jan 9, 2017 Member

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.

@gauntface
gauntface Jan 9, 2017 Contributor

Done.

packages/sw-lib/src/index.js
+ * '/scripts/main.js',
+ * new Request('/images/logo.png')
+ * ]);
+ *
@addyosmani
addyosmani Jan 9, 2017 Member

Same comment as above.

@gauntface
gauntface Jan 9, 2017 Contributor

Done.

+});
+
+describe('Test swlib.cacheOnly', function() {
+ it('should be accessible goog.swlib.cacheOnly', function() {
@addyosmani
addyosmani Jan 9, 2017 Member

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.

@gauntface
gauntface Jan 9, 2017 Contributor

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 gauntface Fixing PR comments
0f490e1
@gauntface
Contributor

@addyosmani happy for this to land?

@jeffposnick
Contributor

I'm taking a look right now, too.

+ * }
+ * ]);
+ *
+ * // If you have assets that aren't revisioned, you can cache them
@jeffposnick
jeffposnick Jan 9, 2017 Contributor

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

@gauntface
gauntface Jan 9, 2017 Contributor

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

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()
@jeffposnick
jeffposnick Jan 9, 2017 Contributor

"service work" ➡️ "service worker"

@gauntface
gauntface Jan 9, 2017 Contributor

Fixed.

packages/sw-lib/src/lib/sw-lib.js
*/
constructor() {
this._router = new RouterWrapper();
this._precacheManager = new PrecacheManager();
+ this._strategies = {
@jeffposnick
jeffposnick Jan 9, 2017 Contributor

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?

@gauntface
gauntface Jan 9, 2017 Contributor

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:

  1. This approach and expect developers to use the smaller modules is desired (not ideal).
  2. This approach + expose the classes as well, i.e. allow new goog.swlib.strategies.<Name of Strategy>(). Simple use case plus allows configuration.
  3. Just include the class strategies. Only con is that is requires some boilerplate for basic use.
@jeffposnick
jeffposnick Jan 9, 2017 Contributor

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.

@gauntface
gauntface Jan 9, 2017 Contributor

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?

@jeffposnick
jeffposnick Jan 9, 2017 Contributor

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.

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
@jeffposnick
jeffposnick Jan 9, 2017 Contributor

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
gauntface Jan 9, 2017 Contributor

done.

gauntface added some commits Jan 9, 2017
@gauntface gauntface Changing comments
6b7ea24
@gauntface gauntface Minor tweaks to comments
9e11ac2
@gauntface gauntface Removing strategies
fe2613b
@gauntface
Contributor

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

@jeffposnick
Contributor

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
gauntface added some commits Jan 10, 2017
@gauntface gauntface Fixing lint errors
1a75be4
@gauntface gauntface Removing tests that are no longer relevant
efbf3e8
@gauntface gauntface Adding better logging for failing tests
73bd04f
@gauntface gauntface undeleting router code
f1396cb
@gauntface gauntface merged commit ed29d60 into master Jan 10, 2017

3 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@gauntface gauntface deleted the sw-lib-caching-strategies branch Jan 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment