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

Revisiting globalThis as an EventTarget #51372

Open
jasnell opened this issue Jan 4, 2024 · 10 comments
Open

Revisiting globalThis as an EventTarget #51372

jasnell opened this issue Jan 4, 2024 · 10 comments

Comments

@jasnell
Copy link
Member

jasnell commented Jan 4, 2024

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:

  • Should Node.js globalThis implement EventTarget
  • Should we emit the Web standard unhandledrejection and rejectionhandled events on globalThis and either (a) deprecate or (b) mark as legacy the equivalent events on process?
  • Should we support other Web standard events on globalThis including:
    • beforeunload (equivalent to process' beforeExit event)
    • error (equivalent to process' uncaughtException event
    • load
    • message (equivalent to process' message event)
    • messageerror
    • etc

@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
Copy link
Member

I'm a +1 and I have a PR that implementes this already #45993

@jasnell
Copy link
Member Author

jasnell commented Jan 4, 2024

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 mcollina added the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Jan 4, 2024
@mcollina
Copy link
Member

mcollina commented Jan 4, 2024

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
Copy link
Member Author

jasnell commented Jan 4, 2024

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
Copy link
Member

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
Copy link

eligrey commented Jan 4, 2024

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
Copy link
Member

mcollina commented Jan 5, 2024

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
Copy link
Member

mcollina commented Jan 5, 2024

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
Copy link
Member

mcollina commented Jan 5, 2024

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?

@mcollina mcollina removed the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Jan 10, 2024
@joyeecheung
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants