-
Notifications
You must be signed in to change notification settings - Fork 820
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
Bundling changes/improvements #1831
Conversation
5a90a5c
to
295228f
Compare
I finished updating the rest of the packages, and I updated all the tests accordingly. I know this is a lot of changes to review, but overall I think these changes make our package structure much simpler. Also, it looks like it reduced our overall code size by more than 1KB (gzipped) from the previous PR. We're now in the 8KB range! 🎉 |
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.
Marking workbox.strategies.<strategyName>
deprecated in favor of new workbox.strategies.StrategyName()
is also a pretty big change that got rolled this PR, which wasn't included in the description. It's worth calling that out because we're going to have to communicate that in the release notes and update a bunch fo documentation examples.
Ahh, sorry, I updated that in the proposal doc, but I forgot to also add it to the bullet points in this issue. FWIW, I'm open to other options as well. Here are a few I think are find in a rough order of preference:
|
I'm cool with moving to workbox/packages/workbox-build/src/lib/runtime-caching-converter.js Lines 144 to 145 in b8eb5a2
|
Ok, I can address that in a new PR. The rest of the stuff you mentioned has been updated. |
PR-Bot Size PluginChanged File Sizes
New Files
All File SizesView Table
Workbox Aggregate Size Plugin8.83KB gzip'ed (59% of limit) |
As a first step toward the changes proposed in #1830, I've updated the
workbox-routing
andworkbox-core
packages to give you an idea of what I'm thinking. If no one has any serious objections, I'll go ahead and update the rest of the packages as well.The primary things I've done here are:
_public.mjs
,_default.mjs
, andbrowser.mjs
filesskipWaiting()
andclientClaim()
methods fromworkbox-sw
toworkbox-core
(see here for rationale)R: @jeffposnick @gauntface