-
Notifications
You must be signed in to change notification settings - Fork 685
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
HTML Caching and SW Communication API Implementation. #1905
Conversation
… is not mentioned. Weird.
Co-Authored-By: Andy Terranova <13182778+supernova-at@users.noreply.github.com>
…gento/pwa-studio into revanth/serviceWorkerRefactor
…ngRefactor' into revanth/swCachingHTML
console.log('SW Registered'); | ||
}) | ||
.catch(() => { | ||
window.console.warn('Failed to register SW.'); |
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.
Why window
here and not in the .then
?
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.
Hmm yeah I thought this got dropped to just console.log
and our minifier would drop console.log
s (so this won't show up in production)
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.
Yeah console.*
gets removed by webpack
for production. But we need a warning for SW not registering. That is why I used window.console.*
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.
Please add a comment to that affect so we don't accidentally "fix" that in some later work.
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.
Verification steps work! This looks good. I had a few small things I'd like to see but otherwise this is great. Fix them and I'll approve.
console.log('SW Registered'); | ||
}) | ||
.catch(() => { | ||
window.console.warn('Failed to register SW.'); |
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.
Please add a comment to that affect so we don't accidentally "fix" that in some later work.
@@ -0,0 +1,22 @@ | |||
import { handleMessageFromSW } from '@magento/venia-ui/lib/util/swUtils'; |
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.
If moving this stuff to venia-concept
is a "TODO" then open an issue for it :)
I have added the comment. Regarding moving files to |
Description
HTML Caching and Service Worker Communication API Implementation. The service worker will be following the
StaleWhileRevalidate
strategy for all HTML files. It will return the readily available file from the cache and try to update the file in the background. If the file happens to be different from the one already in the cache, it will send a message to the UI about an update available and a button in the message to refresh the page.Related Issue
Closes #1772 #1781
Verification Steps
yarn build && yarn stage:venia
.venia-concept/dist/index.html
and make a change like adding a new script tag<script>console.log('Hey this is a new HTML')</script>
Screenshots / Screen Captures (if appropriate)
Checklist
yarn stage:venia
andyarn watch:venia
should work like previous.