sveltejs / rfcs

RFCs for changes to Svelte
274 stars 81 forks source link

Remove Service Worker support from Sapper #35

Closed benmccann closed 3 years ago

benmccann commented 4 years ago

Rendered

arxpoetica commented 4 years ago

Initially I was for this, but after some discussion w/ @pngwn I think I'm more inclined to just fix all the vexing problems around our current SWs setup, rather than tear it out all together.

The main thing that needs firm shoring up is controlled cache invalidation. Uncertain if timestamps are sufficient for this. Might need to be programmatic w/ a nonce flag. Dunno. SWs are certainly weird, which I think is basically (more) what this RFC is about. Fixing the weirdness.

TheComputerM commented 4 years ago

I think this should be fine as long as we include a tutorial (preferably in the sapper FAQ) on how to add a service worker.

jbg commented 4 years ago

@arxpoetica Fixing the SWs would be a great idea, but I strongly suggest removing them from the standard Sapper templates. Most people building a Sapper app don't need a SW, and they're a regular source of hard-to-debug problems. The very first thing I do when starting a Sapper app is rip out the SW.

arxpoetica commented 3 years ago

Regarding this statement in the RFP:

None or almost none of the core Sapper developers use service workers themselves.

Guilty. But I don't think that's a good reason to rip them out. Rather, I favor:

lukeed commented 3 years ago

I use SWs – just not Sapper's. Purposefully keep my SW usage relatively simple.

benmccann commented 3 years ago

@lukeed can you share an example of your service worker? I'd like to see what a better implementation would look like

lukeed commented 3 years ago

I plan to release an OSS app that would include a template for this. Currently I can't since they're tied up in private projects & not all that easy to extract at the moment... but 🔜

I haven't checked in on this thread in a while – do we have a definitive list as to why/what needs fixing?

benmccann commented 3 years ago

I tried to include a list of issues in the RFC: https://github.com/benmccann/rfcs/blob/rm-service-workers/0000-remove-service-worker.md#motivation

pngwn commented 3 years ago

If we remove the eager caching of all assets in the install step and do it selectively instead then half the issues with sapper's sw will go away. The DX with service workers isn't ideal but we can add a note in the docs about that.

Sapper's service worker implementation isn't ideal, I personally do use service workers (heavily in some cases) and am familiar with the APIs and common approaches. However, I don't really use Sapper. If I used it more, I would modify the SW substantially in those cases.

thgh commented 3 years ago

I would prefer to keep support, but disable the service worker by default. Vue nails this with the PWA plugin. For the record: I have also disabled service workers in production Sapper apps.

seanlail commented 3 years ago

Completely agree with @thgh and others here - disabled by default would be great.

Both Vue and React (at least CRA) have an opt-in strategy. https://cli.vuejs.org/core-plugins/pwa.html https://create-react-app.dev/docs/making-a-progressive-web-app/#why-opt-in

I quite like the Vue consideration around development too (see the noopServiceWorker).

Instead, in the development mode the noopServiceWorker.js is included. This service worker file is effectively a 'no-op' that will reset any previous service worker registered for the same host:port combination.

As mentioned, there are current issues around service workers in Sapper, example: https://github.com/sveltejs/sapper/issues/1661

So opt-in would be welcomed.

Perhaps a short-term "fix" is to document how to disable the current service worker implementation?

benmccann commented 3 years ago

Closing this since we've moved on from Sapper to SvelteKit