-
Notifications
You must be signed in to change notification settings - Fork 823
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
Log warning for bad cache control #1317
Conversation
Chillin till I can checkout the size growth (This should only be a dev feature). |
43f350e
to
dd94124
Compare
@jeffposnick a review would be appreciated - I figured out how to make rollup not include this stuff. |
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.
Had a quick read through. Check for cache headers and the warning logged LGTM.
(Smallest of nits we can address in the codebase later: we're still using 2017 in our license headers)
packages/workbox-core/_default.mjs
Outdated
@@ -61,6 +62,8 @@ class WorkboxCore { | |||
`${padding}https://github.com/GoogleChrome/workbox/issues/new` | |||
); | |||
logger.groupEnd(); | |||
|
|||
checkSWFileCacheHeaders(); |
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.
Small nit: could you wrap this in
if (typeof checkSWFileCacheHeaders === 'function') {
checkSWFileCacheHeaders();
}
just to double-check that we're not somehow in a scenario in which it's set to null
? I can't really imagine why that would happen with our normal builds, since this is in a NODE_ENV !== 'production'
block , but I wonder if some custom build scenarios might end up leading to a mismatch.
@jeffposnick it shouldn't ever happen (It's following the same approach as assert class - but I added in the check regardless) |
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.
Thanks for the extra check.
PR-Bot Size PluginChanged File SizesNo file sizes have changed. New FilesNo new files have been added. All File SizesView Table
Workbox Aggregate Size Plugin☠️ WARNING ☠️We are using 155% of our max size budget. Total Size: 22.7KB Gzipped: 9.1KB |
R: @jeffposnick @addyosmani @gauntface
Fixes #620
Adds a basic test for cache-control header.