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 PWA mode #1972

Closed
wants to merge 2 commits into from
Closed

Add PWA mode #1972

wants to merge 2 commits into from

Conversation

endrl
Copy link
Contributor

@endrl endrl commented Apr 17, 2023

  • Add vite plugin pwa, no user prompt for update, just self reloading.
  • While jellyfin-web used a manifest.json we use a jellyfin.webmanifest to follow the standards
  • So this is a breaking change for jellyfin-webos at least. As noted in Add webOS/Tizen support #1967

jellyfin pwa

@sonarcloud
Copy link

sonarcloud bot commented Apr 17, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@jellyfin-bot
Copy link

Cloudflare Pages deployment

Latest commit ddd4f5c
Status ✅ Deployed!
Preview URL https://5438ecba.jf-vue.pages.dev
Type 🔀 Preview

View build logs
View bot logs

Copy link
Member

@ferferga ferferga left a comment

Choose a reason for hiding this comment

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

I'm sorry, but this is a hard no (unless you defeat this with really good reasons) at this moment. PWA capabilities have been recently removed in #1939 in fact. There has not been much public discussion as of the reasoning behind the decision (besides fixing the issue that PR was targeting), I'm sorry about that, but here it comes:

Problems in our client/workflows

  • PWA in general is very picky about its configs and needs to be done right from the beginning. The issue fix: reloading after client changes #1939 solved could have been fixed if we used the networkFirst strategy from the beginning, for instance. But changing that in production is risky. But it's not only that: modifying the service worker itself can be challenging.
  • PWAs, when poorly implemented, can take roughly 1 GB of storage in your browser. You can check by yourself, you probably visited a lot of sites which have a lot of cached assets. You can blame PWA for all of that. Jellyfin Vue was also affected, taking like 800 MB of storage. Everytime I accessed a newly built commit from CF Pages added up to the total size.
  • Another example of the complexity it adds: people who used the hosted instance before fix: reloading after client changes #1939 was merged will still carry the service worker and cache indefinitely, unless they clear the app/browser data or manually unregister the worker in DevTools (not something everybody knows how to do). We could push a version that registered an SW whose sole mission is to kill the old one. But for how long we should've kept it? This also virtually force us to be dependents of workbox (what vite-plugin-pwa uses under the hood) forever to properly "kill" the PWA infection, because if an user who hasn't used Vue for years visited the client after we removed the destroying self worker, he would still be "infected".
  • Summing up: Our client evolves really fast. We don't have real support for offline servers (yet). We don't have a real idea of how we want to structure/design our caching system (caching itself it's one of the biggest problems in engineering). When something is broken in our client, I send PR, merge and fix deployed. Doing that in PWA is really complex in some aspects as cache can be so persistent (example in the previous point).
  • TLDR: We recognise that PWA is important and want to have good support. But, given the points above, it's clear it's not a drop-in thing and must be carefully studied and rightfully implemented (so all it will ever need will be minor modifications) since the beginning. The truth is, we aren't (still) well educated about how PWA works and we have a lot of other things to do first than care about that, given that most browsers can add any website as an app to the OS (PWA experience is better, but it's not a night/day difference)

Problems in the dependencies itself

  • vite-plugin-pwa maintenance seems to be dropping in frequency. That's partly caused by the status of workbox maintenance also being in the air.
  • Everywhere I read online, Workbox is a must for building service workers. There is no real alternative. Writing your own service worker is fully discouraged everywhere
    • This is not that relevant to our topic, but I philosophically ask myself why the technology/specs behind it are so obscure. It was born by Google and I remember it faced a lot of criticism for not being open enough etc etc. I think we're dealing with this here, basically,
  • Workbox has deprecated dependencies in it's dependency tree. We had 0 warnings in npm i while migrating to Vue 3, we would like to be up to date as much as we can to avoid another Vue2/Nuxt 2 situation.
  • The worst: Workbox, as of today, injects Google Analytics code into the service worker without us being aware of it. The code itself seems to be dead and do nothing, but why it's there in the first place, and across many versions? (tested in the vite-plugin-pwa version we used in chore: upgrade to Vue 3 #1812 and it's there too, even worse since it was mentioned multiple times and with references to IDs, which seem to not be present in your version).
    Why I need to doubt a dependency? With the rise of supply chains attacks in the JS ecosystem, dependency upgrades are a real security nightmare. If I have a dependency that, as of today, makes me worry about security, why should I for future versions? What prevents them from pushing an upgrade that makes real use of GA?. I can't trust them right now

I approved the workflows just so you could see the results in your own code and not trust my word, but ofc you can replicate this locally too!

image

TLDR2: I think we just need some time to properly do PWA and let the ecosystem recover from the workbox state. I'm open for a manifest-only approach (don't know if that helps), but no service workers at all for the time being.

@endrl
Copy link
Contributor Author

endrl commented Apr 19, 2023

Thank you for clarification. I wasn't aware that you recently removed it! Just had a look at the tasks

  • GA is a no-go wasn't aware of it (no mentioning in the vite docs at least. hm)
  • workbox state looks indeed... unmaintained
  • A network first strategy should solve the outdated problem. vite pwa wrote somewhere about forced update checks with setInterval as another solution

So no solution for the moment. But a great wrap up of the state. It might be possible to replace vite pwa against a self written service worker with a suitable cache strategy. In the end just some events, a cache and a fetch interceptor?!

@ferferga
Copy link
Member

@endrl That's what I wanted to try, but everywhere it's said it's bad practice, but they don't give further reasoning to. Even if we could, as I said in my wrap up, PWA is very sensible and we should do it as right as possible since the beginning, it can't be developed progressively and there's a lot of other things we (Tiwenty and me) have in the backlog before we can focus on that (you can also check the approximate roadmap)

It would be great to explore that area (or just wait for workbox and vite-plugin-pwa to settle it's own dust). If you want to still tackle it go ahead! I'm going to close this PR for now, it's better for historical reasons to keep all of this in a discussion, so feel free to welcome it if you want to gather feedback about the implementation, possible solutions, etc etc... Will be much more visible than here.

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.

3 participants