nodejs / node

Node.js JavaScript runtime ✨🐢🚀✨
https://nodejs.org
Other
107.69k stars 29.64k forks source link

Revisiting `globalThis` as an `EventTarget` #51372

Open jasnell opened 10 months ago

jasnell commented 10 months ago

In browsers, deno, bun, workers and other runtimes, globalThis implements the Web platform standard EventTarget API. While there are differences in the specific events emitted at the global scope, the unhandledrejection and rejectionhandled events are common to all of these runtimes. In Node.js our globalThis, of course, does not implement EventTarget and implements unhandled rejection events using EventEmitter instead. This ends up causing some interop headaches for library authors who have to special case handling of these events across runtimes.

We've discussed this before but I'd like to re-open the discussion for consideration around the questions:

@nodejs/tsc @nodejs/web-standards

Context: this came up in a WinterCG discussion around whether interoperable/consistent handling of unhandled errors and rejections should be covered by the WinterCG common minimum API spec.

KhafraDev commented 10 months ago

I'm a +1 and I have a PR that implementes this already https://github.com/nodejs/node/pull/45993

jasnell commented 10 months ago

Yep, I was previously -1 on that change because I do think it would introduce some confusion but I've reconsidered my point of view and I think I'm +0 on it... that is, I'm open to it but I still have a number of reservations.

mcollina commented 10 months ago

I’m -1 as I think it would add more confusion. None of those mechanisms are correct for library authors.

Most libraries should not touch global objects, because they can conflict in behaviors.

For library authors, the problem is that more often you want to “execute something only if OBJ was not GCed” to avoid memory leaks on global objects. This is currently hacky, widespread and problematic with the use of FinalizationRegistry.

Application builders should use these events, not library authors. Application builders target a specific runtime, so there is not much of a compatibility issue to discuss.

jasnell commented 10 months ago

I disagree. We have application builders targeting multiple runtimes that have explicitly raised these kinds of compat issues.

Also, "libraries should not touch global objects" is obviously incorrect when it comes to other kinds of globals (URL, crypto.subtle, Buffer, etc).

GeoffreyBooth commented 10 months ago

Application builders should use these events, not library authors. Application builders target a specific runtime, so there is not much of a compatibility issue to discuss.

What is an "application builder"? There are plenty of full-stack frameworks that explicitly target multiple runtimes, like SvelteKit has adapters to run in Node or Cloudflare etc. I'm sure the SvelteKit authors would appreciate as much similarity between those targets as possible.

eligrey commented 10 months ago

Should we support other Web standard events on globalThis including: message (equivalent to process' message event)

Supporting message in this way sounds dangerous unless Node's message events are fully compatible with browser MessageEvents.

mcollina commented 10 months ago

Also, "libraries should not touch global objects" is obviously incorrect when it comes to other kinds of globals (URL, crypto.subtle, Buffer, etc).

Most of those do not allow changing configuration that alter how the process work or is set up.

The problem I'm describing happens when installing custom behavior. Most libraries out there would not install an uncaughtException or exit or beforeExit handlers, because they will actually leak memory in most situations. Most of the times adding those in libraries introduce unexpected behaviors for the end users.

mcollina commented 10 months ago

What is an "application builder"?

A developer that creates an application, vs one that creates a library to be installed.

There are plenty of full-stack frameworks that explicitly target multiple runtimes, like SvelteKit has adapters to run in Node or Cloudflare etc. I'm sure the SvelteKit authors would appreciate as much similarity between those targets as possible.

I've done a quick skim of SvelteKit repo and I haven't found an "uncaughtException" event handler for example. Could you point me to a real-world examples of the complexities of handling this case for those libraries.

mcollina commented 10 months ago

I disagree. We have application builders targeting multiple runtimes that have explicitly raised these kinds of compat issues.

This is an interesting use case, and I never tried to build one. Could you make an example of the compatibility problems?

joyeecheung commented 10 months ago

I think we should only implement the events that are exposed to Worker/global scopes in https://html.spec.whatwg.org/multipage/indices.html#events-2 . message and messageerror would make sense, unhandledrejection/rejectionhandled/error less so, and beforeunload/load shouldn't be there. In general, exposing things that are available to Window but not to Worker in the spec can be problematic, because Node.js mostly implement worker scope things but not the window scope things.