-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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
make global object an instance of EventTarget
#45981
Comments
I think your actual request is "make the global object an instance of
I think I would need to have a more detailed proposal, but as is it seems... weird to attach those to the global object rather than more specialized objects. |
This comment was marked as resolved.
This comment was marked as resolved.
addEventListener
/ removeEventListener
/ dispatchEvent
EventTarget
There's a few issues with the current EventTarget implementation that would need to be fixed:
|
Fixes nodejs#45981 Changes: - Moves all EventTarget symbols to one symbol, to prevent exposing 4 symbols to globalThis. - Fallback to `globalThis` as the this value in EventTarget if this is null or undefined. This is needed to make the "floating" methods work (ie. `addEventListener(...)`).
I notice that there's some discussion in #45993 about whether or not this is a good idea. I'd like to present a particular use case for this feature: polyfills of other web platform features! For instance, take the let proto = Object.getPrototypeOf(global);
delete proto.constructor;
Object.defineProperties(WorkerGlobalScope.prototype, proto);
proto = Object.setPrototypeOf(global, new WorkerGlobalScope());
['postMessage', 'addEventListener', 'removeEventListener', 'dispatchEvent'].forEach(fn => {
proto[fn] = proto[fn].bind(global);
}); https://github.com/developit/web-worker/blob/29fef9775702c91887d3d8733e595edf1a188f31/node.js#L167-L173C5 There are other polyfills that target other browser-related APIs that would benefit from the common baseline of assuming that the globalThis is an instance of EventTarget. For instance, polyfills for the unload (process.on(exit)) beforeunload (process.on(beforeExit)) etc. would all need to do this song and dance, but will do it in a slightly different way that may-or-may-not be Having this EventTarget globalThis be in Node.js core would be beneficial to avoid clashing shims. I think it would also be a good step towards #43583. If a Node.js-native Worker ever happens, having globalThis already be an EventTarget would mean less "inside the Worker is an EventTarget, but the normal Node.js global is NOT an EventTarget" mayhem and mismaps. |
miniflare is another popular user package with (~250k downloads / week) that emulates a spec'ed worker by having global fetch event listener. And it's evidents by all ~250 👍 on the 2nd most upvoted NodeJS feature request 👉 #43583 that ppl want to have a isomorphic way to create web worker that can listen to standard classic message events in the same way.
NodeJS is the only one going against the flow. I too also wish to be able to register a |
I think having globalThis extend EventTarget and making our workers also web workers (assuming there is no performance penalty) is a much less controversial ask than making the Node global an EventTraget. |
Node's global and globalThis are equal to each other, we can't do it to only 1 of them. My PR made globalThis extend EventTarget, similar to other environments. Web workers were brought up in my PR, the general gist being that until someone wishes to implement web workers, my PR would be blocked. |
That was half of the reason why i made this feature request. we can't have web compatible workers if we don't have a EventTarget on |
If this ever get resolved then i would like for some other things to be considered but they are blocked by this particular issue.
|
We can have globalThis extend EventTarget in workers but not on the main thread I think those are pretty orthogonal? |
Let me clarify given the reactions - the global object extending EventTarget is "wontfix", it has multiple collaborators blocking it at the moment (and no approvals on the PR). Does making it extend EventTarget for workers solve the issue you are describing or do you believe anything short of extending the global object with EventTarget does not address your use case? |
If the global object is only extended in workers, it won't solve the issue for polyfills or interoperability for Cfw/Deno/browsers. In my opinion it would create more confusion than not extending it anywhere. |
I agree with @KhafraDev
That would at least solve my use case. but i would still want globalThis to extend EventTarget everywhere. But i also think it would be good to just let old worker_threads be as it's and build a new slimmer spec'ed web Workers from scratch that isn't as rich as worker_threads and add it to The point is that i want to use spec'ed web worker in a Isomorphic way that dose work exactly the same way in all other environment. I do not want to have to call otherwise ppl will just create things like shims (alá jQuery) that works with all the inconsistency across different env or not even bother at all. we do not want that I think that this new slimed web worker should
|
@jimmywarting I think we have consensus on all of your points, we would "just" need someone to champion this. |
In any case I think this issue can be closed as "won't fix" since we don't have consensus on it. I do think the contents of the comments here by both @jimmywarting and @KhafraDev are valuable so I'm hesitant to close this while there is still discussion about workers going on. If we had web workers and they had EventTarget globalThis then maybe some of the people blocking can change their mind. |
Actually I see #43583 @jimmywarting let's continue discussion about workers there? As Antoine mentioned web workers have consensus (as far as I know) and just need someone to drive it. |
I can accept and bite the apple for now.
alright. just one question tough. I would probably bet on creating a new spec compatible worker... |
There isn't much to "fix" with the current Worker, it just isn't a web Worker. I assume most of the internals could be re-used so it shouldn't need to be fully rewritten. If it was exposed globally it'd be even more of a pain to write cross-environment apps/libraries so I'm a strong -1 on exposing anything globally that isn't a web Worker. I do understand why this is a wontfix, buuuut I don't really agree with the reasoning tbh. Before I made the PR I was neutral on adding it, but then the argument(s) against it didn't feel very strong to me. The biggest issue doesn't seem to be with the idea or the pull request, but that it isn't "fit" for node/there is an alternative. I don't consider web apis fit for node, but they've been getting slowly implemented over the last few years nonetheless. Just because it's not a good fit (which is an opinion, not necessarily a fact) doesn't mean that people would prefer to use a "better" api for their runtime. Then there's the other argument that Furthermore, it seems like the idea to add it behind a flag seems to have been denied(?), but I don't see any reasoning behind that. If the behavior is opt-in, the flag could always be removed if at a later time the api is deemed to be unsuitable. Node can keep using I also believe that if it can't land now it likely won't ever be able to land, barring a full implementation of web workers. Even then, someone implementing web workers will have to 1) implement this change and then 2) implement the Workers, which is more work & thus less appealing to work on... |
I also fail to see what is so breaking about about having And I agree upon that Events on it dose not even have to emit one to I do not think even if NodeJS are not using i think a global event target would be useful for library authors who wish to use it right now. and what's about to arrive in the feature |
What is the problem this feature will solve?
This sounds like a simple feature request. We already got
EventTarget
andEvent
. So my feature request is to have aglobalThis.addEventListener
. (along with remove & dispatch) added to the global scope (by extending EventTarget)What is the feature you are proposing to solve the problem?
Deno, browser and web workers and even cloudflare workers already has a global event listener that can be used to listen to a variety of things.
It can be used for listening to eg PostMessage, network offline/online, background fetch, any kind of errors etc.
This is partly related to #43583
Having different code paths to solve the same things gets troublesome.
What alternatives have you considered?
No response
The text was updated successfully, but these errors were encountered: