Repository navigation
make global object an instance of EventTarget #45981
Description
Activity
I think your actual request is "make the global object an instance of
EventTarget", but Node.js global object doesn't have any event attached to it (so far).It can be used for listening to eg PostMessage, network offline/online, background fetch, any kind of errors etc.
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.
Reacted by Anna Henningsen and Tobias Nießen- changed the title
[-]global `addEventListener` / `removeEventListener` / `dispatchEvent`[/-][+]make global object an instance of `EventTarget`[/+]on Dec 26, 2022 There's a few issues with the current EventTarget implementation that would need to be fixed:
- Use
globalThisas the this value if this is null or undefined (https://webidl.spec.whatwg.org/#dfn-create-operation-function). This is only needed for EventTarget because globalThis will implement its methods/properties (ie.addEventListener(...)will work). Remove private properties and replace with symbolsedit: EventTarget doesn't use private properties - Event does.- Add those symbols to globalThis (easily done with
initEventTarget. Object.setPrototypeOf(globalThis, EventTarget.prototype)
Reacted by Jacob Hummer- Use
- added a commit that references this issue
on Dec 28, 2022 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
Workerclass from the HTML spec. It uses aWorkerGlobalScopeon the "inside" of the spawned worker which is an EventTarget. Current polyfills like developit/web-worker (~320k downloads weekly) need to do some song and dance to apply.addEventListener()and friends to the global scope: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
ref https://html.spec.whatwg.org/multipage/workers.html#the-workerglobalscope-common-interfaceThere 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
if-gated and override or stack EventTarget.prototype shims on top of each other! 😩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.
because it's such a pain to deal with the differences of worker_threads and Web Worker API
the fact that worker_threads is different enough from Worker often makes me decide just not to bother with threads at all in code (even though it could often benefit) that is polymorphic for Node/Browsers
NodeJS is the only one going against the flow.
I too also wish to be able to register a
unloadevent instead of having to depend onprocess.on('exit')(i don't wish to bring in the holeprocesspackage into other runtimes only for using it globally or importing it fromnode:processI 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.
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.
To implement cross comp. worker we would need to haveonmessageoraddEventListeneras globalwe can't have web compatible workers if we don't have a EventTarget on
globalThishence why it would be blocked on @KhafraDev PRIf this ever get resolved then i would like for some other things to be considered but they are blocked by this particular issue.
- Make
globalThisextendEventTarget- When receiving post messages in a worker, construct a
MessageEventand dispatch it toglobalThis - Add
globalThis.postMessagethat can reply back without having to includingparentPortfromworker_thread - Add
globalThis.onmessageas an alternative mean to usingaddEventListener- maybe flag
worker_thread.parentPortas a bit obsolete (just doc wise)
- maybe flag
- implement some of Deno's events
- load
- beforeunload
- unload
- unhandledrejection
- Support Web Workers #43583
- Cross-thread URL.createObjectURL #46557 to better be able to create dynamic worker / ESM code:
- Being able to do
import(blobUrls)#47573 - being able to do
new Worker(blobUrl) - allow "experimental loaders" to fetch the content out of blob urls (no matter where the thread came from)
- basically allow any thread to just
fetch(blobUrl)where a blob could come from another thread
- Being able to do
- Make disk- blob transferable #47666
- When receiving post messages in a worker, construct a
- Make
We can have globalThis extend EventTarget in workers but not on the main thread I think those are pretty orthogonal?
Reacted by Antoine du HamelReacted by Khafra and Jimmy WärtingReacted by Jacob HummerLet 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?
Reacted by Jacob HummerIf 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.
Reacted by Jacob HummerReacted by Jacob HummerI agree with @KhafraDev
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?
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
globalThis.WorkerThe 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
import { Worker } from 'worker_thread'cuz that would pollute other env and bundlers and CDN would then try to polyfill everything that is specific to NodeJS. I do not wish to ship any NodeJS specific polyfilled worker_threads into other env like Deno, bun.js or browsers that may imply that i want to use ref(), workerData, parentPort, reciveMessageOnPort EventEmitter etc etc. cuz that is just too much. and things like the syncreciveMessageOnPortdose not even work on other env. Atomic should have been used for this instead.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
- Not have any
globalThis.Bufferor anyglobalThis.processif you intend to use them then it should be imported explicitly globalThisin this new spec'ed worker should extendEventTarget- there should not be any magical
worker.ref()or.unref()- if you want to have this kind of functionality then i think it would be better to have something like
util.unref(worker)that change internal private properties in a more private side channel manner.
- if you want to have this kind of functionality then i think it would be better to have something like
- it should not be possible to create workers by doing any string evaluation, dynamic code should use
new Worker(URL.createObjectURL(new Blob))instead.- so creating worker from blob urls should work as intended like how browser works.
new Worker(url, { type: 'module', name: 'xyz' })should work too- it should be more easily controllable if it should run as a esm or classical script rather than being dependent on what's said in package.json
type. There could maybe even be 3 possible solutions,module,scriptandcjswhere withscriptyou would have to import things with the olderimportScript() - but i would not mind if we only supported
moduleas a initial MVP or totally discarded script and cjs entirely.
- it should be more easily controllable if it should run as a esm or classical script rather than being dependent on what's said in package.json
Reacted by Benjamin Gruenbaum- Not have any
@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 think this issue can be closed as "won't fix"
I can accept and bite the apple for now.
but i think this could circle back and be requested again and again further down the road.
Right now i only mostly care about having EventTarget inside of a Worker.
But eventually i probably would also want to use EventTarget onglobalThisin the main thread some time in the feature also. but not right now.let's continue discussion about workers there
alright. just one question tough.
Do you think it would be better to create a new spec-compatible worker or would it be better to try and "fix" the currentworker_thread.Workerthat we already have so it lives onglobalThisand have all the blows and whistles of a normal web worker and behaves the same way as a web worker so it quacks like a duck and walks like a duck?I would probably bet on creating a new spec compatible worker...
Reacted by Antoine du HamelThere 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
processis an alternative. While I don't disagree that they have similar uses, they are fundamentally different & as I mentioned in the PR, can coexist. There's already a server environment that emits global events using an EventTarget, which node could use for inspiration. Even if node doesn't emit any events itself, this is very unlikely to break anything (unless people are doingif (globalThis instanceof EventTarget) { ... }) and would solve the issues mentioned here.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
processto emit global events, but library authors can then start using EventTarget globally in node.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...
Reacted by Jimmy WärtingReacted by Jacob HummerI also fail to see what is so breaking about about having
globalThisbeing a instance ofEventTarget.And I agree upon that
processand globalEventTargetcan coexist. and that they are two fundamentally different things.Events on
processdose not have to halt, face out and die flat on the ground.it dose not even have to emit one to
processthe NodeJS style and another instance ofEventtoglobalThiseffectively sending out duplicated events everywhere. it could be enough to just emit the things we already have toprocessas it already dose. just b/c we introduce global EventTarget dose not mean that we should stop usingprocess.on(x)I do not think even if NodeJS are not using
EventTargetfor emitting something globally right now and being completely meaningless in the NodeJS core context makes it deserving of a rejections of the idea of having it globally only b/c it dose not deemed fit or not useful for anything and that we already haveprocessshould hold up for a strong argument against it shipping it. Otherwise the next reasoning for having anything anything similar to a classical global browser event that could emit something would then be: "We should emit "push|fetch|message|storage|sync" event toprocessinstead cuz that's what we are 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
Reacted by Jacob Hummer and Andres RodriguezComing late to this, but it's a nice DX to have global event listeners (like a browser) when you want your code to remove dependencies.
What is the problem this feature will solve?
This sounds like a simple feature request. We already got
EventTargetandEvent. 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