-
Notifications
You must be signed in to change notification settings - Fork 16
PB-1754: add client-side caching through Service Worker API #1358
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
Conversation
web-mapviewer
|
||||||||||||||||||||||||||||
| Project |
web-mapviewer
|
| Branch Review |
feat-PB-1754-client-side-caching
|
| Run status |
|
| Run duration | 04m 55s |
| Commit |
|
| Committer | Pascal Barth |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
|
|
0
|
|
|
0
|
|
|
20
|
|
|
0
|
|
|
253
|
| View all changes introduced in this branch ↗︎ | |
e342fe0 to
b70bef1
Compare
|
We should decide on how we want to show users the "offline" feature is ready. Right now it's an ugly popup in the middle of the app. I propose we add that somewhere in the header (on desktop), and in the help section on mobile |
| return url | ||
| } | ||
|
|
||
| // caching images from WMS and WMTS after they are loaded from the network |
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.
Just a thought: would it make sense to make the background layers have more priority in caching somehow?
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.
I don't know if I want to hard-code any layer ID there, and doing that "dynamically" depending on the layers config looks quite complicated. Maybe in a second step
sommerfe
left a comment
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.
I noticed that when you have activated the browser option "Deactivate cache" it does not work (obviously) so maybe we could add a warning if we have this option active
|
@pakb to test, we set it to flight mode then refresh the web page right? I couldn't see any difference (or any pop up) while testing it. Did I miss something? |
set up caching for HTML/JS/CSS files (so that the app can be reloaded even without network), essentials backend items (layers config, topics) and for a limited amount of tiles from our WMS/WMTS the app has loaded while being used.
half our test suites were failing with Service Worker active, so disabling the setup of Service Worker API if we detect that we are in our Cypress tests
as Service Worker is disabled when testing with Cypress, the component that was importing part of the SW API function was failing to build when running Cypress. With this conditional build, it can now be emptied (and especially not import the SW part) when we are running Cypress. This lib needed rollup v4+ so I've set it as such in the overrides of the package.json of the mapviewer package Also adding TypeScript types for all env variables declared through Vite and .env files
the CI was still running in non Cypress mode
causes some issue that it stays active on some CI tests (it still messes up with the loading bar logic when it should be deactivated) taking the opportunity to add a little bit of style to our logging, to better visually differentiate the now huge amount of logs we have in our dev console Add style to store/router sync plugin and fix 2 TS error/lint issues
should reduce the flakiness on some print tests that were running too early This is also "testing" our PostMessage API code, which will later be used to implement our new print service.
c1c8278 to
e4e3b7d
Compare
You should load the app, wait for the SW to be done caching things (in the help section on mobile, you can see the status) and then you can airplane mode and reload, you should still have the app working |
e4e3b7d to
b5197b0
Compare
MobileWorks. I also get the warning in the menu. Is it intentionally a bit hidden? If a user puts it in airplane mode, then they might know that it's the case, but if you just lose connection, then it might be confusing. (sorry if I missed a discussion/decision on this) Something else: you could probably disable "Give feedback" and "Report problem" buttons as well, when detecting offline. Also: I somehow managed to open the map zoomed in. Then I activated airplane mode. Now I have some tiles loaded on LK50: Now if I zoom out, the BG layer would change to LK100, which can't be loaded. But there is nothing displayed either, so I would know that when zoomed in I still have data, maybe? Maybe these observations are in optimization territory and you'll want to do them later :) DesktopWorks as well. The first time I tried it had an error after reloading, but now it seems to work 👍 Feature infosJust some other observation: when clicking on a feature in offline mode nothing happens. Maybe we could still show the popup but with an information... |
schtibe
left a comment
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.
Nice, just one small comment! 👍
| </FontAwesomeLayers> | ||
| <span | ||
| v-if="withText" | ||
| class="fw-bold mx-1" |
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.
You could maybe also use gap in the enclosing flexbox
Add a SimpleWindow that warns the user he/she should refresh the app because the version of the code that is cached on the device is obsolete add i18n for ServiceWorker updating the existing text for out-of-date data, and adding a new text for the refresh button
b5197b0 to
fac7ec9
Compare

set up caching for HTML/JS/CSS files (so that the app can be reloaded even without network), essentials backend items (layers config, topics) and for a limited amount of tiles from our WMS/WMTS the app has loaded while being used.
Test link