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

🏗️ Remove service-worker hack #525

Merged
merged 1 commit into from
Dec 23, 2018
Merged

Conversation

resir014
Copy link
Member

@resir014 resir014 commented Dec 22, 2018

Motivation

We used some kinda of hack to get the service worker URL to register it, but we can actually do it with serviceworker-webpack-plugin to be able to load up our custom service worker without having to rewrite everything in Workbox.

So we can keep our existing service worker, but to include it, we can simply do this:

- import workerPath from 'bemuse/hacks/service-worker-url/index.loader.js!serviceworker-loader!./service-worker.js'
+ import serviceWorkerRuntime from 'serviceworker-webpack-plugin/lib/runtime'

Things to note

  • Since we're rebuilding our service worker, need to figure out a way to 'safely' unregister the old service worker before installing the new one.
  • I had to update the webpack config so that the publicPath is set at the / directory. This means I needed to rewrite some configs in webpack to reflect that change (specifically with file output.).
  • No TS typings included for the serviceworker-webpack-plugin runtime, so I had to include the typings myself.

@resir014 resir014 requested a review from dtinth December 22, 2018 15:02
@codecov-io
Copy link

codecov-io commented Dec 22, 2018

Codecov Report

Merging #525 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #525   +/-   ##
=======================================
  Coverage   81.38%   81.38%           
=======================================
  Files         153      153           
  Lines        4619     4619           
  Branches       68       68           
=======================================
  Hits         3759     3759           
  Misses        842      842           
  Partials       18       18

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 71b9091...56bc60c. Read the comment docs.

@resir014
Copy link
Member Author

We can probably try and follow what create-react-app did to register/unregister our service worker, here: https://github.com/facebook/create-react-app/blob/master/packages/react-scripts/template-typescript/src/serviceWorker.ts

Copy link
Member

@dtinth dtinth left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

Actually, on an unrelated note, I have an (unpublished) branch that removes the webpacked service worker and replaced it with a workbox-based service worker. (It is not using webpack in any way, but instead the service worker file is placed in public/sw.js, similar to the skin files). I haven't tested it thoroughly yet though. Service workers are rocket science, that's why I preferred to have as little custom service worker code as possible (the original service worker is created before workbox exists, and furthermore, workbox supports range requests).

I agree with changing the publicPath to /. This allows for more flexibility when bundling stuff. Let’s get this in :D

@resir014
Copy link
Member Author

resir014 commented Dec 23, 2018

@dtinth Ah yeah! We can give this a try, then at the release after this we can experiment with Workbox more. I tried fiddling with Workbox at one point but I didn't go far. 😅

I'll get this merged later today.

@resir014 resir014 merged commit 188e0ab into master Dec 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants