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: precached files are not updated #386

Merged
merged 6 commits into from
Nov 28, 2020
Merged

Conversation

kedrzu
Copy link
Contributor

@kedrzu kedrzu commented Oct 21, 2020

I stumbled upon an issue - my main page was not updating. (route /).
After lot of digging I found, that it's precached inside service worker and is not updated, even when updated service worker is uploaded.

Some more about this issue with my comment here:
#381

I tried to fix it with cacheOptions option you mention in docs, but it's not working (https://pwa.nuxtjs.org/workbox#cacheoptions).
That's because this object is not passed into sw.js file.

More details in comments to code.

I had problem to make it passing through tests, because you use some kind of snapshot of resulting service worker file.

Comment on lines 49 to 55
const cacheOptions = options.cacheOptions || {};
const precacheList = options.preCaching.map(url => ({
url: url,
revision: cacheOptions.revision
}))

workbox.precaching.precacheAndRoute(precacheList, options.cacheOptions)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to google:
https://developers.google.com/web/tools/workbox/modules/workbox-precaching#explanation_of_the_precache_list

Revision must be passed separately per each precached file and not in options object.

Comment on lines 24 to 27
// revision
if (!options.cacheOptions.revision) {
options.cacheOptions.revision = randomString(12)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If no revision is set, browser complains about precaching file without revision:
image

I think that adding some randomized revision as a default would be beneficial.
I tried to get random version from nuxt environmental variables, but couldn't find it.

Copy link
Member

Choose a reason for hiding this comment

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

I think we have to pass null instead for revision info of _nuxt/ assets because we always add hash to URL for long-term caching:

{url: '/scripts/app.0d5770.js', revision: null},
For the second and third object in the example above, the revision property is set to null. This is because the revisioning information is in the URL itself, which is generally a best practice for static assets.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, for script assets that's true. But this case is about .html files, which do not have any hash in name.
Once they are precached, you cannot do anything to update them, other than unregistering and registering service worker.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The same applies to offline page (404.html) and start url for PWA (/?standalone=true by default).
Once they get precached, PWA stops being updated and offline page too.

This is a behavior I observed in my own application - when installed app as a standalone PWA application it refused to be updated even if completely new deploy was made.

Copy link
Member

Choose a reason for hiding this comment

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

I see you are right. But the problem is that generating revision on build-time only works for static generated websites. If response of a SSR page changes, workbox still refuses to update..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If precache would work with NetworkFirst, everything would be fine.
But from what I've read, it always work as CacheFirst.

Maybe it's a good idea to disable precaching at all? Or replace it with manually fetching those pages in sw.js file and caching them with standard NetworkFirst cache, instead of depending on workbox standard precache?

Copy link
Member

Choose a reason for hiding this comment

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

@kedrzu This PR looks good after and partially fixing issue. But indeed, precaching is only CacheFirst (GoogleChrome/workbox#1767).

I will try to implement new offlineRoutes option for (NF or SWR) strategy :)

Copy link
Member

Choose a reason for hiding this comment

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

@codecov
Copy link

codecov bot commented Oct 22, 2020

Codecov Report

Merging #386 into master will decrease coverage by 1.50%.
The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #386      +/-   ##
==========================================
- Coverage   84.67%   83.17%   -1.51%     
==========================================
  Files          12       12              
  Lines         398      410      +12     
  Branches      121      123       +2     
==========================================
+ Hits          337      341       +4     
- Misses         56       64       +8     
  Partials        5        5              
Impacted Files Coverage Δ
lib/workbox/module.js 100.00% <ø> (ø)
lib/workbox/utils.js 69.23% <0.00%> (-25.51%) ⬇️
lib/workbox/options.js 89.65% <80.00%> (-0.92%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d8fc325...801ff5c. Read the comment docs.

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