-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 support with service worker #978
base: main
Are you sure you want to change the base?
Changes from 7 commits
85a014b
1bd6f2d
ef6d78b
2d6207d
2936fad
53458ba
49ca43f
39b7233
3e0ba2e
e8ff8b1
c79524c
1e75370
ae14615
5ce2a31
ac3f4dc
25e916d
c8f206f
d32e376
687842e
7c90808
6d557eb
18d2580
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
#offline-indicator { | ||
text-align: center; | ||
padding: 5px; | ||
} |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
--- | ||
--- | ||
|
||
importScripts('/scripts/sw-toolbox.js'); | ||
|
||
const VERSION = 1; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How will you make sure that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes sure. We could use the |
||
|
||
toolbox.cache.name = "Babel-Cache-" + VERSION; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see anything here that will delete old caches once this cache name changes. That means that you'll end up with many copies of your full site in the SW site, each time you change the name. Normally, you'd want to clean up unneeded caches inside an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 to Jeff's comment. You'll want to iterate through your cache entries and // Where urlsToCacheKeys is a map of your items to cache
self.addEventListener('activate', function(event) {
let setOfExpectedUrls = new Set(urlsToCacheKeys.values());
event.waitUntil(
caches.open(cacheName).then(function(cache) {
return cache.keys().then(function(existingRequests) {
return Promise.all(
existingRequests.map(function(existingRequest) {
if (!setOfExpectedUrls.has(existingRequest.url)) {
return cache.delete(existingRequest);
}
})
);
});
}).then(function() {
return self.clients.claim();
})
);
}); There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rather than cleaning manually the cache, I think we need to limit the cache using |
||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It'll be good to use a versioned cache name. const VERSION = 1;
toolbox.cache.name = "Babel-Cache-" + VERSION; There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, toolbox.cache.maxEntries = 20 // or 15 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I will add cache versioning. What do you think about a max age ? Like 7 days ? I don't know if limiting entries is a good idea. I pre-cache already all pages. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TL for sw-toolbox and sw-precache here. Great to see you using it :) If you're going to set I do think there's use in imposing a maxAgeSeconds age. @boopathi can probably comment to the length of time, but a week doesn't sound like a bad ballpark. |
||
var preCachedRessources = [ | ||
{% for page in site.pages %} | ||
'{{ page.url }}', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you're precaching entire HTML documents, then you're going to end up populating user's caches with a lot of duplicated data. I.e. if your HTML shares common headers/footers (because they're all using some underlying layouts) then the structure of those headers and footers will be take up space on your user's disk when they don't actually add any extra value. The ideal implementation would just cache the layout elements once, then cache the underlying content once, and then perform the templating logic in the service worker to combine them at runtime. I've got a very experimental example of this implemented at https://github.com/jeffposnick/jeffposnick.github.io/tree/work/src, but I don't know that it's stable enough to recommend using for a high-traffic site. |
||
{% endfor %} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure if the following is valid. I see this to be valid at-least for page transitions from and to REPL. The Service Worker shouldn't change from pageA to pageB if both the pages are under the scope of the same service worker. This is because, service worker updates based on the byte diff, and during a page change, a new service worker will install (if the resources are different -> i.e new There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since the strategy is |
||
]; | ||
|
||
toolbox.precache(preCachedRessources); | ||
|
||
toolbox.router.get('/*', toolbox.cacheFirst); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is super broad. Are you sure you're okay with every URL under your origin being served There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wanted an offline documentation. This caching strategy might indeed not be the best approach. |
||
toolbox.router.get('/*', toolbox.cacheFirst, { origin: "cdnjs.cloudflare.com" }); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You'd probably want to configure a different cache name here the runtime CDN caches, so that they're reused indefinitely and don't become irrelevant whenever you change the default If you're using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes good point. This is part of good practice I guess. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I checked, resources from CDN (Cloudflare and jsdelivr) have a version in the URL. |
||
toolbox.router.get('/*', toolbox.cacheFirst, { origin: "cdn.jsdelivr.net" }); | ||
toolbox.router.get('/*', toolbox.cacheFirst, { origin: "unpkg.com" }); // for repl | ||
toolbox.router.post('/*', toolbox.cacheFirst, { origin: "algolia.net" }); // Cache Algolia search response | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I mentioned this in your earlier commits, but is the goal here to always return a cached post response from Algolia or to only have this work when the network is available? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wanted to keep search even once offline. The goal here is to cache some responses from Algolia because it does not support offline searching. I'm not sure about this since the user will be able to find what he already searched online. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @addyosmani I guess not. it should be networkFirst. @xtuc There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't know exactly what There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I'm now thinking that idea wasn't that good. (#978 (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.
Mostly leaving here as a note in case helpful later :)
The navigator.{online/offline} events may not accurately indicate that you can or can't access the network. There are well documented gotchas where you might need additional means to check that you're really online. One is checking connection loss by making failed XHR requests (retry a request a few times, if it doesn't go through, you're definitely offline).
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.
@addyosmani flipkart is awesome !