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

Fix ServiceWorkerRegistration is not defined (#173) #182

Open
wants to merge 2 commits into
base: next
Choose a base branch
from

Conversation

gpoole
Copy link

@gpoole gpoole commented Jun 7, 2023

Adds an additional check for ServiceWorkerRegistration being defined before trying to access it, allowing the module to be imported in non-browser environments or browsers that don't define ServiceWorkerRegistration.

Fixes #173

@markcellus
Copy link
Owner

Seems reasonable. Thanks!

@markcellus
Copy link
Owner

@gpoole before merging this, do you mind creating a test for it? Just want to avoid this being reverted. Test file is located in the test directory and should be pretty easy. Thanks again!

@mspae
Copy link

mspae commented Dec 14, 2023

@gpoole @markcellus I'd be willing to help adding a test to get this MR merged. I'm running into the same issue and am currently patching cookie-store which is not ideal obviously. Let me know what you think.

@markcellus
Copy link
Owner

Yeah pls add tests and repro steps thanks. And will take another look.

I was waiting to see if someone could give me steps to reproduce on my machine so I can verify locally before merging. 👍

@mspae
Copy link

mspae commented Dec 18, 2023

Actually can't get the tests to run through at all.

Uncaught TypeError: Cannot set property cookieStore of #<Window> which has only a getter

I' on node v18.17.1. This might be related.

@goto-bus-stop
Copy link

goto-bus-stop commented Dec 27, 2023

ServiceWorkerRegistration is not defined in insecure contexts. It works on HTTPS and on localhost, but if you have an alias in your hosts file (for example i use hserv.local to access my home server where I work on), and you access that through HTTP non-S, ServiceWorkerRegistration is not defined.

Another way to check is using the console on http://example.com (make sure you're not redirected to HTTPS).
image

I'm not really sure how to add a test for this as it requires deleting the global ServiceWorkerRegistration object before the library is loaded at all, which is not doable in index.tests.js. It needs to run in a completely separate environment.

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.

ServiceWorkerRegistration is not defined
4 participants