Conversation
corrideat
added
Kind:Enhancement
group-income
|
||||||||||||||||||||||||||||
| Project |
group-income
|
| Branch Review |
2503-offline-first-pwa-caching
|
| Run status |
|
| Run duration | 11m 47s |
| Commit |
|
| Committer | Ricardo Iván Vieitez Parra |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
10
|
Skipped
|
0
|
Passing
|
114
|
| View all changes introduced in this branch ↗︎ | |
| if (navigator.onLine !== false) { | ||
| await sbp('chelonia/contract/sync', identityContractID) | ||
| } |
There was a problem hiding this comment.
So... do we sync this once we do come back online? If so, where?
There was a problem hiding this comment.
No, we don't explicitly. It'd be handled like any other contract by that point.
taoeffect
left a comment
taoeffect
left a comment
There was a problem hiding this comment.
Here's what I tried on Android:
- I created the PWA and logged in with it (via the ngrok tunnel)
- In the app launcher, I swiped up on the PWA app to kill it
- Disabled phone's internet
- Opened app again
Greeted with:
Followed by:
Is this something that's preventable? I.e. is it possible to still have the app load?
| const CACHE_VERSION = '1.0.0' | ||
| const CURRENT_CACHES = { | ||
| assets: `assets-cache_v${CACHE_VERSION}` | ||
| } |
There was a problem hiding this comment.
Should this version number be tied to some environment variable, e.g. GI_VERSION or something else?
There was a problem hiding this comment.
I was thinking about that. It could, but I don't think it needs to, so long as it's updated if breaking changes to the cache are made.
There was a problem hiding this comment.
Great work @corrideat! Review is ready.
One issue is that this seems to not work well still when offline. I found an app that seems to work when the PWA is force quit and restarted while offline, so it should be possible to improve the behavior:
| // Files like `main.js` or `main.css` should be revalidated before use. Se we use the default headers. | ||
| // This should also be suitable for serving unversioned fonts and images. | ||
| return h.file(subpath) | ||
| .header('Cache-Control', 'public,max-age=604800,stale-while-revalidate=86400') |
There was a problem hiding this comment.
Hmm... why is this being added? I see that we have a special section for cached files above, so why should these be cached too when it seems like these are files that might change?
And if this is really necessary, could you add a comment here explaining why?
I want to avoid the app failing to update when files are updated...
There was a problem hiding this comment.
Because if we don't store these in the cache, the app can't possibly work offline. Adding stale-while-revalidate means that it's fine to serve stale files while they're being revalidated (normally, all cached requests need to be revalidated).
There was a problem hiding this comment.
In any case, the comment is outdated, so I agree that this should be updated.
corrideat
force-pushed
the
2503-offline-first-pwa-caching
branch
from
46b4488 to
3680a42
Compare
| if (request instanceof URL) { | ||
| console.error('@@@@XX', 120, request, response.status) | ||
| } | ||
| return fetch(event.request.clone?.() || request).then(async (response) => { |
There was a problem hiding this comment.
This is what seems to have fixed the issue. Note that I'm still using the original request here instead of the 'rewritten' request.
| } | ||
| } | ||
|
|
||
| return fetch(event.request.clone?.() || request).then(async (response) => { |
There was a problem hiding this comment.
Can you add a comment here explaining what problem this event.request.clone?.() || request is fixing? If you're not sure why it works it's fine to say that, but it still needs a comment explaining why this was added in the first place since it falls into the category of "weirdness".
taoeffect
left a comment
taoeffect
left a comment
There was a problem hiding this comment.
It's still getting stuck on something (stuck on the same white screen...)
|
The error iOS Safari-based PWA gives:
Doesn't happen with https://github.com/GoogleChromeLabs/squoosh |
corrideat
force-pushed
the
2503-offline-first-pwa-caching
branch
from
67db3d3 to
7779f0b
Compare
2 participants
No description provided.