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

Add a Service Worker with minimal asset caching to a default Ember CLI application #117

Open
wants to merge 2 commits into
base: master
from

Conversation

@martndemus

martndemus commented Mar 13, 2018

Rendered

Changelog:

2018-03-17

  • Added requirement that the Service Worker should only return cached responses if the network is unavailable.
  • Added a section about working in development mode.
  • Updated wording of Service Worker must be served on HTTPS to include http://localhost as an exception.
  • Dropped the killswitch part.
@mikkopaderes

This comment has been minimized.

mikkopaderes commented Mar 13, 2018

I think being able to configure additional assets to cache as part of the minimum should be included. Is there a reason why that was left out?

@bcardarella

This comment has been minimized.

Contributor

bcardarella commented Mar 13, 2018

I believe that #106 should be solved before this RFC is implemented. Having good ssl ergonomics will be essential to the dev on boarding of SW. Otherwise ember-cli is asking each dev to manage ssl on their own which is a nightmare

@martndemus

This comment has been minimized.

martndemus commented Mar 13, 2018

@bcardarella Service Workers don't require HTTPS on localhost. Meaning that you would only need said certificates if you run your Ember CLI app locally on a domain which is not localhost.

@martndemus martndemus changed the title from Create 0000-service-workers.md to Add a Service Worker with minimal asset caching to a default Ember CLI application Mar 13, 2018

The cached `index.html` file should be served by the Service Worker whenever a `fetch` event happens that has the request mode of `navigation` and the `accept` header of the request has `text/html` as value.
### Service Worker invalidation
By default each build should include a random sequence of text in the compiled output of `sw.js`, this way the Service Worker gets invalidated after every build.

This comment has been minimized.

@cibernox

cibernox Mar 13, 2018

Contributor

This random sequence is needed in production or only in development?

This comment has been minimized.

@martndemus

martndemus Mar 13, 2018

Also in production. You need this because your index.html file isn't fingerprinted.
An alternative would be for the script to contain a fingerprint of index.html.

This comment has been minimized.

@sdhull

sdhull Apr 28, 2018

@martndemus I attended your PWA training recently at EmberConf. The end of the training focused on cache settings at the CDN/webserver level because sw.js can't be fingerprinted and if it gets cached, users could end up getting stale assets for 24 hours after a deploy.

Does this somehow get around that limitation or would ember devs need to be careful about what gets cached in production?

@cibernox

This comment has been minimized.

Contributor

cibernox commented Mar 13, 2018

How does this play with ember-service-workers. Does this basically mean that ember-service-worker and ember-service-worker-asset-cache will it be part of the default blueprint or that they will be "merged into core"?

@martndemus

This comment has been minimized.

martndemus commented Mar 13, 2018

I intentionally left out the implementation details, but my intention is to add the required ember-service-worker addon and it's plugins as a part of the default blueprint.

EDIT: an alternative could be a 'good defaults' meta-addon, that packages said combo of addons with the correct configuration preset.

@knownasilya

This comment has been minimized.

Contributor

knownasilya commented Mar 13, 2018

Having good ssl ergonomics will be essential to the dev on boarding of SW. Otherwise ember-cli is asking each dev to manage ssl on their own which is a nightmare

I want to second what @bcardarella voiced, because now-a-days I find myself having to setup a local domain via etc/hosts either to handle multitenant apps with subdomains or for oauth testing.

It should also include a warning that a Service Worker must be served over HTTPS and served from the same origin to function correctly.
It might also be worth to add a few pointers on how to set up developer tools to improve the development experience with an active Service Worker.

This comment has been minimized.

@jrjohnson

jrjohnson Mar 13, 2018

We've been working with a service worker enabled app for a few months and have found most development tasks (particularly TDD) to be so painful that we all just run SW_DISABLED=true ember serve. While there are some workarounds; it shouldn't be underestimated how much complexity an extra cache layer can add to an active development process. Consider turning this off by default in non-production environments.

This comment has been minimized.

@martndemus

martndemus Mar 13, 2018

Instead of using SW_DISABLED=true most browsers have good dev tools to deal with this problem. For example in Chrome you can check Update on reload in the dev tools and it will will run the whole SW download/install/activate cycle before it loads the page.

This comment has been minimized.

@mike-north

mike-north Mar 13, 2018

Member

There are other considerations here -- for example, working with a variety of apps on localhost:4200, some of which are not PWAs. It's common for even very experienced developers, well versed in PWAs and the appropriate devtools best practices to regularly troll themselves.

This comment has been minimized.

@knownasilya

knownasilya Mar 13, 2018

Contributor

Yeah like the page is white because it loaded the SW for another app. Another reason I use etc/hosts to setup local test domains.

@rtablada

This comment has been minimized.

rtablada commented Mar 13, 2018

@martndemus I like the idea around this, but I worry about the invalidation and versioning complexity.

+1 for having an opt out that when disabled a script to deregister service worker.

@mikkopaderes

This comment has been minimized.

mikkopaderes commented Mar 13, 2018

After thinking about this. I'm on the side that this should be an opt-in thing. There's just too much GOTCHA around this to be turned on by default.

@piotrpalek

This comment has been minimized.

piotrpalek commented Mar 13, 2018

I agree, I think making it opt-in by using a generator shipped with Ember CLI would be a good approach with this.
After it had time to settle and shake out any potential bugs or DX downgrades it could be enabled in the next major Ember version.

@martndemus

This comment has been minimized.

martndemus commented Mar 13, 2018

but I worry about the invalidation and versioning complexity.

There's just too much GOTCHA around this to be turned on by default.

After it had time to settle and shake out any potential bugs or DX downgrades it could be enabled in the next major Ember version.

I hear your concerns, and I admit that there are some ways to troll yourself with Service Workers.
But I am very confident in that we can ship this minimal SW implementation without any major gotcha's, DX downgrades, etc.

If you have a specific concern, I'd love to hear about it and I'm sure there is a good solution for it.

By default each build should include a random sequence of text in the compiled output of `sw.js`, this way the Service Worker gets invalidated after every build.
### Kill switch
The build process should be able to take a flag that acts as a kill switch in case the Service Worker needs to be unregistered in the deployed environment as the result of a bad deploy.

This comment has been minimized.

@mike-north

mike-north Mar 13, 2018

Member

If we aim to make a killswitch part of the default setup, it's essential that we make its limitations and side-effects known.

  • A potentially unintuitive consequence of SW unregister: push subscriptions will be invalidated.
  • There are also some situations where a simple unregister is not enough to dislodge a misbehaving worker (i.e., someone has passed a forever-pending promise to ExtendableEvent#waitUntil)
  • What happens if worker v16 is installed in a user's browser, and they're not online when the killswitch is flipped. Are we forcing all apps down a path where skipWaiting() must be called? It's not desirable to deviate from the standard serviceworker lifecycle in some situations.

A robust kill switch is not trivial to build, and I have concerns about giving developers a false sense of safety by stating that this requirement is handled.

This comment has been minimized.

@eriktrom

eriktrom Mar 13, 2018

^^ Probably the most important comment on the thread.

It should also include a warning that a Service Worker must be served over HTTPS and served from the same origin to function correctly.
It might also be worth to add a few pointers on how to set up developer tools to improve the development experience with an active Service Worker.

This comment has been minimized.

@mike-north

mike-north Mar 13, 2018

Member

There are other considerations here -- for example, working with a variety of apps on localhost:4200, some of which are not PWAs. It's common for even very experienced developers, well versed in PWAs and the appropriate devtools best practices to regularly troll themselves.

* `assets/vendor.css`
* `assets/<app-name>.css`
The cached `index.html` file should be served by the Service Worker whenever a `fetch` event happens that has the request mode of `navigation` and the `accept` header of the request has `text/html` as value.

This comment has been minimized.

@mike-north

mike-north Mar 13, 2018

Member

Particularly for developers who are interested in server-rendered HTTP responses, caching index.html may be undesirable. Have you considered the more basic starting point of static assets only being cached? It doesn't leave us with an "offline first" starting point, but there's far less chance of interfering with dev workflows.

## How we teach this
The Ember CLI documentation should be updated with a notice that a default Ember CLI application registers a Service Worker and explain the functionality of said Service Worker.
It should also include a warning that a Service Worker must be served over HTTPS and served from the same origin to function correctly.

This comment has been minimized.

@mike-north

mike-north Mar 13, 2018

Member

Please reword to include the secure origin http://localhost. We do not want to tempt developers into using self-signed certificates if they don't have to

@mikkopaderes

This comment has been minimized.

mikkopaderes commented Mar 14, 2018

@martndemus - It's been a while since I've worked with PWA since the last one I was developing on got abandoned. But if I was remembering it correctly, the main problem was I was accessing my localhost from another mobile device to test out the app. So in my ipad/iphone/android/etc it was like http://<local_ip_address>:4200. I would always have to remove the cache on those devices to see my changes. I think private browsing solves it but I was using some library that wouldn't work on private browsers.

EDIT: I could've always turned off SW back then but on the chance that I forgot it, damn. Erasing just one specific cache is impossible at least on Android. I had to remove my whole browsing history for those devices.

@kylemellander

This comment has been minimized.

kylemellander commented Mar 14, 2018

I know that there is the polyfill for service workers that brings support up for browsers that do not support service workers yet, but I know for one of my use cases, we were using android/ios webviews for apps and the service worker polyfills did not work properly. I know that Apple recently added support for service workers on their mobile webview, but I just wanted to raise the concern about potential areas of lack of support that would cause issues with devs.

### Caching
The default Service Worker configuration should only cache the following files:
* `index.html`

This comment has been minimized.

@rwwagner90

rwwagner90 Mar 17, 2018

How will this default set be handled for non-standard apps? For example, if you are using prember, and not using SW, then this default gets turned on, it will troll you because what you want for prember is caching all the individual index.html files, and if it does the root one, you get it always trying to load the root page of your app.

@cibernox

This comment has been minimized.

Contributor

cibernox commented Mar 17, 2018

@kylemellander service workers are designed as progressive enhancements. If you're developing for a browser/webview that doesn't support them, all will continue to work as it does today. It's only an improvement for browser who do.

@martndemus

This comment has been minimized.

martndemus commented Mar 17, 2018

Updated according to various feedback. See OP for changelog.

@courajs

This comment has been minimized.

courajs commented Mar 18, 2018

I don't think this should be on by default. Currently if an app is designed with zero thought about offline support, it will simply not load while offline. This communicates clearly to a user that this app shouldn't be expected to work offline.

If a service worker is installed by default, I believe a lot of apps will still be designed with zero thought about offline support. But now, the app will appear initially appear to work offline to a user, before breaking in arbitrary and confusing ways as the developer's assumptions about connectivity are violated.

I think a default-installed service worker would lead to an overall more confusing world wide web.

I would not be opposed to adding a --service-worker or --offline-capable flag to ember new, since that would be opt-in.

@knownasilya

This comment has been minimized.

Contributor

knownasilya commented Mar 18, 2018

What about a default that displays an offline page?

@piotrpalek

This comment has been minimized.

piotrpalek commented Mar 18, 2018

We definitely should also consider that CRA is moving away from having service workers on by default. It's a strong signal that there were issues with it despite efforts to make it non invasive: facebook/create-react-app#2554

@simonihmig

This comment has been minimized.

simonihmig commented Mar 19, 2018

I would also ask not to do this by default. Mainly because of my concern that users unaware of it could be trolled by its side effects. Although I had checked this reload checkbox in Chrome, at least once Chrome was messing things up, having a list of SW registrations queued for activation, but nothing ever happened. So my browser was only seeing stale assets, everything I changed in my project did not reflect in the runtime. It took me a while to figure this out, which required me to manually unregister the SW and clear all site data. And this although I knew we had a SW in place. Now imagine someone isn't aware of this at all, and how to fix it!

@martndemus I know the recent strategy change (cache-fallback instead of cache-first) would mitigate that problem, but I am not sure this is a good default. I think in production cache-first still makes sense. But for dev, we actually disable SW all together by default (i.e. make in opt in, to work with SW only if you're actually working on SW related things). But this kind of setup would probably be even more dangerous if you are not fully aware of it.

tl;dr Adding a SW should be a conscious choice by the user!

@ddoria921

This comment has been minimized.

ddoria921 commented Mar 19, 2018

I think this feature will end up backfiring new users who might not expect this behavior from the very beginning. It can lead to a frustrating first experience with Ember since you'll have to check the reload box in Chrome for changes to take effect.

I don't think this should be on by default, but I would like to see it available as part of a flag when you're starting a new project. Like @simonihmig said, it should be a conscious choice by the user. By having it as a flag on setup, a user would benefit by out-of-the-box support for service workers, but only after explicitly opting into it.

@kellyselden

This comment has been minimized.

Member

kellyselden commented Mar 19, 2018

We could start thinking about a ember new --interactive with a yeoman style list of choices. This is where our "approved defaults" that take additional thought and configuration could live. This could also answer the what to do with ember-data question.

@simonihmig

This comment has been minimized.

simonihmig commented Mar 19, 2018

We could start thinking about a ember new --interactive with a yeoman style list of choices.

This is exactly the kind of idea I had in my mind for quite some time. Just didn't find the time yet to dive deeper into it, not to speak of writing a RFC. Something like...

$ ember new --interactive
What kind of project do you want to create?
● default
○ minimal
○ PWA
○ SSR (FastBoot)
○ Native Mobile (Cordova)
○ Native Desktop (Electron)

It's a bit off-topic here, but could indeed solve the issue of supporting more opinionated stuff!
Happy to help push this idea forward, if that sounds reasonable!?

@btecu

This comment has been minimized.

btecu commented Mar 19, 2018

@simonihmig Oldest pull request open was doing something related. It never got completed:
ember-cli/ember-cli#4709

@knownasilya

This comment has been minimized.

Contributor

knownasilya commented Mar 19, 2018

@simonihmig #7

@eriktrom

This comment has been minimized.

eriktrom commented Mar 19, 2018

per @kellyselden's comment

We could start thinking about a ember new --interactive with a yeoman style list of choices

https://workboxjs.org/ is worth looking into further. It's basically what we want now, and gives power users more control, without a foot gun for new users. We could hook into their api instead of inventing our version, but with perhaps nicer conventions, a la --interactive for ember apps.

It's specifically designed to deal with many of the worries/concerns/excitement around this web feature.

just a thought

@eriktrom

This comment has been minimized.

@mike-north

This comment has been minimized.

Member

mike-north commented Mar 20, 2018

There are still some seriously rough edges around service workers. I know of an internet-scale web app that's having to temporarily roll back serving up a SW to chrome users due to this bug which causes IPC between frames to blow up (Chrome 65): https://bugs.chromium.org/p/chromium/issues/detail?id=822145

While the idea of having a "pwa by default" or "offline first" experience is enticing, there's a whole lot of foot-gun factor involved today, some of which can prevent affected users from accessing a domain entirely for 24 hours or more. I really want to see us get there -- we just have to decide how many rough edges are acceptable.

@oskarrough

This comment has been minimized.

oskarrough commented Mar 20, 2018

tl;dr Adding a SW should be a conscious choice by the user!

Fully agree, considering the gotchas that are still here.
Why not tell people to ember install ember-service-worker instead?

@mikkopaderes

This comment has been minimized.

mikkopaderes commented Mar 20, 2018

@oskarrough although I do agree that SW shouldn't be on by default, I do would like to get built-in support for SW should we opt-in. That's one less package to worry about when maintaining our app dependencies.

@NullVoxPopuli

This comment has been minimized.

NullVoxPopuli commented Jun 3, 2018

I think a default service worker implementation needs to have a way to auto-detect updates and prompt the user for refresh. There is an existing ember-server-worker addon for this out there, but it doesn't work 'automatically' (at least for https://github.com/NullVoxPopuli/emberclear anyway...).

been trying this hack:

    setInterval(() => {
      navigator.serviceWorker.getRegistration()
        .then(registration => registration && registration.update());
    }, 60000);

which still doesn't quite get me there.

See: topaxi/ember-service-worker-update-notify#2

# Add a Service Worker with minimal asset caching to a default Ember CLI application
## Summary
This RFC introduces a Service Worker to the default app blueprint. It will only cache the most essential assets, such as `index.html`, and the vendor and app specific JS and CSS files, it will only respond with those assets if getting the asset over the network fails.

This comment has been minimized.

@ASH-Michael

ASH-Michael Jun 6, 2018

How would this work for apps that do not use the index.html file that was output in the /dist folder in production, and instead just point to the assets from a server rendered page? Would the asset cache list be configurable by environment?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment