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

replace window.* by self.* in order to support service worker context #271

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

johuder33
Copy link

@johuder33 johuder33 commented Oct 28, 2022

This package works perfectly well, but when you use it in a service worker context, it failed because window.* access is not support by service worker.

So in order to support this package into service worker, we should make a little change, replace any window.* usage by self.*.

So self will work for non-window contexts where this code can be executed, so with this change this package can be used into a service worker without any problem 😄 .

if window.addEventListener worry you, it is well-covered, since before adding some listener to specific events, the code is checking if the context it's actually a window one by typeof window execution.

@travist
Copy link
Owner

travist commented Nov 2, 2022

A change like this really needs some automated tests around it. More specifically I would like to know if the "addEventListener" methods and things referencing the "self" object are indeed referencing the "window" object in a browser environment.

@johuder33
Copy link
Author

johuder33 commented Nov 3, 2022

Hello @travist , thank you for taking some time and read this PR, sure I can add some test in order to cover this change, please let me know which type of test you think is better to implement, also, the self variable will point to the main object based on the environment where the code is executed, for example, if you execute the code in a normal web environment it will point to window, but if it is executed into a Service Worker it will point to the ServiceWorkerGlobalScope, but please let me know how can I help in this.

@MaxNoetzold
Copy link

I would love to have this feature as well.
In principle, I see the point of automated tests, but it doesn't really seem necessary in this case. As mdn states window and self are equal in the browser context:

const w1 = window;
const w2 = self;
const w3 = window.window;
const w4 = window.self;
// w1, w2, w3, w4 all strictly equal, but only w2 will function in workers

However, I would love to help get it approved if there is anything for me to do.

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