events,bootstrap: make globalThis extend EventTarget

[KhafraDev]

This is probably a breaking change. I will add tests once the actual implementation's been approved just so I don't waste time writing tests for a feature that won't land 😄. Same with docs.

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(...)). It's also a webidl thing,
  https://webidl.spec.whatwg.org/#dfn-create-operation-function
• Adds 3 'globals', dispatchEvent, addEventListener, removeEventListener.

addEventListener('fetch', (event) => {
  console.log(event)
})

// somewhere else
dispatchEvent(new Event('fetch'))

Fixes: #45981

[nodejs-github-bot]

Review requested:

• @nodejs/startup

[anonrig]

We can enable certain web platform tests with this, right? @KhafraDev

[anonrig]

If we are going to do this, and if this is a breaking change, I believe a flag is needed to enable this experimental feature

[KhafraDev]

We can enable certain web platform tests with this, right?

I don't think so, I'm pretty sure the WPT runner will throw an error if previously failing tests started passing.

edit: one test was enabled actually.

[aduh95]

We'd need to forbid their use in lib/ as we do with other browser globals:

node/lib/.eslintrc.yaml

Lines 155 to 156 in 3ce4cef

	- name: console
	message: Use `const { console } = require('internal/console/global');` instead of the global.

[aduh95]

@joyeecheung does this have an impact on the snapshotting effort?

[KhafraDev]

I updated the eslint config but I wasn't sure what they should be replaced with.

[addaleax]

Do we feel like this is something we want in Node.js? I understand that this is something that other platforms do, but those platforms aren't Node.js and didn't have things like the process object (which is effectively where we emit all our "global" events) from the start. Do we want to start exposing Node.js builtin events on the global object? How would we handle cases where events are posted to both process and globalThis?

I'm not even sure that browsers would do this nowadays with the benefit of hindsight in how JS developed as a language. But yeah, either way, definitely a significant breaking change, probably one where it's worth pinging @nodejs/collaborators.

[mcollina]

Tagging it tsc-agenda for awareness.

[mcollina]

I'm not convinced we should land this change. None of the cases mentioned in the linked issues are applicable to Node.

Most of the useful cases are already handled by Node.js process.

[jasnell]

I'd say no right now. If we do ever make globalThis emit events then yes it should be EventTarget and not EventEmitter but I don't think there's enough justification for this now.

[benjamingr]

I'm also -1 on this for the previously mentioned reasons.

[ShogunPanda]

I'm neutral at the moment.
Let's assume we want to favor isomorphic JS: is there any current code/use case that would benefit by this?

[GeoffreyBooth]

Is this something that perhaps users could opt into? Like a method on process that tells it to stop emitting events so that they can be emitted by globalThis instead?

[KhafraDev]

is there any current code/use case that would benefit by this?

Currently? No, because node doesn't implement it. In the future I can see libraries adopting it to emit events, rather than using the node-only process/EventEmitter combo. It helps writing isomorphic libraries, in a way that build tools (such as typescript, linters, etc) will be fine with.

It should also be pointed out that both process and globalThis EventTarget can exist separately, without causing issues. globalThis doesn't need to re-implement all of process' events. Although I assume if this landed it would be requested.

None of the cases mentioned in the linked issues are applicable to Node.

I would say that this change isn't applicable because node doesn't implement it. If node's global object always extended EventTarget, there would undoubtedly be libraries/apps/scripts using it, rather than using process. The same argument could probably be used for any web standard - "x isn't applicable to node, you can use y instead" (even when the proposed alternative is a very loose fit IMO).

I don't think there's enough justification for this now.

If there's not enough reason to add it now, I doubt there ever will be 😅. People aren't writing code that will hopefully work in node one day.

[mcollina]

My two cents: libraries emitting global events make the ecosystem more brittle. Sometimes certain behaviors must be global, but it's essentially a pattern to avoid whenever possible.

