-
Notifications
You must be signed in to change notification settings - Fork 682
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
Service Worker HTML caching bug fix #2390
Conversation
|
Performance Test Results The following fails have been reported by WebpageTest. These numbers indicates a possible performance issue with the PR which requires further manual testing to validate. https://pr-2390.pwa-venia.com/venia-tops.html : LH Performance Expected 0.75 Actual 0.64 |
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.
Looks great all around—and much more precise than the prior attempt, as well. Hopefully this solves the bug, so let's test it thoroughly and find out.
packages/venia-concept/src/ServiceWorker/__tests__/setupWorkbox.spec.js
Outdated
Show resolved
Hide resolved
* We add the `index.html` URL to the precache list, because this | ||
* file will be created after the emit phase of webpack. Due to this, it | ||
* will not be available in `self.__WB_MANIFEST`. Hence adding it manually. | ||
*/ |
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.
Good comments here.
…x.spec.js Co-authored-by: Jimmy Sanford <jimbo@users.noreply.github.com>
So this means the user won't see the changes until they ctrl+r refresh? |
Yeah, the user won't be seeing an update toast when an update is available. |
@revanth0212 I don't think original issue (blank screen) is fixed with this -
Issue - New tab still loads blank screen. On terminal I see its referring to previously cached data which is no more available. |
It must be because when you switched from Nice catch @dpatil-magento , ill have to check how to handle SW version update because this PR is gonna introduce a new version of the SW and the browser handles the SW update, not us. There must be something we can do to handle breaking SW changes. |
I followed your steps and the app did not crash. It showed the Venia Error screen and when I refreshed it worked as expected. After I refreshed, the SW updated itself and everything worked fine. I guess this is something we have to live with the first time because if the user does not refresh the page after a new deployment, new assets will not be downloaded and the new SW will be not available to serve the app. We even have a piece of code to let the browser know that this SW needs to be activated immediately.
Also, the SW usually updates itself in the background (technically it is the browser that requests and updates the SW) every 24 hours or something like that, so if we released a new version of the app and the user has not refreshed the page in a while, the SW should be updated and the new assets should be available to be served. This is hard to verify. |
@revanth0212 Tested on AWS deployed instance and below are my observations - Issue seems partially fixed on AWS deployed instances but in order to make site work fully user has to refresh twice. Assumption user has accessed storefront in past and has not cleared cache -
Note - Steps mentioned in Jordan's youtube video works on local. |
@revanth0212 Local and AWS deployed instances look good. But still seeing issue on magento cloud deployment.
But in both cases I didn't see sw updated in devtools. Screen is from safari browser but same issue in chrome too. |
QA Pass. need review on recent commits. |
Bit late to the party, just want to make sure I understand it correctly. The htmlUpdate Toast is gone and we rely on the browser to update the app itself within 24-ish hours ? |
@Jordaneisenburger I think its relies on InjectManifest together with webpack-assets-manifest. But my question is - @revanth0212 does it work without reload the page ? I see the issue is still there. For example:
Does this PR handle that case ? I would like to do something with 500 error and handle it in some way, but the other issue is that file should return whole error object:
} catch (e) {
const routeError = e.message === '404' ? NOT_FOUND : INTERNAL_ERROR;
// we don't have a matching RootComponent, but we've checked for one
// so associate the appropriate error case with this location
return { pathname, routeError };
} We can get only pathname, and routerError which gives me two flags: not found or internal error. When I get full error information about chunk loading issue I could handle it in my way, but I can't - I can rely only on INTERNAL_ERROR flag. |
I mirror the above comments that there is still something not quite right in pushing out an updated version.
This does not seem to happen for me. Steps to reproduce:
This didn't change after 24 hours nor with multiple deployments nor hitting reload on the client. Only way to get a client (chrome on desktop or android) to use the latest is to nuke away the application cache.
How will an end user ever think to reload the page, even more so if we don't communicate it. |
Well I've implemented this pr (not sure if changed after) but our customers didn't like the 'There is an update please click here'. Mostly because users don't understand this and it can cause issues where FE and BE are out of sync. So instead of showing that Toast we just do |
@Jordaneisenburger there is also a workbox upgrade to 5.x currently in develop - do you also use that already? Using location.reload() instead of a toast seems like the better way to handle it. |
@fooman no we are not using the 5.x version, Kinda don't like changing the sw once it works properly |
Thanks for sharing your valuable experience, @fooman & @Jordaneisenburger ! 🙇 Did you face any issues or maybe do you have some concerns about the direct application of I see it (checkout page, or when a form receives input, in general) an exception of applying the Eager to read your thoughts on this matter. Thank you! |
@Jordaneisenburger how did you implement location.reload()? Do you run it when there is a service worker update through listening to service worker events? |
Description
This PR addresses the issue of Venia based apps showing white screen (crashing) when a new version is released.
Background
The Service Worker provided as part apps built using the
PWA-Studio
are built using the workbox framework. Workbox provides a bunch of utilities to ease our work of SW development/maintenance. Service worker files are built along with the client code using webpack during the build stage but are not loaded when the app loads.Here is a timeline of what will happen when a user loads the app the first time.
As shown above, the Service Worker is loaded and activated after the JS files are requested and loaded. Hence the first time when this happens the Service Worker was not able to handle those requests and cache their responses.
But, if the user refreshes the page or goes to a new URL in the same domain since the SW is active, it will handle those requests and if the requests have been cached in the storage, it will return them immediately instead of fetching them from the server.
The SW also caches the
index.html
file as well.index.html
file is requested every time the user refreshes or navigates to a new URL but it is not changed often. When the file is requested by the app, the SW will return the caches HTML file from the cache and in the background request the new HTML file. After the request has been completed, the SW checks the files for changes and if it determines that the file has changed (one possibility being a new version of the app has been deployed), it sends a message to the app to show a toast asking the user to refresh.Problem
As mentioned above, the SW caches the HTML file for quick access. If it happens that, when the file has been returned, if it has links that are not available anymore either in cache or on the remote server, the app crashes because the network would resolve with a 404 error.
This happens to be a super edge case. This only happens when the SW has not cached all the JS files but has cached the HTML file and a new deployment is available, hence even the server does not have files anymore. This wouldn't happen if the user refreshes the page twice / gives enough time for the SW to cache the JS files.
Steps to Reproduce
@Jordaneisenburger found the bug and has documented the steps in this video.
To test it out locally:
develop
version ofpwa-studio
.yarn build
Terminal 2:yarn stage:venia
.precache
in it. It should have the HTML and JS files cached by the SW.step 2
. This should create a newclient.[HASH].js
file.client.[HASH].js
which is not in the cache or in the remote server.These steps replicate a new version release because every time we release a new version, we are releasing a new version of our app's codebase which stays in
client.[HASH].js
(parts of it will be in vendors and runtime as well).Solution
We use the
InjectManifest
function of theworkbox-webpack-plugin@5.1.3
to inject all the assets emitted by webpack during the build process into the SW, so once the SW is activated, it will download and cache all the files mentioned in the precache manifest.If the build assets change,
sw.js
will change and when the active SW on the browser realizes that the precache artifacts have changed, it will gracefully delete unnecessary ones (not immediately, it is up to the SW implementation to decide when to perform this) and download the new assets.Hence when the user goes to a new page, old assets that have been cached will be returned, and in the background, SW will update itself with all the required assets preached.
Unfortunately, since this is handled by the workbox-plugin and not us, we can not let the user know if an update is available like before. I will look into way to get that working, meanwhile, this is the best solution we have.
To validate this, perform the same set of steps as mentioned above on this branch.
This is the 3rd version of the solution for this problem and by far the simplest solution, thanks to the Zman. Here are the other 2 for reference:
https://github.com/magento/pwa-studio/tree/revanth/sw_redeploy_bug
https://github.com/magento/pwa-studio/tree/revanth/sw_redeploy_bug_v2
Trivia
This is a major change because I had to change the signature of the configureWebpack.js file. This is needed because, as of the
develop
branch the Client code and the SW code are compiled using 2 different webpack compilation processes and theInjectManifest
plugin in the SW compilation can not read the assets emitted by the client compilation.Related Issue
Closes PWA-408
Closes #2351
Acceptance
The app should not crash on a new version release.
Verification Stakeholders
@zetlen
@jimbo
@Jordaneisenburger
@jcalcaben
Verification Steps
precache
in it. (Cache name might differ)workbox-runtime
cache.You can also perform a sophisticated check like mentioned in the description, but unfortunately, it can only be done locally because it involves code changes and rebuilds.
Checklist
dev
mode. This is only intended for thestage
mode because the SW is not active in thedev/watch
mode.