-
Notifications
You must be signed in to change notification settings - Fork 328
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
I signed it! |
CLAs look good, thanks! |
precache: precache | ||
cache: helpers.cache, | ||
uncache: helpers.uncache, | ||
precache: helpers.precache | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This cleans up the main sw-toolbox entry quite a bit while making it easier to just pull in the helpers. It's also passing our current CI tests. I'm personally fine with this change, but let's see what @jeffposnick and @gauntface think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A wild @wibblymat appears!
I agree that this change makes sense for people who want to use the helpers on their own.
👍 from my perspective as well. As an FYI, we're thinking about similar topics in GoogleChrome/workbox#44 I see this PR a move in that direction using the current codebase, which should be continue to be relevant for a while yet. But if you have any feedback about the proposals tracked in that issue, that would end up being the most meaningful longer term. |
Thanks! I'll take a look at that issue. |
Friendly ping to merge this in. :) |
To make it easier to use sw-toolbox as a library, this PR moves all the helper and listener functions out of sw-toolbox.js so they can be imported without setting up the event listeners.
Tests in Chrome 51 pass locally.