I'm loosely -1, but I might be convinced to approve it if there is a fundamental use case to cover.

[joyeecheung]

@joyeecheung does this have an impact on the snapshotting effort?

I don't think so, as long as no events are emitted/handlers are added during bootstrap (I don't see any here).

I am +1 to the idea of making globalThis an EventTarget, but I don't think we should simply make it an EventTarget without considering its complications. For example, out of the common events fired on window in the browsers, the ones that make more sense for Node.js are message, unhandledrejection (note the cases here), rejectionhandled, which we already (sort of) fire on process, but with disparities in the shapes of the event objects. It seems to be confusing to make globalThis in Node.js an EventTarget without a good story or at least documentation of how we are going to handle these disparities or even whether we are going to support these Web events in our globalThis at all. I am simply guessing here, but I think the majority use cases of globalThis being an EventTarget come down to globalThis.addEventListener() with a well-specified event or maybe sometimes globalThis.dispatchEvent('message'), it's relatively rare to fire/handle custom events in globalThis. Users might attempt to use these well-specified events in globalThis and realize that it doesn't actually work like how things are in browsers, which IMO is worse than not making globalThis an EventTarget.

[KhafraDev]

Here are the events Deno has https://deno.land/manual@v1.29.1/runtime/program_lifecycle#program-lifecycle (load, beforeunload, unload, and unhandledrejection).

[GeoffreyBooth]

Just so that my question doesn’t get lost, is there a way to opt into this? So that the default behavior doesn’t change, but users who want this can enable it. I think that might make it much more palatable, and we can defer the decision on whether it should be enabled by default.

[srl295]

Question. Is there any hope/request that the browsers, deno, etc would move these out of global and into some proper scope? like fetch.addEventListener(…) or something? The webIDL spec referenced shows a scope to addEventListener.

Edit: per #45993 (comment)

I'm not even sure that browsers would do this nowadays with the benefit of hindsight in how JS developed as a language.

