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 resource caching from service worker #4885

Merged
merged 2 commits into from
Oct 25, 2023

Conversation

nielsvanvelzen
Copy link
Member

@nielsvanvelzen nielsvanvelzen commented Oct 16, 2023

I've ran into annoying cache issues from the worker occasionally and after thinking about it I came to the conclusion that it does not make any sense to cache the complete SPA.

Jellyfin web does not have offline functionality and the resources are served by the server. If the web client is used offline that means you can't connect to the server, so there is no point in caching it 🤷.

Changes

  • Remove resource caching from service worker
    • We still use the worker for notifications
  • Remove workbox dependencies
  • Manually implement clientsClaim function from workbox (it really is just a one liner)

Issues

Fixes #4549, fixes #4651

@nielsvanvelzen nielsvanvelzen marked this pull request as ready for review October 16, 2023 14:36
@nielsvanvelzen nielsvanvelzen requested a review from a team as a code owner October 16, 2023 14:36
@thornbill
Copy link
Member

I've been wondering if there was really any point in this for awhile now... 😅

@thornbill
Copy link
Member

@nielsvanvelzen I noticed the serviceworker file wasn't being included in the build after these changes, I pushed up a change that should fix that if you want to confirm...

@thornbill thornbill added the build This PR or issue mainly concerns build tools label Oct 20, 2023
@nielsvanvelzen
Copy link
Member Author

Oops, I think it worked for me at first but perhaps I forgot to restart Webpack or something was cached. Can confirm the fix works on the deployed pages instance ✅:

image

@jellyfin-bot jellyfin-bot added the merge conflict Conflicts prevent merging label Oct 25, 2023
@jellyfin-bot

This comment has been minimized.

@jellyfin-bot jellyfin-bot removed the merge conflict Conflicts prevent merging label Oct 25, 2023
@sonarcloud
Copy link

sonarcloud bot commented Oct 25, 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

@nielsvanvelzen
Copy link
Member Author

Rebased to fix merge conflicts in package-lock file

@jellyfin-bot
Copy link
Collaborator

Cloudflare Pages deployment

Latest commit 534b2fd
Status ✅ Deployed!
Preview URL https://5dde0c35.jellyfin-web.pages.dev
Type 🔀 Preview

View build logs
View bot logs

@thornbill thornbill added this to the v10.9.0 milestone Oct 25, 2023
@thornbill thornbill merged commit 05e2186 into jellyfin:master Oct 25, 2023
22 checks passed
@nielsvanvelzen nielsvanvelzen deleted the nvv-drop-workbox branch October 26, 2023 05:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build This PR or issue mainly concerns build tools
Projects
None yet
3 participants