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

Add support to customize the Service Worker with InjectManifest and Workbox v5 #8485

Closed
wants to merge 19 commits into from

Conversation

marlonmleite
Copy link

@marlonmleite marlonmleite commented Feb 14, 2020

Description

This PR originated from discussion #7966 and many issues related to Service Workers and customizations in the CRA to configure the Workbox plugin over the past last year.

With this feature after compiling the Service Worker, the compiled features are identical to the existing one. The conversion of the existing functionalities in GenerateSW to InjectManifest was carried out, respecting all existing standard configurations currently.

Why Workbox v5?

We have also taken advantage of the momentum and migrated to v5.0.0 from the Workbox, because for build the service worker with webpack, environment variables and more are only available in v5. See more.

What features?

Now, it will be possible to make any customized configuration in the Service Worker and thus make it possible to use several functionalities at the service worker level: #7966 (comment)

Migrate

To update your current react-scripts to this new version, this not contain a break change.
To use the configuration using InjectManifest you will need to create the sw-template.js or sw-template.ts file in your application's src/ directory.

Local test

Checkout in this branch, run the installation and bootstrap scripts and run the commands:

with cra-template:

yarn create-react-app ../sw-app --template file:./packages/cra-template

with cra-template-typescript:

yarn create-react-app ../sw-app --template file:./packages/cra-template-typescript

Live app

This is a real application created from this new functionality, it already has the service worker enabled. Look: https://test-cra-97e59.firebaseapp.com/

@facebook-github-bot
Copy link

Hi @marlonmleite!

Thank you for your pull request and welcome to our community.We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file.

In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@marlonmleite marlonmleite changed the title Add support to customize the Service Worker with Workbox v5 Add support to customize the Service Worker with InjectManifest and Workbox v5 Feb 14, 2020
@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@marlonmleite
Copy link
Author

In the last 2 commits I added support for the current GenerateSW configuration, so after the release of this no configuration or migration will be necessary for current users.

# Conflicts:
#	packages/create-react-app/yarn.lock.cached
Copy link

@MOZGIII MOZGIII left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great! Workbox 5 really solved the most difficult problem with service workers support - getting service worker files to run through webpack. This is what blocked my attempt to introduce service workers a while ago.
The approach looks good.

@marlonmleite
Copy link
Author

@iansu @mrmckeb @ianschmitz please review, this is related to other discussions that you have been involved in.

This is very important and there are people waiting for it.

@mrmckeb
Copy link
Contributor

mrmckeb commented Feb 29, 2020

Hi @marlonmleite, sorry - we're all volunteers on this project and sometimes we can't get to issues in a timely manner.

This PR is interesting, would the sw-template replace the service-worker file?

@marlonmleite
Copy link
Author

I'm sorry @mrmckeb, I did not know.

The sw-template will be the input file for the Workbox and Workbox will generate the build/service-worker.js file based on this template.

The current service-worker.js is generated from a standard Workbox template and will remain so for those who do not provide the sw-template-js.

For those who use sw-template.js, the Workbox will use that template.

@mrmckeb
Copy link
Contributor

mrmckeb commented Feb 29, 2020

It might make sense to have just the one file. @ianschmitz or @iansu - any thoughts?

@marlonmleite
Copy link
Author

@mrmckeb initially the use of the sw-template.js file was required, but this would cause a break and could delay the release of this functionality in a minor/patch version.

If your choose to keep 1 file required, this code can be removed https://github.com/facebook/create-react-app/pull/8485/files#diff-dc0c4e7c623b73660da1809fc60cf6baR676-R691

@sidati
Copy link

sidati commented Mar 25, 2020

Any news about this PR ?

@jflayhart
Copy link

jflayhart commented Apr 11, 2020

To avoid too much entropy in one PR I opened #8822 to take advantage of some default config in version 5.

@Abdul007Malik
Copy link

Abdul007Malik commented Apr 15, 2020

There should be support for customization on service worker. As @jakearchibald mentioned in his presentation about giving more power to a web developer. Currently react can do offline but web developer is restricted to make the app offline according to his need. I know there is a work around by including custom service worker, but in this way we are completely neglecting the offline features provided by react itself.

@mrmckeb mrmckeb added this to the 3.5 milestone Apr 16, 2020
@mrmckeb
Copy link
Contributor

mrmckeb commented Apr 16, 2020

I've tagged this as a potential 3.5 addition, but need to get some thoughts from @ianschmitz or @iansu on it first.

@jflayhart
Copy link

jflayhart commented Apr 21, 2020

@mrmckeb Hey 👋! I see you responded to this fairly recently and only wanted you to be aware of a seemingly smaller changeset that I think could be released much sooner before this PR is ready: #8822

CRA would greatly benefit from this to resolve some inherent ServiceWorker caching issues that google fixes in their V5 workbox update.

@ianschmitz ianschmitz modified the milestones: 3.5, 4.0 May 3, 2020
@bebraw
Copy link

bebraw commented May 19, 2020

I have been testing this a bit and I think it would be a worthwhile addition to CRA especially given it enhanced its functionality. The conflicts seemed straightforward to fix and I updated my branch to the newest workbox as well. A couple of notes based on my testing so far:

  1. Likely it would make sense to support mixed usage of JS and TS. In my project I have TS but initially I tried writing my worker in JS until I realized the code is forcing TS here.
  2. Currently the service worker gets compiled only during production mode making it tricky to develop them. I would likely add means of compiling them during development as well. This might imply adding a new flag in the environment or so and I am not sure what would be the best way to tackle it.
  3. It would be good to at least document that you'll need a small piece of at the app side. Here's what I ended up using:
if ('serviceWorker' in navigator) {
  window.addEventListener('load', () => {
    navigator.serviceWorker
      .register('/service-worker.js')
      .then((registration) => {
        console.log('SW registered: ', registration);
      })
      .catch((registrationError) => {
        console.log('SW registration failed: ', registrationError);
      });
  });
}

It's possible I missed something obvious here, though. It's likely good if the consumers can control this code but there should be an example you have to do it.

Overall it would be a welcome change to CRA from my perspective and I hope the PR gets some momentum and will be merged eventually as it's a vast improvement over status quo.

@iansu
Copy link
Contributor

iansu commented Jul 22, 2020

We just merged another PR that upgrades to workbox 5 and adds many more configuration options for service workers. Thanks for the PR but I'm going to close this as hopefully #9205 solves these issues.

@iansu iansu closed this Jul 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants