search

1j01 / jspaint

🎨 Classic MS Paint, ＲＥＶＩＶＥＤ + ✨Extras
https://jspaint.app/about
MIT License
7.22k stars 562 forks source link

Add Service Worker by Workbox #144

Open christianliebel opened 4 years ago

christianliebel commented 4 years ago

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

Closes #109

kenchris commented 4 years ago

What is holding this up?

TheLastGimbus commented 3 years ago

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

christianliebel commented 3 years ago

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 commented 3 years ago

Oh, Mozilla killed PWAs for desktop, marvelous :tada:

Anyway, big thanks for this!

1j01 commented 3 years ago

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 commented 3 years ago

Those are some very valid points. Take care!