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

Implement service worker and add PWA manifest #372

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions numbat-wasm/www/index.html
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
<meta property="og:url" content="https://numbat.dev/">
<meta property="og:description" content="High precision scientific calculator with full support for physical units.">
<title>Numbat - scientific calculator</title>
<link rel="manifest" href="numbat.webmanifest">
<link href="https://fonts.googleapis.com/css?family=Exo+2%7CFira+Mono:400,700&display=swap" rel="stylesheet">
<link href="terminal.css" rel="stylesheet">
<link href="main.css" rel="stylesheet">
Expand Down
22 changes: 21 additions & 1 deletion numbat-wasm/www/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -110,11 +110,31 @@ function setup() {
});
}

// TODO: I would prefer to use const lambdas rather than `function` in order to
Copy link
Author

Choose a reason for hiding this comment

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

I don't plan to merge this comment as-is but to remove it or update this file based on feedback. I'm not confident enough in my JavaScript abilities to use function in general unless I'm implementing generators, where it's strictly required. 🙂

Copy link
Owner

Choose a reason for hiding this comment

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

Not sure I can help here, sorry.

Copy link
Contributor

Choose a reason for hiding this comment

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

I offered a suggestion in my review.

// avoid unexpected `this` resolution. Not sure if there's a reason this is
// currently being used along with `var`. Those "old school" techniques do
// hoist, but this can easily be worked around by reordering declarations.
async function registerSw() {
if ("serviceWorker" in navigator) {
try {
// NOTE: The service worker URL should be stable between releases or this
// could lead to unexpected behavior.
await navigator.serviceWorker.register("./service-worker.js");
} catch (error) {
// NOTE: We don't allow the error to propagate because the app should
// still work without offline capabilities.
console.error("Failed to register Service Worker:", error);
}
} else {
console.warn("Service Workers not available in this browser");
}
}

Comment on lines +113 to +132
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// TODO: I would prefer to use const lambdas rather than `function` in order to
// avoid unexpected `this` resolution. Not sure if there's a reason this is
// currently being used along with `var`. Those "old school" techniques do
// hoist, but this can easily be worked around by reordering declarations.
async function registerSw() {
if ("serviceWorker" in navigator) {
try {
// NOTE: The service worker URL should be stable between releases or this
// could lead to unexpected behavior.
await navigator.serviceWorker.register("./service-worker.js");
} catch (error) {
// NOTE: We don't allow the error to propagate because the app should
// still work without offline capabilities.
console.error("Failed to register Service Worker:", error);
}
} else {
console.warn("Service Workers not available in this browser");
}
}
const registerSw = async () => {
if ("serviceWorker" in navigator) {
try {
// NOTE: The service worker URL should be stable between releases or this
// could lead to unexpected behavior.
await navigator.serviceWorker.register("./service-worker.js");
} catch (error) {
// NOTE: We don't allow the error to propagate because the app should
// still work without offline capabilities.
console.error("Failed to register Service Worker:", error);
}
} else {
console.warn("Service Workers are not available in this browser.");
}
}

var numbat;
var combined_input = "";

