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

make global object an instance of EventTarget #45981

Closed
jimmywarting opened this issue Dec 26, 2022 · 19 comments · May be fixed by #45993
Closed

make global object an instance of EventTarget #45981

jimmywarting opened this issue Dec 26, 2022 · 19 comments · May be fixed by #45993
Labels
feature request Issues that request new features to be added to Node.js.

Comments

@jimmywarting
Copy link

jimmywarting commented Dec 26, 2022

What is the problem this feature will solve?

This sounds like a simple feature request. We already got EventTarget and Event. So my feature request is to have a globalThis.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

@jimmywarting jimmywarting added the feature request Issues that request new features to be added to Node.js. label Dec 26, 2022
@aduh95
Copy link
Contributor

aduh95 commented Dec 26, 2022

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.

@jimmywarting

This comment was marked as resolved.

@jimmywarting jimmywarting changed the title global addEventListener / removeEventListener / dispatchEvent make global object an instance of EventTarget Dec 26, 2022
@KhafraDev
Copy link
Member

KhafraDev commented Dec 27, 2022

There's a few issues with the current EventTarget implementation that would need to be fixed:

  • Use globalThis as 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 symbols edit: EventTarget doesn't use private properties - Event does.
  • Add those symbols to globalThis (easily done with initEventTarget.
  • Object.setPrototypeOf(globalThis, EventTarget.prototype)

KhafraDev added a commit to KhafraDev/node that referenced this issue Dec 28, 2022
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(...)`).
@jcbhmr
Copy link
Contributor

jcbhmr commented May 25, 2023

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 Worker class from the HTML spec. It uses a WorkerGlobalScope on 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-interface

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 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.

@jimmywarting
Copy link
Author

jimmywarting commented May 25, 2023

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 unload event instead of having to depend on process.on('exit') (i don't wish to bring in the hole process package into other runtimes only for using it globally or importing it from node:process

@benjamingr
Copy link
Member

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.

@KhafraDev
Copy link
Member

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.

@jimmywarting
Copy link
Author

jimmywarting commented May 26, 2023

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 have onmessage or addEventListener as global

we can't have web compatible workers if we don't have a EventTarget on globalThis hence why it would be blocked on @KhafraDev PR

@jimmywarting
Copy link
Author

If this ever get resolved then i would like for some other things to be considered but they are blocked by this particular issue.

  • Make globalThis extend EventTarget
    • When receiving post messages in a worker, construct a MessageEvent and dispatch it to globalThis
    • Add globalThis.postMessage that can reply back without having to including parentPort from worker_thread
    • Add globalThis.onmessage as an alternative mean to using addEventListener
      • maybe flag worker_thread.parentPort as a bit obsolete (just doc wise)
    • 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
    • Make disk- blob transferable #47666

@benjamingr
Copy link
Member

benjamingr commented May 26, 2023

We can have globalThis extend EventTarget in workers but not on the main thread I think those are pretty orthogonal?

@benjamingr
Copy link
Member

benjamingr commented May 31, 2023

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?

@KhafraDev
Copy link
Member

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.

@jimmywarting
Copy link
Author

jimmywarting commented May 31, 2023

I 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.Worker

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 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 sync reciveMessageOnPort dose 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.Buffer or any globalThis.process if you intend to use them then it should be imported explicitly
  • globalThis in this new spec'ed worker should extend EventTarget
  • 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.
  • 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, script and cjs where with script you would have to import things with the older importScript()
    • but i would not mind if we only supported module as a initial MVP or totally discarded script and cjs entirely.

@aduh95
Copy link
Contributor

aduh95 commented May 31, 2023

@jimmywarting I think we have consensus on all of your points, we would "just" need someone to champion this.

@benjamingr
Copy link
Member

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.

@benjamingr
Copy link
Member

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.

@aduh95 aduh95 closed this as not planned Won't fix, can't repro, duplicate, stale May 31, 2023
@jimmywarting
Copy link
Author

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 on globalThis in 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 current worker_thread.Worker that we already have so it lives on globalThis and 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...

@KhafraDev
Copy link
Member

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 process is 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 doing if (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 process to 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...

@jimmywarting
Copy link
Author

jimmywarting commented May 31, 2023

I also fail to see what is so breaking about about having globalThis being a instance of EventTarget.

And I agree upon that process and global EventTarget can coexist. and that they are two fundamentally different things.

Events on process dose not have to halt, face out and die flat on the ground.

it dose not even have to emit one to process the NodeJS style and another instance of Event to globalThis effectively sending out duplicated events everywhere. it could be enough to just emit the things we already have to process as it already dose. just b/c we introduce global EventTarget dose not mean that we should stop using process.on(x)

I do not think even if NodeJS are not using EventTarget for 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 have process should 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 to process instead 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants