-
Notifications
You must be signed in to change notification settings - Fork 16
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
KaiVandivier
wants to merge
7
commits into
master
Choose a base branch
from
dhis2-15625-catch-sw-precaching-errors
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
KaiVandivier
commented
Aug 16, 2023
{ url: 'https://not-a-site.com/squip4.jpg', revision: '1' }, | ||
]) | ||
|
||
/* An alternative to patch package, where a custom precache controller is made and implemented |
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.
This is included here to show an example of another approach to catching & handling precaching errors (option 2 in the main PR comment)
Quality Gate passedIssues Measures |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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).
index.html
and the contents ofstatic/js/*
(and maybestatic/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 insideevent.waitUntil()
during the installation phase, which causes the installation to abort completely.Options
patch-package
to edit a few lines in theworkbox-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:I like this option ^
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 inset-up-service-worker.js
to show what that looks likeTesting 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:
To do:
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.