async function main() {
await init();
await Promise.all([init(), registerSw()]);
Copy link
Author

Choose a reason for hiding this comment

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

I went back and forth as to whether this should be done in-line at init() time or during the load event as is often suggested. My initial thought was to register() and claim() first in order to take advantage of ServiceWorker-cached artifacts. However, it looks like the SW "prefetching" can actually fall back to the browser cache if we allow the page to load first. This makes me think we should maybe wait until both the load event has fired and the Numbat terminal has initialized before registering. On the other hand, this is a pretty minor detail given how large the WASM payload is at the moment and the fact that most users will be on modern devices with a good connection.

Copy link
Owner

Choose a reason for hiding this comment

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

I am mainly concerned about further delaying the time until a user can type something into the terminal. Due to the large WASM size, this is already a long wait time, unfortunately — you're right. If registerSw is negligible in comparison, I'm okay with either way.


setup_panic_hook();

Expand Down
24 changes: 24 additions & 0 deletions numbat-wasm/www/numbat.webmanifest
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
{
"name": "Numbat",
"short_name": "Numbat",
"description": "high precision scientific calculator with full support for physical units.",
"icons": [
{
"src": "numbat-16x16.png",
"sizes": "16x16",
"type": "image/png"
},
{
"src": "numbat-32x32.png",
"sizes": "32x32",
"type": "image/png"
},
{
"src": "numbat-196x196.png",
"sizes": "196x196",
"type": "image/png"
}
],
"start_url": ".",
"display": "standalone"
}
181 changes: 181 additions & 0 deletions numbat-wasm/www/service-worker.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,181 @@
// The prefix is necessary if multiple web apps will be hosted at the same
// origin, e.g., as would be done on GitHub Pages. This prevents separate apps
// from clashing when it comes to origin-scoped resources such as Caches, OPFS,
// etc.
const APP_PREFIX = "numbat_";
// This should be updated any time cached assets change. It allows the entire
// set of precached files to be refreshed atomically on SW update.
// TODO: It would be nice if we did this automatically whenever a commit affects
Copy link
Author

Choose a reason for hiding this comment

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

I guess this could be done in the deploy or build script, but it's not clear under what conditions exactly we should generate a new version. I think in that case we would want a single source of truth for all self-hosted artifacts (and remote URLs), such as a local JSON file. Then the local script could check the file hashes for changes and the service worker could fetch that JSON to determine the lists of files.

Copy link
Owner

@sharkdp sharkdp Feb 27, 2024

Choose a reason for hiding this comment

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

I mean.. unless we change something completely unrelated like the docs, which are currently not part of this PWA, almost everything would require a VERSION_UUID change, right?

Could we simply update it every time we use the deploy.sh script to push a new version/state to numbat.dev?

Copy link
Author

Choose a reason for hiding this comment

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

I was originally thinking that we could put this into the deploy script, but the more I think about it the more sense it makes to use deterministic hashing of all built artifacts (i.e., anything that would get pushed via the deploy script). See my other comment in the main thread.

// cached files. Note that this could also be a sequence number or similar.
// Random UUIDs just make it easier to avoid collisions without coordination.
const VERSION_UUID = "819e71eb-c3df-46b8-885e-5650feaea241";
const CACHE_NAME = `${APP_PREFIX}${VERSION_UUID}`;

// NOTE: If these lists get very large, they should be moved into Sets for
// membership tests.

// Files atomically pre-cached at installation/update.
// NOTE: We do _not_ cache the service worker itself.
// TODO: Consider self-hosting Google fonts and caching. Alternatively, we could
// cache the remote fonts directly.
const PRECACHED_FILES = [
"./",
"index.html",
"index.js",
"jquery.min.js",
"jquery.mousewheel-min.js",
"jquery.terminal.min.js",
"keyboardevent-key-polyfill.js",
"main.css",
"opensearch.xml",
"pkg/numbat_wasm.js",
"pkg/numbat_wasm_bg.wasm",
"pkg/package.json", // Does this need to be cached?
"terminal.css",
];
// Files cached on-demand (at first load), but not refreshed. This should mostly
Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for writing these comments, but I'm still not sure I understand the difference between PRECACHED and CACHED.

Copy link
Author

Choose a reason for hiding this comment

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

PRECACHED files are downloaded deterministically before the first request to the service worker, since we know they will be used. CACHED files are cached on-demand at first request but then not updated thereafter (until the next app release, via service worker update). This helps save space because certain user agents will download different subset of optional assets, such as icons. This would be more pronounced if we have more icon sizes, but I've tested it in different browsers and they do in fact request different icon sizes and formats. There's no way to tell in advance which subset a given client might want.

I wasn't sure how to name this. It's not the same as "dynamically cached", since that refers to files whose contents are expected to change with every request (currently the exchange rate data).

Copy link
Owner

Choose a reason for hiding this comment

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

Thank you.

If we're only talking about downloading a few icons or not, I think I'd go with a simplified approach where everything is PRECACHED, if that makes sense?

I'm mainly thinking about the maintenance in the future. When new assets are added by contributors, they would not have to understand this distinction.

Copy link
Author

Choose a reason for hiding this comment

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

Agreed. Especially in the case of auto-generation, this is simpler and more robust if everything is precached.

// include platform/device-specific resources such as icons which take up a lot
// of space and not required across the board. These should also not typically
// contain resources which would lead to semantic breakages if out of sync with
// other files.
const CACHED_FILES = [
"icon.svg",
"numbat-16x16.png",
"numbat-196x196.png",
"numbat-32x32.png",
"numbat.png",
"numbat.svg",
// TODO: Google Fonts? Self-host and pre-cache those?
];
// URLs for which we hit the internet on every request, only falling back to the
// cache when offline.
const DYNAMIC_URLS = ["https://numbat.dev/ecb-exchange-rates.php"];

const getAbsoluteUrl = (file) => new URL(file, self.location.href).href;

const PRECACHED_URLS = PRECACHED_FILES.map(getAbsoluteUrl);
const CACHED_URLS = CACHED_FILES.map(getAbsoluteUrl);

const handleInstall = async (e) => {
e.waitUntil(
(async () => {
// Atomically pre-cache common assets. This is technically racy because
// there's no coordination between SW version id and cached file
// contents (that would require something like asset hashes). However,
// the time window for such a race is pretty narrow and unlikely to
// arise in practice.
const cache = await caches.open(CACHE_NAME);
await cache.addAll(PRECACHED_FILES);
console.log(`[Service Worker] installed version ${VERSION_UUID}`);
})()
);
};

const handleActivate = async (e) => {
e.waitUntil(
(async () => {
// Drop old cache versions so storage doesn't explode. By doing this
// only at activation time, we ensure that older versions of the app
// continue to work until the new one takes over.
const keys = await caches.keys();
const deletions = [];
for (const key of keys) {
if (!key.startsWith(APP_PREFIX)) {
// Only touch caches related to this app, even if they're at the
// same origin.
continue;
}
if (key != CACHE_NAME) {
// Only keep the most recent cache version.
deletions.push(caches.delete(key));
}
}
// NOTE: It might be excessive to fail activation on cleanup of old
// caches, but something has clearly gone wrong in this case.
await Promise.all(deletions);
})()
);
};

const handleFetch = async (e) => {
const url = new URL(e.request.url).href;

const servePrecachedFile = async () => {
const cache = await caches.open(CACHE_NAME);
// NOTE: It may be possible for our caches to be evicted, in which case
// we'd need to re-fetch. On the other hand, this also breaks the atomic
// update semantics and could lead to mismatched asset versions and
// subtle breakage.

// NOTE: The await isn't strictly necessary here thanks to Promise
// coalescing, but this is less error-prone if we add more logic below.
// Same applies elsewhere.
return await cache.match(e.request);
};

const serveCachedFile = async () => {
const cache = await caches.open(CACHE_NAME);
const cachedResponse = await cache.match(e.request);
if (cachedResponse) {
// Serve the cached resource and don't attempt to refresh.
return cachedResponse;
}
// The requested icon was not in the cache. Fetch it and return it, but
// also add it to the cache for later.
let fetchedResponse;
try {
fetchedResponse = await fetch(e.request);
} catch (error) {
console.error(
`[Service Worker] failed to fetch cached resource at ${url}`,
error
);
}
if (fetchedResponse && fetchedResponse.ok) {
// Asynchronously extend this event handler but don't wait to return
// the response. We also don't cache failure values.
e.waitUntil(cache.put(url, fetchedResponse.clone()));
}
return fetchedResponse;
};

const serveDynamicUrl = async () => {
// We want fresh data for this resource. Try to hit the internet first,
// falling back to cache on failure.
let fetchedResponse;
try {
fetchedResponse = await fetch(e.request);
} catch (error) {
console.error(`[Service Worker] error fetching ${url}`, error);
}
if (fetchedResponse && fetchedResponse.ok) {
e.waitUntil(cache.put(url, fetchedResponse.clone()));
return fetchedResponse;
}
console.error(
`[Service Worker] failed to fetch dynamic resource at ${url}; falling back to cache`
);
return await cache.match(e.request);
};

if (PRECACHED_URLS.includes(url)) {
// Respond with precached asset. We don't hit the internet for these
// until the app is next updated.
e.respondWith(servePrecachedFile());
return;
}

if (CACHED_URLS.includes(url)) {
e.respondWith(serveCachedFile());
return;
}

if (DYNAMIC_URLS.includes(url)) {
e.respondWith(serveDynamicUrl());
return;
}
};

self.addEventListener("install", handleInstall);
self.addEventListener("activate", handleActivate);
self.addEventListener("fetch", handleFetch);
Loading