Skip to content
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

fix: catch sw precaching errors [DHIS2-15625] #812

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

KaiVandivier
Copy link
Contributor

@KaiVandivier KaiVandivier commented Aug 1, 2023

https://dhis2.atlassian.net/browse/DHIS2-15625

Proof of concept of handling precaching errors so we can do something about it

Options from here

Once we do handle errors, there are a couple options for what to do next. In any solution, we should give a thorough error report (an example of one is below).

  1. Abort* installation and give thorough error report (might confuse user about why it’s not updating)
  2. Continue with installation, despite missing files (might end up with a dysfunctional app)
  3. Make some logic for deciding which installations to continue or abort*, based on the files that failed to be precached
    • For example, if failed files are all font files, then maybe the app is okay to continue. Then again, maybe we don't want to commit to an ugly app
    • A couple crucial files might be index.html and the contents of static/js/* (and maybe static/css/*)

* If we abort, should we “tear down” a previously installed app version, so the user can see the (potentially broken) new version? If not, we can provide technical instructions for how they could do it themselves, as in the example error report below

Code stuff

Background
Precaching errors cause installation to fail, because in the workbox PrecacheController code, those errors cause a promise to reject inside event.waitUntil() during the installation phase, which causes the installation to abort completely.

Options

  1. One way is to use patch-package to edit a few lines in the workbox-precaching code to catch errors and prevent precaching errors from scrapping the entire installation. Currently I have it set up to temporarily catch precaching errors and compile the list of errors, yet still abort the installation, but this time with a thorough error message:
Uncaught (in promise) Error: The following assets were unable to be fetched and cached 
when installing pwa-app version 10.3.9, so the installation has been aborted:
 * https://not-a-site.com/hey.jpg
 * https://not-a-site.com/bogus1.jpg
 * https://not-a-site.com/derp2.jpg
 * https://not-a-site.com/diddlibap3.jpg
 * https://not-a-site.com/squip4.jpg

If another version of the app is installed in the browser, you will continue to see that version.
If not, you will see version 10.3.9, but caching and offline features will be unavailable, and 
the app may not be fully functional due to the missing files. You can find the app version 
that's currently active at the bottom of the user profile menu.

[FOR TECHNICAL USERS]: If another version is installed and you would like to attempt to view
version 10.3.9 (knowing the above caveats), you may use your browser's developer tools to
find the service worker that's currently active for this app and unregister it.

I like this option ^

  1. Another way is to extend the PrecacheController object to have some more flexibility about how it's used, which may be useful depending on how we choose to respond to precaching failures. There's some commented-out code in set-up-service-worker.js to show what that looks like

Testing notes
The service worker is temporarily set up to perform precaching in dev environments, and is configured to try to precache nonexistent files to show the error.

You can run this in the repo:

yarn && yarn build:pwa && yarn workspace pwa-app start

To do:

  • Some testing instructions
  • Remove logs and comments

Other stuff

I also refactored the normal precaching logic in set-up-service-worker.js a bit to simplify it now that I understand how the Workbox internals work better. I tested out the routing and precaching and it still works as intended.

{ url: 'https://not-a-site.com/squip4.jpg', revision: '1' },
])

/* An alternative to patch package, where a custom precache controller is made and implemented
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is included here to show an example of another approach to catching & handling precaching errors (option 2 in the main PR comment)

@KaiVandivier KaiVandivier marked this pull request as ready for review August 16, 2023 15:31
@KaiVandivier KaiVandivier requested a review from a team August 16, 2023 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant