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

Service Workers - Consolidated issues list #2554

Closed
6 tasks
ro-savage opened this issue Jun 17, 2017 · 34 comments
Closed
6 tasks

Service Workers - Consolidated issues list #2554

ro-savage opened this issue Jun 17, 2017 · 34 comments
Milestone

Comments

@ro-savage
Copy link
Contributor

ro-savage commented Jun 17, 2017

This is a consolidation of issues and fixes service workers in CRA.

For general discussion of service workers see issue #2398


Last updated: 18/06/2017 00:10:00 GMT

  • SWI1: Service workers are causing CRA to show up when someone hosts a non CRA app on the same address:host.

PR #2551 attempt to address this by auto reloading non-service worker apps.

  • SWI2 User sees old app when revisiting a site, they must refresh to see the updated site.

PR #2426 attempts to address by showing a toast notifying user to refresh.

  • SWI3 service-worker.js can be cached accidently or by default on some hosts, meaning that new versions of apps do not appear until the cache period ends.

Eventually browsers should force no-cache on service workers, but this is only implemented in firefox currently. In the mean time, only fix is to ensure no-cache is set on service-worker.js
SW Spec Issue, Firefox ✔, Chrome

PR #2563 is a proof of concept how this could be addressed in CRA.

  • SWI4 Browser support is currently patchy. Different browsers support different specs and there is no polyfills to align browsers, things means developers need to be aware of browser differences

There is currently no fix for this.

  • SWI5 Odd behaviour for the end users when an app is cached but no data is loaded, images don't load etc

There is currently no fix for this. Developers must know of this behaviour an plan for it. Due to the fact users aren't allowed to fully configure the service worker, they won't ever be able to cache API data and images (I think? @jeffposnick).

A developer can store data in localStorage and load that initially / if offline.

  • SWI6 When registerServiceWorker.js is updated to fix issues, it will not be updated for current users of CRA. Breaking the 'as long as you don't eject you always have the latest features'

Currently no PR but we could move the logic out of registerServiceWorker.js and add it to react-scripts config folder.


Please feel free to add any issues that you have encounter using CRA with service workers enabled.

@ro-savage
Copy link
Contributor Author

Pinging the experts - @jeffposnick @addyosmani @gaearon @geelen @sethbergman @goldhand

@ubershmekel
Copy link

The first address:host issue can manifest in 2 ways: when taking down the CRA app and replacing it entirely, vs when hosting multiple different apps on different url paths and only one of them is a CRA app.

@et304383
Copy link

The fact that service worker code can get cached is the single greatest reason not to enable them by default. The majority of developers won't realize they have to configure caching headers on the file to prevent browser caching the service worker logic and being unable to update their site in any way for end users.

Next top issue is caching index.html by default.

@sethbergman
Copy link

I found a project that I believe implements service workers fantastically. It happens to be using the Aurelia framework. The project is one I'm sure many of you are familiar with: gist.run.

I put together a gist of the main implementation of service workers in this project here.

On a side note, if you don't have the chrome extension Octotree installed, you must get it right away. It's incredible.

Why am I talking about this? I'm going to give you a link to the gist.run repo and you'll be able to navigate the file tree of the repo without reloading the page.

Here's some additional reading and resources for those in a bind:

This is a complex new piece of technology that is going to be great for mobile / offline resource caching, especially for those with low bandwidth. It's just going to take time for the browsers and API's to sync up to make it what it's really supposed to be. I'm going to look further into how gist.run implemented it with Aurelia and go from there. 🍻

@ro-savage
Copy link
Contributor Author

ro-savage commented Jun 17, 2017

I am wondering if we could implement a cache busting technique to solve some of these issues while browser support and the spec is still in flux.

Every time a new bundle is created, we are already creating a new service worker. The service worker could have its own hash. This would bust the cache and solve SWI3.

Then with a modified version of PR #2551, where we reload the page if no service worker is found, we could instead show an empty white page while a fetch request is done to the service worker.

  • if its found with the same URL as the current service worker saved in the browser. We just remove the white page and show the app.
  • If we are offline. Remove the white screen.
  • If its not found, or there is a new service worker, we unregister the old one and reload the page. (solves SWI1 and SWI2)

This would then solve SWI2 that the user sees old content. It also avoid them being told they need to refresh to see the latest content (currently odd behaviour for a normal browsing experience)

Showing a 'white screen' is standard browser behaviour when loading a page, so the use doesn't see anything strange. We also get any improvements in chrome where it may have a pre-parsed version of your app for faster loads.

The obvious disadvantage here is that you can't get 'instant loading' from the offline version of the app, because at least one request has to be completed (or fail) before the app is loaded.

I think this would be a good trade off between expected experience, solving developer issues, and solving caching issues.

However, I am not sure that we can actually find out the filename of the currently saved service worker. Any idea @jeffposnick?

If it's not possible, there might be another way to check if a service worker is the same or difference to the currently saved one.


Update Added a proof of concept on how this could be done. PR #2563

@sethbergman
Copy link

sethbergman commented Jun 18, 2017

Yeah that's what you want to do. It's essentially an app shell that renders first paint while other requests/ promises resolve behind the scenes. Polymer explains this in detail.

The decision comes to which assets to cache by default.
This is a good read. Service Worker Lifecycle

@jeffposnick
Copy link
Contributor

Thanks for your continued work exploring the pain points and potential solutions, @ro-savage!

I've been maintaining a similar document, heavily drawn from the experience of the create-react-app community, but with the idea of sharing this knowledge across the larger number of projects that have already adopted/are thinking of adopting offline-first by default.

I'll add in some comments/feedback to the new and existing PRs that you've linked up to.

(FWIW, it does look like there's been some movement on getting no-cache by default for the service worker script implemented in Chrome, hopefully in the Chrome 61 timeframe.)

@ro-savage
Copy link
Contributor Author

@jeffposnick that document looks good. I'll see if there is anything to add. And good news on the no-cache.

@jeffposnick @sethbergman Is there currently a way to tell if the registered service worker navigator.serviceWorker.register(swUrl) is the same as the previous service worker?

That is, we can tell if the service worker is new/updated from registration.onupdatefound but what if I want to confirm the sw that I just registered is the same as the current/previous service worker?

A noupdatefound listener or something?

The use case here, follows on from #2563. Which hides content until the app is sure the content has not been updated. The PR does this by giving each service-worker at unique file name, but it'd be better if there was another way to tell.

Secondly, is there a way to refresh the content the service-worker is showing on the browser other than reloading the page?
At the moment, if it finds an update it runs a document.location.reload() but it would be better if the updated service-worker could just reload/download (respecting cache-control) all the content in the new service-worker and execute them again.

(And even better if any file received from browser cache was not parsed again)

@petetnt
Copy link
Contributor

petetnt commented Jun 21, 2017

Hi @ro-savage,

just to be sure, would #2551 solve issues where one is serving two apps on the same URL but with different paths? For example I am serving a CRA app on https://myfoo.com and the docs in https://myfoo.com/docs, which are served by the same static server from different static servers. The app forces a reload on the CRA app if user navigates to https://myfoo.com/docs, but if the service-worker has been loaded /docs will serve the CRA app regardless?

Great work on improving the PWA experience on CRA by the way ❤️

@giladaya
Copy link

@ro-savage thanks for maintaining this list!
I'd also add the issue @Swizec brought up in #2398 with regard to assets from /public not being included in webpack asset list and thus not being accessible once the SW kicks-in:
#2398 (comment)

@stereobooster
Copy link
Contributor

stereobooster commented Sep 18, 2017

I'd also add the issue @Swizec brought up in #2398 with regard to assets from /public not being included in webpack asset list and thus not being accessible once the SW kicks-in:
#2398 (comment)

This issue is reported as #3137

Also there is this one #2340

@gaearon
Copy link
Contributor

gaearon commented Jan 8, 2018

@jeffposnick Could you please take another look at this list, note which issues are still relevant and which aren't, which have solutions (merged or unmerged) and which don't, and whether you know of any other issues based on feedback in #2398 and other threads?

@gaearon gaearon added this to the 2.0.0 milestone Jan 8, 2018
@jeffposnick
Copy link
Contributor

jeffposnick commented Jan 8, 2018

Here's an update on the various issues that I'm aware of, mentioned in this thread or elsewhere. Please ping this thread with anything I'm missing and I'll provide a similar update.

  1. Service workers are causing CRA to show up when someone hosts a non CRA app on the same address:host. This was fixed by Add simplified service worker invalidation #2551.
  2. User sees old app when revisiting a site, they must refresh to see the updated site. There is an open PR to add in a standard, user-visible message that would be displayed when there's an update available: Toast for Service Worker #2426
  3. service-worker.js can be cached accidentally or by default on some hosts, meaning that new versions of apps do not appear until the cache period ends. Not yet addressed in Chrome due to internal delays; already addressed in Firefox. Chrome bug: https://bugs.chromium.org/p/chromium/issues/detail?id=675540
  4. Browser support is currently patchy. Different browsers support different specs and there is no polyfills to align browsers, things means developers need to be aware of browser differences. No currently shipping browsers that support service workers require a polyfill to use the generated code. A number of browsers do not yet support service workers, but the two largest that don't, Edge and Safari, both have service worker support enabled in their Technical/Developer Previews.
  5. Odd behaviour for the end users when an app is cached but no data is loaded, images don't load etc. The service worker can't safely cache third-party data or images from remote domains by default, so this can still be an issue that would prevent some c-r-a apps from displaying all content while offline. (Flexibility around configuration, e.g. Allow runtimeCaching option without ejecting #3137, could help here, but runs counter to the c-r-a philosophy.)
  6. When registerServiceWorker.js is updated to fix issues, it will not be updated for current users of CRA. Breaking the 'as long as you don't eject you always have the latest features'. As per @ro-savage: Currently no PR but we could move the logic out of registerServiceWorker.js and add it to react-scripts config folder.
  7. Navigations to HTML documents outside of the webpack asset pipeline, like HTML files in public/ are incorrectly fulfilled with index.html. (See service worker swallows requests for files added to public/ #3008) There's an open PR for this (Remove the navigateFallback behavior from the generated service worker #3419), though implementing that has some trade-offs, discussed in the bug I've linked to.

Quick discussion: 3. remains a serious pain point for Chrome users, 2. has some potential for being fixed if developers are willing to adopt the UX introduced by that PR, and 7. can be addressed by the open PR (while losing some offline-first functionality).

It's worth noting that if the decision is made to disable service workers by default and making them opt-in, a fairly clean way of doing so would be to change

https://github.com/facebookincubator/create-react-app/blob/ce07e98bbd665524570f7b0f49317c4fc1b50500/packages/react-scripts/template/src/index.js#L8

to call unregister() by default (to clear out existing service worker registrations), with instructions to swap unregister() for registerServiceWorker() for the developers who choose to op-in. (This would not require changing the build process.)

@ro-savage
Copy link
Contributor Author

Thanks for the write up and feedback @jeffposnick.

I've been tracking and reading #2398, and the strong feedback that has appears has been to turn SW off by default.

As @jeffposnick suggested, having unregister() at the top with a comment about service workers feels like the logical thing to do. It makes it super easy for people to use SW when they want it but isn't on by default for people who aren't expecting it / don't want it.

It fixes all the issues that people encounter with it while allowing people to use it very easily, without configuration when they understand the implications or want to experiment.

SW should/can be used in certain circumstances but until that becomes the majority of what people are using React / CRA for it should be off by default because it interferes with standard practise and expectations of people building webapps.

@petetnt
Copy link
Contributor

petetnt commented Jan 9, 2018

👍

Bikeshedding, but as a developer something like

// index.js

// change this to true if you want to register a service worker (see https://[CRA docs])
registerServiceWorker(false);

// registerServiceWorker.js

export default function register(registerServiceWorker) {
  if ('serviceWorker' in navigator) {
    if (registerServiceWorker === false) {
      unregister();
      return;
    }

    if (process.env.NODE_ENV === 'production' ) {
       ...
    }
  }
}

Sounds like a neat way to solve the problem without breaking existing behaviour but giving an easy way to nuke the serviceWorker for those that upgrade with minimal code changes. I can send a PR for this if wanted 🐈

@gaearon
Copy link
Contributor

gaearon commented Jan 9, 2018

Is there some up-to-date information on Firebase caching of service-worker.js? It seems odd that it's a Google property but it's still (ten months in) causing issues (#3659) for service workers, a spec pushed by Google advocates.

@highmountaintea
Copy link

Is there a service worker configuration guide for CRA we can use to avoid the issues @jeffposnick so kindly compile? I presume most Chrome users are running into issues #2 and #3 with the default service worker configuration.

@jeffposnick
Copy link
Contributor

Re: @petetnt, I'm not sure if a false/true flag offers additional clarity vs. what unregister() would offer (with registerServiceWorker() present, but commented out). If the current unregister() name isn't clear, then the exported function could be renamed unregisterServiceWorker() instead.

Re: @gaearon, see #3659 (comment) regarding Firebase caching service-worker.js.

Re @johnfliu818, there's a lot of material in the README, though it might not be as visible as it could be: https://github.com/facebookincubator/create-react-app/tree/master/packages/react-scripts/template#making-a-progressive-web-app

@petetnt
Copy link
Contributor

petetnt commented Jan 9, 2018

@jeffposnick like I said, mostly just bikeshedding 😃 That said, unregister is a pretty bad name (unregister what?) and having

import { unregister } from './registerServiceWorker.js';

...
// some comment
unregister();

does feel like a pretty weird default, especially to someone opening the index.js file for the first time. If registerServiceWorker() is present but commented out, it would either need to be commented out from the import too (or explained that one should swap the import if commenting out the below line.
Plus renaming it to unregisterServiceWorker would mean either that it's a breaking change or requires exporting both unregister and unregisterServiceWorker.

Passing a boolean is backwards compatible, it's a single-to-zero word change for everyone using CRA and I guess (IMHO) it does bring more clarity.

/🚲 🏠

@piotr-cz
Copy link
Contributor

@petetnt
What about changing docs to

import { unregister as disableServiceWorker } from './registerServiceWorker.js';

// ...
disableServiceWorker()

@petetnt
Copy link
Contributor

petetnt commented Jan 10, 2018

Sure, that works too. The other points still stand though.

@anshul
Copy link

anshul commented Jan 12, 2018

For 3, how about cache busting using url params like this?

@jeffposnick
Copy link
Contributor

Re: @anshul's comment, cache busting the service-worker.js file is not feasible. See #2440 (comment)

@highmountaintea
Copy link

highmountaintea commented Jan 12, 2018

Debugging a cache problem is annoying, especially when you are not aware there is a cache turned on somewhere. It's the type of things you want for your worst enemies. I really think default service-worker to be off (instead of trying to fix it via cache busting) is the right choice.

On a lighter note :)
uncommenting registerServiceWorker - 10 seconds
commenting out registerServiceWorker - 10 seconds
making website work again after commenting out registerServiceWorker - 35,871 seconds
not spending countless hours figuring out what the hell is going on - priceless

@iansu
Copy link
Contributor

iansu commented Jan 16, 2018

I decided to try creating a PR to disable service worker by calling unregister with a call to registerServiceWorker commented out. However, after doing so I now agree with the points @petetnt raised above.

Normally I'm opposed to boolean function arguments but in this case it might be the best option. The main problem is that if you import both unregister and registerServiceWorker then one of those imports will be unused resulting in a lint error. You could remove one of the imports but then you also have to explain all of that in the comment/documentation about enabling the service worker.

@jeffposnick
Copy link
Contributor

I'm happy to defer to the approach preferred by folks closer to the create-react-app project's codebase and linting constraints!

@theenoahmason
Copy link

I gotta say, working with this service worker over the last year, on multiple CRA projects, that it's been harder to track and read all these issues and make sense of anything than it has been to work with the sw in the first place (which is saying a lot).

These awesome features have been dangling in front of our faces; and with it enabled by default, we try and work and sweat to even understand wtf is going on... because we aren't gonna remove these awesome features; oh no, we are good devs and we always get it working. right? Wrong, we just get so mad at CRA that our only solace is a cigarette and an espresso.

This service worker issue is a complete disaster.

@theenoahmason
Copy link

For people finding this looking to use a production build served by express without ejecting, in a project that serves an /api route for example, you will have to run 2 servers on 2 ports, one for your data, and one to serve the build of the app alone. the service worker will always hijack all routes if served from a root scope.

There is no way to configure the service worker to ignore routes without ejecting, as the relevant whitelisting options are not exposed in any way that I can see. Someone correct me if I'm wrong.

Without building and serving, there is no way to know or feel what errors this would cause, so keep this in mind and save yourself my extreme frustration

gaearon pushed a commit that referenced this issue Jan 19, 2018
* WIP disable service worker by default (#2554)

* Updated service worker registration

* Readd default export in registerServiceWorker.js

* Updated comments about using Service Worker

* Call it serviceWorker

* Nits
@gaearon
Copy link
Contributor

gaearon commented Jan 19, 2018

We’re moving forward with making service-workers opt in in the next major release.
They’re still readily available behind a single line change but won’t be registered by default.

#3817

I hope this is a satisfactory conclusion for most users. We can revisit this in a year or two.

@gaearon gaearon closed this as completed Jan 19, 2018
@jakearchibald
Copy link

@gaearon Am I right in thinking that there's no TODOs for the service worker spec here? updateViaCache is spec'd and ready to go.

@piotr-cz
Copy link
Contributor

@theenoahmason There is a #3029 to release your pains.

@theenoahmason
Copy link

@piotr-cz that seems reasonable, though very restricted. Seems to me the best solution would be to add a key to package.json not unlike proxy or homepage with an array of simple text routes using path-to-regexp pattern. I am not complaining however, I love this package and am extremely greatful for it ✊

I got around the issue by serving the built app on its own server, which works just fine.

@piotr-cz
Copy link
Contributor

@theenoahmason
When people look up to resolve problems related with service workers, they probably end up here as it comes up at the top of in search results and specific fixes as #3029 don't get enough support.

The CRA maintainers are hesitant to add new configuration properties to not complicate the package so adding an exclusion for /api path seems like reasonable all-case scenario.

BTW I'm too serving API from different domain then build.

akstuhl pushed a commit to akstuhl/create-react-app that referenced this issue Mar 15, 2018
* WIP disable service worker by default (facebook#2554)

* Updated service worker registration

* Readd default export in registerServiceWorker.js

* Updated comments about using Service Worker

* Call it serviceWorker

* Nits
@jeffposnick
Copy link
Contributor

From the original post:

SWI3 service-worker.js can be cached accidently or by default on some hosts, meaning that new versions of apps do not appear until the cache period ends.

It's bad form to comment on a closed issue, but... I wanted to let folks who have previously run into issues due to service-worker.js being served from the HTTP cache know that, starting with the upcoming Chrome 68 release, this will no longer be an issue.

You can read more at https://developers.google.com/web/updates/2018/06/fresher-sw

zmitry pushed a commit to zmitry/create-react-app that referenced this issue Sep 30, 2018
* WIP disable service worker by default (facebook#2554)

* Updated service worker registration

* Readd default export in registerServiceWorker.js

* Updated comments about using Service Worker

* Call it serviceWorker

* Nits
@lock lock bot locked and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests