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

Implemented a cache post-delete hook. #40

Merged
merged 4 commits into from
Oct 28, 2016
Merged

Implemented a cache post-delete hook. #40

merged 4 commits into from
Oct 28, 2016

Conversation

bourquep
Copy link
Contributor

My use case is:

My service worker creates an IndexedDB database to store some offline data. The database name corresponds to the cache name, so that it can leverage the CACHE_VERSION versioning mechanism and start fresh when the service worker code changes.

I needed a way to delete stale databases just like old caches are deleted in the activate event handled by delete-old-caches.js. I figured that a hook would be appropriate, where the service worker code simply needs to define a specific function name to enable the hook.

Copy link
Owner

@jkleinsc jkleinsc left a comment

Choose a reason for hiding this comment

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

@bourquep thanks for the PR! In general I'm in favor of making the change, but maybe you could simplify the code at bit by checking for the existence of the function vs catching the error if the function doesn't exist?

@@ -9,11 +9,28 @@ self.addEventListener('activate', function(event) {
return (cacheName.indexOf('$$$inactive$$$') === -1 && cacheName.indexOf(CACHE_PREFIX) === 0 && cacheName !== CACHE_VERSION);
}).map(function(cacheName) {
logDebug('Deleting out of date cache:', cacheName);
return caches.delete(cacheName);
return caches.delete(cacheName).then(function() {
Copy link
Owner

Choose a reason for hiding this comment

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

@bourquep why not check for existence of function here instead of catching non existence of function in _postDeleteCacheHook? Seems like that would be simpler? Something like

if (brocswPostDeleteCacheHook) {
  return brocswPostDeleteCacheHook(cacheName);
} else {
  return Promise.resolve();
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed. I'll update and test. JS is not my main programming language and I was not aware that it could be done that way.

@bourquep
Copy link
Contributor Author

@jkleinsc done.

@bourquep
Copy link
Contributor Author

Had to rebase and force-push my branch. PR is clean now.

@jkleinsc
Copy link
Owner

@bourquep looks good to me. Thanks for the PR!

@jkleinsc jkleinsc merged commit eb37196 into jkleinsc:master Oct 28, 2016
@bourquep bourquep deleted the delete-old-caches-hook branch October 28, 2016 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants