Skip to content
This repository has been archived by the owner on Jan 23, 2021. It is now read-only.

Move to a single cache for all responses #123

Merged
merged 6 commits into from
Jun 9, 2016
Merged

Conversation

jeffposnick
Copy link
Contributor

R: @addyosmani @gauntface @wibblymat

I think I'm happier with the code using this approach, as it also simplifies some of the logic by using Map and Set, neither of which were widely available when the code was originally written.

The one drawback that I can see is that it's no longer possible to use caches.match() to pick up a given Response from arbitrary code that the user writes, since the Request objects used as cache keys now have URL parameters appended to them. That's not an advertised use case, though, so I'm not worried.

This will be part of a forthcoming 4.0 release, so I'm not ready to update the package.json metadata yet.

Closes #86

@@ -181,7 +181,7 @@ function generate(params, callback) {
var runtimeCaching;
var swToolboxCode;
if (params.runtimeCaching) {
var pathToSWToolbox = require.resolve('sw-toolbox/sw-toolbox.js');
var pathToSWToolbox = require.resolve('sw-toolbox/build/sw-toolbox.js');

Choose a reason for hiding this comment

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

Ack, my bad. I read through the list of commits to decide what kind of version bump I needed for the latest release, and missed that this caused a breaking change.

Copy link
Contributor

Choose a reason for hiding this comment

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

Honest oversight :) IMO we should revert this change upstream (or revert, retag, intro as a major change). It's starting to cause issues to be filed on projects elsewhere (Polymer CLI etc)

Choose a reason for hiding this comment

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

sw-toolbox could be changed to copy the build back to the root of the project for the release. This is the result of not having the built sw-toolbox in the master branch.

@wibblymat
Copy link

This is great!

@wibblymat
Copy link

Regarding the point about caches.match... could the parameters be stored on the Response instead? So, construct the modified URL, fetch that, and cache.put with the original Request as the key.

I guess it makes it harder to look this stuff up...

@jeffposnick
Copy link
Contributor Author

That's an interesting idea re: using Response objects—maybe in a header?—to store the cache versioning info. The drawbacks I see there are:

  • I'd have to do a bunch of cache.match() operations to get all the versioning info within the install handler, vs. (what I'm assuming is a cheaper?) cache.keys().
  • Most of the fields in a Response object returned by fetch() are read-only, so I'd end up constructing new Response objects and synthesizing a combination of the original and an updated headers in order to do that.
  • I wonder if there are CORS concerns from the perspective of the controlled page if it ends up using a Response that has extra headers set?

Any thoughts on those?

@addyosmani
Copy link
Contributor

LGTM. My only real comment was to do with the toolbox dep. Everything else appears in order here.

@jeffposnick
Copy link
Contributor Author

I'm going to go ahead with the strategy of using a custom search param in the Request URL to version each resource. Down the line, if w3c/ServiceWorker#854 is implemented, it could make sense to switch to a URL fragment.

@jeffposnick jeffposnick merged commit db3e199 into master Jun 9, 2016
@jeffposnick jeffposnick deleted the single-cache branch June 9, 2016 20:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants