Service workers

[Rich-Harris]

everyone's favourite topic

[benmccann]

I sent an RFC to remove them, so my thoughts probably aren't a secret... 😄 But @pngwn suggested it's just the current template implementation that is poor and not service workers in general, so I'm hoping he can show us what a good one looks like or ideally update the one in the template to be a bit better default

[Rich-Harris]

yeah, i think it's important that they are supported in some form, since they do enable useful features (add to home screen, offline support, etc)

[benmccann]

Users should always be able to add service workers to an app. I guess my question is whether things like add to home screen make sense in the default template. I'm not sure it's a representative of the types of web apps that are most commonly built

[dummdidumm]

As with other things like TypeScript support, this could be solved by having some kind of Wizard which is run when doing npm init svelte. People would be prompted if they want a simple default Service Worker or not and the default is no.

[Rich-Harris]

Something to consider: https://twitter.com/jeffposnick/status/1324373178043584514

It adds non-trivial complexity — e.g. it means that SSR code needs to be loaded (and cached) in the client

[Rich-Harris]

Ok, been tinkering away for a bit and I have a proposal. First, some observations:

• The service worker API is just plain awkward. event.waitUntil and event.respondWith are finicky APIs — because event handlers must be synchronous, you can't use await. So you can't do this...

  self.addEventListener('install', async event => {
    const cache = await caches.open('v1');
    await cache.addAll(paths);
  });

  ...but must instead do this:

  self.addEventListener('install', event => {
    event.waitUntil(caches.open('v1').then(async cache => {
      await cache.addAll(paths);
    }));
  });

  Ugh. I'm all in favour of using the platform, but I'm definitely not averse to us fixing its flaws.

• I think service workers might have to be a prod mode only thing — I'm not sure how much sense they make in a world of unbundled dev servers, since you can't (for example) cache the files in your app unless you already have a build manifest. Moreover, you can't use import in a service worker, so the service worker itself needs to be built.

So I propose that we add an optional prod-mode-only service worker wrapper that, if present (i.e., if src/service-worker/index.js or similar exists), is automatically registered. It would export install, activate and fetch functions (and sync and push) that would wrap the underlying event calls but in a more ergonomic way. The default service worker, provided in the template, would always serve immutable app files from a cache, but everything else would be network-first (cache-first is the most common source of service worker woes). Other strategies, like stale-while-revalidate, would be easy to implement and could be covered in the docs.

First stab at a default service worker:

import { timestamp, files } from '$app/meta';

const app_name = `app-${timestamp}`;
const fallback_name = `fallback-${timestamp}`;

export async function install(event) {
  const app = await caches.open(app_name);
  app.addAll(files.filter((file) => file.immutable).map((file) => file.path));

  const fallback = await caches.open(fallback_name);
  fallback.addAll(files.filter((file) => !file.immutable).map((file) => file.path));
}

export async function activate(event) {
  for (const key of await caches.keys()) {
    if (key !== app_name && key !== fallback_name) {
      await caches.delete(key);
    }
  }
}

export async function fetch({ request }) {
  if (request.method !== 'GET' || request.headers.has('range')) return;

  const url = new URL(request.url);

  if (url.protocol === 'https:') {
    // always serve immutable app files from service worker cache
    const app = await caches.open(app_name);
    let response = await app.match(request);

    if (!response) {
      const promise = fetch(request);

      try {
        // for everything else, try the network first...
        response = await promise;
        if (response.ok && response.type === 'basic') {
          const fallback = await caches.open(fallback_name);
          fallback.put(request, response.clone());
        }
      } catch {
        // ...then fall back to the cache...
        response = await caches.match(request);
      }

      // ...or return the failed fetch
      return response || promise;
    }
  }
}

$app/meta would have to be service-worker-only, since files is partially derived from the build manifest. files is an array of objects like this:

[
  // built app files
  {
    path: '/path/to/_app/some-js-xyz123.js',
    type: 'application/javascript',
    size: 123,
    immutable: true
  },
  ...

  // stuff in `static`
  {
    path: '/path/to/potato.jpg',
    type: 'image/jpeg',
    size: 789,
    immutable: false
  },
  ...
]

Eager to hear thoughts on all of the above 🙏

[GrygrFlzr]

export async function fetch({ request }) {
  // ...
      const promise = fetch(request);

Wouldn't this just recursively call itself, since we've shadowed the fetch binding? It also seems confusing to name it fetch when it's not actually replacing window.fetch and has a different signature, though I get that it's supposed to correspond to the fetch event.

[Rich-Harris]

hah good catch! this is what happens when you try to do this at 1am. two solutions spring to mind:

export async function onInstall(event) {...}
export async function onActivate(event) {...}
export async function onFetch(event) {...}

(oninstall, onactivate etc appeal more to my aesthetic sense, but given onMount, onDestroy etc we probably need to embrace camel casing.)

Alternatively:

export default {
  install: event => {...},
  activate: event => {...},
  fetch: event => {...}
};

I much prefer the first — it's more amenable to static analysis, and better matches things like export function getContext and export function load.

Worth noting that by the very nature of how this would work (i.e. the Kit service worker runtime importing the user's module), the user could also do things the old fashioned way with self.addEventListener if they're sufficiently masochistic.

[GrygrFlzr]

I think onEvent is also a better solution - the second bandaids over the problem and inevitably someone's going to try export async function fetch without considering the shadowing.

[Rich-Harris]

Recapping discussion with @lukeed and @pngwn:

• the default behaviour (i.e. the service worker contained in the template) shouldn't cache everything, just the app files
• export function onInstall is a bit magical, and obscures the workings of the service worker. It would be better if the file the user writes is the entry point, and onInstall etc are (if used at all) imported from somewhere. (This also makes tree-shaking easier.)

If the default is to only cache the app files on installation, it probably makes sense to separate them out from the contents of static. In which case they can probably just be arrays of strings, which can be passed directly to cache.addAll.

$app/meta is a weird name for a module that exports onInstall etc, so perhaps we should just Ronseal it:

import { timestamp, build, assets, onInstall, onActivate, onFetch } from '$service-worker';

const name = `cache-${timestamp}`;

onInstall(async () => {
  const cache = await caches.open(name);
  cache.addAll(build);
});

onActivate(async () => {
  for (const key of await caches.keys()) {
    if (!key.includes(timestamp)) caches.delete(key);
  }
});

onFetch(async ({ request }) => {
  if (request.method !== 'GET' || request.headers.has('range')) return;

  const cached = await caches.match(request);

  if (build.includes(request.url)) {
    return cached;
  }

  if (url.protocol === 'https:') {
    const promise = fetch(request);

    try {
      // try the network first...
      const response = await promise;
      if (response.ok && response.type === 'basic') {
        const fallback = await caches.open(name);
        fallback.put(request, response.clone());
      }
      return response;
    } catch {
      // ...then fallback to cached response,
      // or return the failed fetch
      return cached || promise;
    }
  }
});

[GrygrFlzr]

Somewhat related thought - if the user having to export functions is magical, wouldn't that also apply to server routes (exporting get etc), preload (exporting load), and the setup file (exporting prepare and getSession)? Part of the difficulty in giving them types is also from the fact that they're effectively user-defined, unlike the onBlah imported functions.

[lukeed]

I had no problem with the exported functions. My only point regarding the API is that the use of these functions (hooks or exports, no matter) is that all it's doing is hiding the event.waitUntil wrapper code, but I'm fine with it 👍

Would prefer exported functions of the two.

[Rich-Harris]

I guess one distinction between these and load, getSession etc is that the presence of those functions alters the behaviour of Kit (i.e. if there's a load function, Kit calls it) whereas it doesn't really care what you do in your service worker (where onInstall(...) is really just sugar over self.addEventListener('install', ...). But moreover we don't have a choice in the case of load and it would make things a bit more awkward for prepare and getSession too.

Maybe I am overindexing on aesthetics though, and we should just make people use self.addEventListener.