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

Naming/logic cleanup around cache keys #65

Merged
merged 3 commits into from
Dec 11, 2015
Merged

Conversation

jeffposnick
Copy link
Contributor

R: @gauntface @wibblymat @addyosmani

This PR's primary purpose is to use asset URLs without cache-busting parameters as the key names in the caches that sw-precache maintains. This should make them play nicer with third-party service worker code that calls caches.match(assetUrl), since assetUrl obviously won't have those cache-busting parameters.

In the course of updating that logic, I also updated some comments in the fetch handler. That led me to simplify the logic that handles an edge case where someone's deleted an entry from one of the caches, i.e. when the cache key is missing.

Closes #48

@gauntface
Copy link

The changes look good.

Only comment would be if its worth moving out the logic for cache busting url to a seperate function along the lines of getCacheBustedUrl(originalUrl).

@jeffposnick
Copy link
Contributor Author

I've refactored the cache-busting into a separate function, and added tests.

@gauntface PTAL.

@wibblymat
Copy link

Can't quite get my head around the consequences.

If my site used a previous version of sw-precache, and then I upgrade, what happens when my users get the new version? Will it upgrade cleanly?

The two possible failure cases I have are:

  1. The new worker fails to match to the existing caches, so downloads resources again
  2. The install/activate process will leave resources in existing caches will the old cache-busted URLs, and only new resources get the new URLs.

I'm unable to get my brain into gear and work this out for myself.

@jeffposnick
Copy link
Contributor Author

This should play nicely with legacy service workers already on user's browsers. The existing caches will be untouched, and can continue to serve responses even with the new service worker code in place. Any new/updated assets will trigger the caching process, which will result in new cache entries that have as keys un-busted request URLs.

The reason that this is backwards compatible is that the cache entry request URL keys are never used in cache lookups; it's the cache name that's used, and then we always use the first entry in the matching cache (regardless of the request URL key) to serve the response. The cache names aren't changing as part of this PR.

I've tested this scenario with the demo.

@wibblymat
Copy link

Awesome. I figured you'd thought of it, but I couldn't grok it at the time :)

LGTM.

jeffposnick added a commit that referenced this pull request Dec 11, 2015
Naming/logic cleanup around cache keys
@jeffposnick jeffposnick merged commit 6daea22 into master Dec 11, 2015
@jeffposnick jeffposnick deleted the cache-key-cleanup branch December 11, 2015 15:16
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.

3 participants