this. (if you'll pardon the pun). What would they do? Probably something different, putting these events into a different scope than globalThis.

[KhafraDev]

Question. Is there any hope/request that the browsers, deno, etc would move these out of global and into some proper scope? like fetch.addEventListener(…) or something?

No, there's no feasible way of adding EventTarget methods onto fetch (etc.). Deno has recently added events to globalThis, so it's unlikely they're gonna change anything either. Specs that were planning on emitting events already extend EventTarget, such as WebSocket, XMLHttpRequest, FileReader, and others. Plus, there are global events for fetch that work in service workers.

Is this something that perhaps users could opt into? Like a method on process that tells it to stop emitting events so that they can be emitted by globalThis instead?

It could be done in a future PR, this PR doesn't change how events are emitted.

[mhdawson]

We discussed in the TSC meeting today, at this point the consensus seems to be that value
does not outway the issues/problems unless there is a compelling use case we are not aware
of. We understand it could be useful for some cases (like webworkers) case but feel there are
workarounds.

[Qard]

It would be handy to implement things like FetchEvent. However, in the browser that is limited to ServiceWorkers anyway. What if we had a special type of worker_threads that mimics ServiceWorkers and only have the event handlers available globally within the scope of one of those? It would also make it easier to handle some of the usability questions of things like supporting the FetchEvent pattern adopted by FaaS providers where generally no actual port is defined in code. We could have the main thread function as an orchestrator for the FaaS style code running in workers.

Main thread:

const fetchWorker = new FetchWorker('my-fetch-worker', {
  port: 1234
})

Worker thread:

addEventListener('fetch', event => {
  return event.respondWith(new Response('Hello!'));
});

I feel like most of the interop cases where we'd benefit from having globalThis extend EventTarget are related to ServiceWorker features. Can anyone think of a case where we'd actually want that in the main thread?

[KhafraDev]

Can anyone think of a case where we'd actually want that in the main thread?

Same uses as process emitting events. I wouldn't call process a good alternative though, considering only one platform supports it. It could technically be used as an alternative to diagnostics_channel as well (FetchEvent is a good example of being able to use the global event listener to debug/log/intercept requests made).

[jimmywarting]

Can anyone think of a case where we'd actually want that in the main thread?

to listen to any of this events:

• offline
• online
• unhandledpromise
• load
• beforeunload
• unload
• unhandledrejection
• fetch and message (but this only applies to worker threads)
• push <- this would be so awesome to have (even in servers), imagine if databases, redis, queue scheduler jobs, and even electron apps like slack could all receive push events. Don't have to create a server, expose any ports or share any public ip. The data that is transfered is end-to-end encrypted. so a server who takes care of push messages can't read them. And push messages would be delayed/queued up if subscriber is offline until it can start listening to push event again when going online. it would even be cooler if you could register native OS push events, then you would not have to spin up a own little demon tool to listen for any google FCM. you could depend on something to invoke a NodeJS script.

[benjamingr]

What's unhandledpromise 🤔 ?

[tniessen]

Many of these events are questionable in the context of Node.js (e.g., offline).

[KhafraDev]

I think I mentioned it in the PR, but the only events node would probably want to add would be the same as Deno: https://deno.com/manual@v1.34.0/runtime/program_lifecycle#program-lifecycle

[jcbhmr]

To quote myself from #45981 for those who are subscribed to this PR thread:

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.

[aduh95]

What's unhandledpromise 🤔 ?

benjamingr https://developer.mozilla.org/en-US/docs/Web/API/Window/unhandledrejection_event

That link doesn't mention any unhandledpromise event, it refers to unhandledrejection (which is in your list), so I guess it's fair to assume it's some kind of LLM hallucination or something, anyway that's beside your point no need to discuss it further. I suggest we hide those comments as off-topic, and if you can edit your comment to remove the reference to unhandledpromise, that should help keep the conversation productive hopefully.

[benjamingr]

(fwiw: There existed such an event in some spec drafts and discussions "back then" around 2015-2017 and I was curious if browsers ended up adding some sort of promise tracking eventually (which they were opposed to)).

I am happy to hide these comments as off topic and I also think we need to open an issue or discussion rather than having this discussion in the PR since none of it is about the actual code in the PR

edit: here: #45981

[jasnell]

I'm still generally -1 on this given the confusion it is likely to cause with process events and a non-trivial risk of breaking stuff (adding globals is always risky, changing the global prototype even more so). I do think there's a good conversation to be had about making the ShadowRealm globalThis extend EventTarget, however.

[jimmywarting]

How about being able to opt into it like two have already suggested?

Having it behind a flag and potentially only try it out in a un-even NodeJS release...
let it be disabled by default
Maybe even having something crazy as:

import `node:util/attach-eventtarget-to-global`
/* 
Would basically change the prototype of globalThis with Object.setPrototypeOf(...)
and add postMessage, addEventListener, removeEventListener, dispatchEvent
doing globalThis instanceof EventTarget would also be true then after importing this util
*/

but this seems very unconventional and weird? think a flag would be better to have

[jasnell]

If someone really wanted to opt in to that they could fairly easily do...

Object.setPrototypeOf(globalThis, new EventTarget());

[jcbhmr]

@jasnell

The this doesn't default to globalThis. I think that's part of this PR? event_target.js

[KhafraDev]

Yes, somewhere in the webidl spec (too lazy to look it up, sorry edit: it's actually in my original post, https://webidl.spec.whatwg.org/#dfn-create-operation-function) it says to default this to globalThis when it's null/undefined, which is in this PR. However, if we don't set globalThis' prototype, it serves no purpose, or might even cause unexpected behaviors:

// Emits an event in browsers/Deno/etc., but does nothing in node.
EventTarget.prototype.dispatchEvent.call(globalThis, new Event('fetch'))