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 Service Worker by Workbox #144

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

christianliebel
Copy link
Contributor

This PR adds a Service Worker generated by Workbox. In order to update the service worker implementation, run npm run generate-sw.

Closes #109

This was referenced Oct 2, 2019
@kenchris
Copy link

What is holding this up?

@TheLastGimbus
Copy link

Guys what's up? Can't you just marge this and let us all enjoy fully offline paint 💅 ?

@christianliebel
Copy link
Contributor Author

Guys what's up? Can't you just marge this and let us all enjoy fully offline paint 💅 ?

@TheLastGimbus In the meantime, I created my own version called paint.js.org (GitHub), which is all offline-capable, based on Web Components, and uses many modern capabilities (e.g., File System Access API and Async Clipboard API). However, not all tools and actions are implemented yet.

@TheLastGimbus
Copy link

Oh, Mozilla killed PWAs for desktop, marvelous 🎉

Anyway, big thanks for this!

@1j01
Copy link
Owner

1j01 commented Jul 9, 2021

Things that have been holding this up that are not a problem with this PR:

  1. I've been very wary of service workers. Previously service workers had serious problems with HTTP caching. The service worker specification has since been updated and browsers updated to prevent the service worker itself from being cached indefinitely. This makes using a service worker much less of a footgun. See note at top of this answer
  2. I don't like build steps, but I have since bitten the bullet and added one for RTL layout support (RTLCSS). I'd want this to be part of npm run dev so I don't have to think about when to run it. That's on a branch.
  3. I have tons of improvements on a branch that I want to merge first, and I don't want to create unnecessary work by asking for changes to a PR that will eventually probably have merge conflicts (and I don't want to merge PRs first because it will create merge conflicts in a lengthy git history, around 200 commits).
  4. Looking at the issue tracker and pull requests gives me anxiety. I'm just one guy. I ain't even shared this project except like once, and it started getting popular before that.
  5. I'm dealing with serious health issues.

PR critique

  1. This breaks the reload button. If I have changes to share, I really don't want to explain that you have to open up the app, then not just reload but close it, and any and all other instances of it (hunt it down!), and then reopen it again. Updating the app shouldn't feel like voodoo.
  2. This breaks the development workflow (live reload) without enabling a devtools option. If an option is needed, this needs to be documented in the Development Setup section of the readme.
  3. What if the service worker script URL changes? Must it stay at the same path forever? If so this needs to be documented.
  4. service-worker.js is so small I would just inline it in the HTML. And I'm already using ES6 syntax. There's no benefit to using type=module here. Also I don't want files (or variables) named distinctly but without semantic difference in the naming (sw.js vs service-worker.js).
  5. I'd like to see what files are missed by the glob patterns (.js.map?). electron-*.js files could be ignored.

@jespertheend
Copy link

Those are some very valid points. Take care!

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.

Offline support with a Service Worker
5 participants