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 #399

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Conversation

sansmoraxz
Copy link

@sansmoraxz sansmoraxz commented Dec 22, 2022

Using this PR for service worker implementation as per #334. As I have scrapped #339 due to it being too ugly approach.

As suggested by @metruzanca here's modular approach for creating service workers. I have added it as an entry point for webpack builds rather than direct copy of a single js file.

Registration of service workers is done at page load from initialBackground.js rather than by the manifest.json. This way it's more portable, and doesn't break as was the case when trying to use with manifest v3 in firefox. Although firefox still disallows service workers for browser extensions, the only difference will be simply service worker registration failing and proceeding as normal.

As an added benefit this also works when the page is not run as an extension. Which means for service worker unsupported browsers (in extensions only) like firefox, users can still gain the features available to service workers by simply settin the remote url as new tab page (which works even when offline).

PS: I did remove [content-hash] from webpack build file for js as I am not sure how to use a static name to load from initialBackground.js

@sansmoraxz sansmoraxz force-pushed the service-worker branch 2 times, most recently from 7af65c8 to be38e38 Compare December 23, 2022 16:07
@sansmoraxz sansmoraxz marked this pull request as ready for review April 16, 2023 19:35
@metruzanca
Copy link
Contributor

Looks good to me, though I've not used serviceWorkers for caching before so I'm unware of any pitfalls of them.

@sansmoraxz
Copy link
Author

Well there's a gotcha regarding cache invalidation. With cache first strategy at least we entirely skip network fetch if it's found in the local cache. Might not be much of an issue for most cases but some ppl may start wondering why their new image isn't being displayed when they updated some image in xyz website.

Perhaps we can add functionality to clear the cache from UI.

@metruzanca
Copy link
Contributor

metruzanca commented Apr 18, 2023

Good idea. Another thought is what happens when the user tries to hard refresh (ctrl/cmd + shift + R normally overrides all caches).

Do we know if the service worker gets reset?

@sansmoraxz
Copy link
Author

sansmoraxz commented Apr 18, 2023

OK in the GitHub hosted page it seems to bypass the cache from service worker, and the page fails to load if completely offline.

But as an extension the images are loading properly but weirdly enough I don't see any network request being made.

@sansmoraxz
Copy link
Author

Also do you know if chrome removed the functionality to view cache from web inspector for extensions? I can't seem to be able to access those.

@metruzanca
Copy link
Contributor

I can try running it myself tomorrow (left work late today) and see what i can figure out.

@sansmoraxz
Copy link
Author

Sorry about the delay, was busy with other work.

I added the option to clear entire cache and service workers when user clears their data. This should take care of any cache related issue that might arise.

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

2 participants