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

Offline-first PWA #56

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

Conversation

cccaballero
Copy link

PWA for web dashboard, #46 related

@carlos-penton
Copy link

carlos-penton commented Apr 12, 2020

I suggest adding a github action to update cache_version automatically

Copy link

@carlos-penton carlos-penton left a comment

Choose a reason for hiding this comment

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

A notification indicating that the app were updated in the background (and then requesting a page refresh) could be nice.

@@ -0,0 +1,140 @@
// This is the service worker with the Cache-first network

importScripts('cache_version.js');

Choose a reason for hiding this comment

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

Consider wrapping this inside a try-catch block and setting cache_version manually in the error handler.

https://stackoverflow.com/a/15421180

Copy link
Author

Choose a reason for hiding this comment

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

I am not sure if we should set a manual cache version. I think we should only fallback to the online version of the page in case of a service worker error (the current behavior). Setting a manual cache_version may stuck users in an old page version.

console.log("[PWA Builder] Claiming clients for current page");
// event.waitUntil(self.clients.claim());
event.waitUntil(
clearCaches().then(() => {

Choose a reason for hiding this comment

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

Perhaps the cache should be cleared after ensuring all files were downloaded successfully, so if anything goes wrong at least the last version is available.

Copy link
Author

Choose a reason for hiding this comment

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

I think we want users always accessing to the last data, so is better if we sent the user to the online version of the page in case of issues.

Choose a reason for hiding this comment

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

Sure, however if the user is completely offline or if anything goes wrong while downloading at least the last data is available. Real-time data should be always displayed among old data as you correctly say, but anything is better than nothing, imho.

Copy link

@carlos-penton carlos-penton Apr 13, 2020

Choose a reason for hiding this comment

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

Setting the cache version manually would be only as a fallback. If importScripts doesn't fail the cache_version won't be undefined, meaning that the user is online and the worker should find updates. The idea of setting it manually is to show the last loaded data, specially in an offline (and potentially previously updated) environment.

Copy link
Author

Choose a reason for hiding this comment

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

Ok, let me explain the idea behind the current implementation, we are addressing here three main issues besides the PWA by itself.

  • We need reduce at minimum the data usage of users.

  • We have some issues with the json files cache in one of the project deployment servers (that's why currently there is using the jquery timestamp for request in the master branch, and no, we can't solve it in the server side)

  • And users should always access to last data available

That's why I am proposing this hard offline-first/cache-first approach. So for achieving this the service worker loads the requests one unique time, and always return them from cache, no matter if the application is online or offline. In a more standard PWA approach, for updating the cache, usually the SW is constantly querying the server looking for changes in files, but I want to avoid this using the cache versioning (which will be automatically updated in every post to the repository). So, the SW will only update the cache when there is a cache version change.

If we set a manual value as a fallback for the cache version I see two main issues:

  • One, if the script loading fails because a network error, there is a really high possibility that other requests also fail, so using a hardcoded cache name will not prevent the application from working improperly if it is opened offline right away. But supposing that only the loading of the version script fails because of a momentary network error, the next time that the application is open, the SW will correctly load the cache version file and reload the whole application again increasing the data usage and wrongly notifying the user that there is a new app version.

  • The second possibility is that the loading of the script fails because of the introduction of a bug (maybe the file name was changed, moved, removed or something else), in that case, the usage of a hardcoded cache version will stuck the user in that version until the bug is discovered and fixed because the cache version will be always the same, that's something we really want to avoid because outdated data can easily be misinterpreted.

I am thinking, and maybe we can include some kind of notification to the user if something fail (the loading of the cache version script or the SW requests) alerting the user that the application will not be available offline

Copy link

@carlos-penton carlos-penton Apr 14, 2020

Choose a reason for hiding this comment

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

Agree, mostly. Sorry if I didn't explain myself correctly.

By setting the cache manually I didn't mean hardcoding a value. I mean assigning a value so the most updated available cache is shown (at least). This could be achieved by querying the cache keys. I also suggest using timestamps for cache version numbers so the cache date can be computed and the numbers will still being a non descending series.

About the first issue, if the script loading (or anything else during the cache update) fails the user could be notified properly (like with a big red banner saying "You are offline and displaying data of YYYY/MM/DD mm:ss". It's important that the cache contains all the updated files (and not some of them) before being used, since the UI might change between versions.

About the second issue...well, things happens. If there is a bug it must be fixed. With enough eyes looking all bugs pop up :D .

I think that computing the cache version (not hardcoding it) would be a nice alternative for the users over a "not available offline" app. Think of this as an alternative to a mobile app, which gets updated but shows your already downloaded data.

Finally, It's great to see responsible developers as you investing time and efforts on this project. Thank you very much.

Copy link
Author

Choose a reason for hiding this comment

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

@g3nger, Thanks in the name of all the team, there are a lot of people helping to make this happen, many of them are even not developers, but without their work all this would be impossible.
I have to say thank you also, because you are helping to take this foward too.

I was exploring your suggestion, but there is a bigger issue. In case of failure, if we wrap the script import within a try-catch and assign a new value to cache_version, the SW is not notified when the cache_version.js script is available again and the install event is not triggered, even if the original cache_version changes.

Choose a reason for hiding this comment

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

Happy to help!

You're right! The sw would never be installed again since the code itself hasn't changed and the browser won't recognize it as new; then not updating the cache in the first place.

How about this approach:

Attempt checking/updating the cache regularly (say, every hour with a setInterval call) as part of the execution of the sw. This implies not calling importScripts, but fetching a file with the cache version instead as part of the checking for updates process. The sw would get installed once and self-maintain it's cache over time. If something new should be cached (like when a new file appears) then precacheFiles should be modified effectively changing the code of the sw then making the browser downloading it again and reinstalling it. The cache version file could be a plain text file with the timestamp maintained apart from the sw, just like cache_version.js. If the cache version changes, the sw eventually will notice it, and if a network error is detected (like when offline) there is always the big red banner indicating the offline/outdated status. Of course the cache should be built while installing too.

Again, all this is to allow displaying at least old data instead of nothing, which is a personal taste of mine. I think it's useful because of the data update frequency, which is quite high.

@cccaballero
Copy link
Author

I suggest adding a github action to update cache_version automatically

@leynier will work in the update action

@cccaballero
Copy link
Author

A notification indicating that the app were updated in the background (and then requesting a page refresh) could be nice.

Done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants