Extract service worker registration code

[gaearon]

Judging by #2551 and #2426 we're going to make more changes to SW registration code. I think it means we should move a bulk of the implementation into the library but expose the necessary hooks for 80% use cases (e.g. for showing a custom toast).

How this could work:

import registerServiceWorker from 'react-scripts/registerServiceWorker';

registerServiceWorker({
  // some options?
});

On ejecting, we'll replace react-scripts reference with react-dev-utils. The file will exist as an extra entry point in both packages, so it will work both before and after ejecting.

Note: this also means react-scripts will move to dependencies from devDependencies. This actually seems fine and reasonable to me. The distinction doesn't really make sense anyway.

The exact naming is up for debate.

[gaearon]

cc @viankakrisna @ro-savage would either of you be interested in tackling this?
We can then reevaluate #2426 or alternatives.

[viankakrisna]

Played around with it on https://github.com/viankakrisna/create-react-app/blob/ServiceWorkerUtils/packages/react-dev-utils/ServiceWorkerUtils.js

Found a few questions:

1. It's outside ./src and written with es6 so UglifyJS will choke on npm run build
   Choice of solutions:
   a. setup build tools for it to compile to es5 in react-dev-utils
   b. setup build tools for it and let lerna manage it like react-error-overlay (maybe name it react-service-worker-utils, but this module is quite general purpose, so react prefix may not be accurate)
   c. add an exception in webpack config for this file in react-dev-utils
   d. rewrite it in es5

2. Changing react-scripts to react-dev-utils on eject -> would need to check all .js file in the ./src and replace react-scripts to react-dev-utils -> scary, but doable -> slow in big project?
   We can't be sure where the user import this module. Probably it's better to explicitly require a separate react-service-worker-utils module.

3. Isn't providing a runtime dependency is out of scope for CRA?
   I still stand by my comment in Why was service worker merged into create react app? #2398 (comment). It's a userspace area...

[gaearon]

a. setup build tools for it to compile to es5 in react-dev-utils

If you don't mind, a separate PR that moves everything to src but compiles it to the top level folder of react-dev-utils would be really handy. It's a bit exhausting to keep track of which syntax is safe to use there.

[gaearon]

Changing react-scripts to react-dev-utils on eject -> would need to check all .js file in the ./src and replace react-scripts to react-dev-utils -> scary, but doable -> slow in big project?

That's a good point. I haven't thought about that. I'm not sure.

Isn't providing a runtime dependency is out of scope for CRA?

Kind of.. But the boundary is a bit blurred here. I was hoping to cheat a little bit.

[viankakrisna]

how about option 1b? react-dev-utils is strictly for the node, webpack, and development tools related utility module, anything else goes to their own separate package. The init script needs to be updated to add react-service-worker-utils as a dependency though.

[gaearon]

The init script needs to be updated to add react-service-worker-utils as a dependency though.

That's the part I'm not very happy about. If we can find any other useful stuff to go there though, maybe it's okay.

[ro-savage]

@viankakrisna option 1d seems to be easiest, if writing in es6 is causing issues.

Should be easy enough to do, happy to re-write it if you need.

[viankakrisna]

@ro-savage In the long run I think setting up build tool for react-dev-utils or registerServiceWorker as a separate top level package is better. The browser that supports service-worker should support parts of es6 too right? we can use babel-preset-env for that.

@gaearon Another option is to set registerServiceWorker as a global function, so we don't need to import it from src. No react-scripts to react-dev-utils renaming on eject.

[piotr-cz]

The options could be callbacks for onupdatefound (executed here) and oninstall (executed here)

[piotr-cz]

IMHO library should be imported inside application

import registerServiceWorker from './registerServiceWorker';

export default class App {
  constructor(props) {
    registerServiceWorker({
      onUpdate: this.handleServiceWorkerUpdate
    })
  }

  handleServiceWorkerUpdate(registration) {
    if (window.confirm('New update has been installed, click to restart')) {
      window.location.reload()
    }
  }
}

[stereobooster]

I have side question about UX of New update has been installed, click to restart. Can we came up with something more friendly? Maybe:

• reload code when user navigates to the next page
• reload code in the manner of hot module reload Proof of concept: simple hot reloading #2304
• other ideas?

[piotr-cz]

@stereobooster I think update notification implementation should be up to developer. For example I'm showing special icon when update is detected and user may click on it to reload page into new state.

This problem should be really separated into two pull requests:

• Providing callbacks in registerServiceWorker.js file
• Adding an callback notification (Toast) that may be customized by developer

More info in here: #3375

[gaearon]

Meh. I’m fairly happy with what we have now and don’t think extracting it is all that useful. People who want service workers probably want more control anyway.