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: cache busting service worker #383

Closed
wants to merge 1 commit into from

Conversation

qzhou1607-zz
Copy link
Contributor

Pull request checklist

Make sure you:

Short description of the change(s)

  1. Cache busting service worker (Re-add service worker #362 )
  2. Currently the service worker caches sw-reg.js as well. Remove that from the list of files being cached.

Btw, generate service worker has to happen after build:hexo otherwise it can't get the right list of .html files in the service worker. And the script has to be in the root directory otherwise the scope isn't right. So instead of copying the scripts to static/scripts, I write its own gulp task.

I also tried moving all rev and revplace tasks after build:hexo, hoping that we don't need to have to separate tasks for rev themes, md files and service worker that way. But sw-reg keeps reporting redundant service workers and I had trouble figuring out why. So I kept the task order as it was, I'm switching to the next task now but let me know if you'd like me to keep digging on this route.

Copy link
Member

@molant molant left a comment

Choose a reason for hiding this comment

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

With this PR the service worker files are revved but they still don't have the right cache-control header. They should be under /static/scripts to get it automatically and right now they are in the root.

@qzhou1607-zz
Copy link
Contributor Author

@molant I think moving the service worker files to /static/scripts will result in scope issues. According to this answer, the max scope of a service worker is where it's located (in our case /static/scripts), so unless we can change the Service-Worker-Allowed header when serving service workers, we can't use the service worker for images (/static/images) or css (/static/styles).

@molant
Copy link
Member

molant commented Feb 7, 2018

@qzhou1607 that's what I though initially but I remember reading somewhere that it was scoped to were the registration was happening or under. Can't seem to find the link.
Can you double check the behavior in both scenarios?

@qzhou1607-zz
Copy link
Contributor Author

@molant I tried having sw-reg.js in the root but keeping the service worker script in /static/scripts, the scope is still not right (doesn't work for files under /static/images or /static/styles). I also tried adding { scope: './' } during registration and I got this error message in the console:

Failed to register a ServiceWorker: The path of the provided scope ('/') is not under the max scope allowed ('/static/scripts/'). Adjust the scope, move the Service Worker script, or use the Service-Worker-Allowed HTTP header to allow the scope.
(anonymous) @ sw-reg-1f4be0a30f.js:1

It does seem the location of the service worker script matters.

@molant
Copy link
Member

molant commented Feb 7, 2018

It does seem the location of the service worker script matters.

😫

Can you then please take a look at https://jakearchibald.com/2016/caching-best-practices/ and make sure that we are doing the recommendations for Service Workers and how to access the resources (network first for HTML, cache first for static resources, and I don't know if something else)?

@qzhou1607-zz
Copy link
Contributor Author

@molant I read the blog again, we are definitely doing the right thing for accessing the resources, including:

  • Cache busting the assets (images, stylesheets, scripts), change the url if there are updates.
  • Always revalidate the HTML pages

It didn't mention anything about the right thing to do about sw-reg and service worker scripts themselves. I think what we do so far is correct, as long as the followings are satisfied:

  • Cache busting the sw-reg and sonarwhal-worker. If there is a change in the service worker, it's reflected in the file name.
  • Make sure to always revalidate the HTML pages linking to sw.reg.js. This way, the latest service worker is registered.
  • A max-age can now be set on the service worker and registration scripts, since updates are reflected by changes in the filenames.
  • Based on my investigation so far, we have to keep the service worker script in the root due to the scope. Alternatively, we can use the Service Worker Allowed header to override the scope like in this post. Or just figure out a way to change the cache control header of the service scripts in the root without moving them to /static/scripts.

@molant
Copy link
Member

molant commented Feb 7, 2018

I think I'd rather change the max-age than sending the extra header.

Is there a way where we can tell the SW to go to the network first for all .html requests and to serve the cache first for the rest? This should probably avoid the issues we've been having with content not being fresh.

Also, our links don't end up in .html so we need to make sure to be intercepting those requests and serve the right asset.

@qzhou1607-zz
Copy link
Contributor Author

@molant Currently the cache-control headers of the HTML pages are set to no-cache, with that the html content will always be revalidated with the server before serving from the cache, right? I don't think we need to control that from the service worker.

Is there a way where we can tell the SW to go to the network first for all .html requests and to serve the cache first for the rest? This should probably avoid the issues we've been having with content not being fresh.

Did you mean the links to sw-reg,js? I'm confused - why is it not in .html?

Also, our links don't end up in .html so we need to make sure to be intercepting those requests and serve the right asset

@molant
Copy link
Member

molant commented Feb 7, 2018

Did you mean the links to sw-reg,js? I'm confused - why is it not in .html?

E.g.:
We are always using /docs/user-guide/ but the value key we are using to store the HTML it in the cache is /docs/user-guide/index.html

with that the html content will always be revalidated with the server before serving from the cache, right? I don't think we need to control that from the service worker.

I'm almost sure the SW is going to intercept that request regardless of what the header says, otherwise how would offline experiences work?

@alrra alrra force-pushed the master branch 4 times, most recently from 4acc893 to fab9134 Compare February 19, 2018 11:57
@alrra alrra force-pushed the master branch 5 times, most recently from ea0e8b0 to 3a673ad Compare May 29, 2018 13:42
@molant molant force-pushed the master branch 4 times, most recently from d354419 to ed24f2d Compare August 8, 2018 20:22
@alrra alrra force-pushed the master branch 2 times, most recently from 0772f78 to cd1add7 Compare August 13, 2018 23:59
@molant molant force-pushed the master branch 2 times, most recently from 647506f to 1211334 Compare August 14, 2018 03:09
@alrra alrra force-pushed the master branch 5 times, most recently from a25daec to b373551 Compare August 14, 2018 06:25
@molant molant force-pushed the master branch 2 times, most recently from f070287 to b373551 Compare August 14, 2018 21:39
@alrra
Copy link
Contributor

alrra commented Aug 22, 2018

We don't have a SW anymore, so closing this for now.

@alrra alrra closed this Aug 22, 2018
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.

3 participants