events: initial implementation of experimental EventTarget #33556

jasnell · 2020-05-25T16:07:13Z

Initial experimental implementation of EventTarget.

This is an adaptation of the Web API EventTarget modified to conform to Node.js' needs. The details are explained in the documentation edits included in this commit.

Refs: #33527

/cc @benjamingr

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

mcollina

I'm concerned by this implementation and its impact about CPU and Memory usage in case EventTarget becomes popular for Node.js applications.

I'm not necessarily ok in having them exposed as globals from the get-go, but I'm not totally opposed either.

lib/internal/event_target.js

doc/api/events.md

lib/internal/event_target.js

doc/api/events.md

lib/internal/event_target.js

benjamingr · 2020-05-25T17:13:15Z

test/parallel/test-eventtarget.js

@@ -0,0 +1,229 @@
+'use strict';


Let me know if you want me to start pushing tests from the WPT and/or browsers here, at another branch or elsewhere.

@benjamingr if you'd like to do this, PTAL at https://github.com/nodejs/node/tree/master/test/wpt#how-to-add-tests-for-a-new-module

@targos that's super useful thanks, I didn't know we already had a clear process documented in line for this. Here are the wpt tests for events, some applicable.

Let's hold off, but it would be good to know which WPT tests for EventTarget and Event we don't pass.

benjamingr · 2020-05-25T17:28:25Z

Thank you for doing this, I will take it for a longer spin (I have a backlog from the AbortController API list PR) :]

The implementation itself and not doing all the cargo-culting some other implementation did and generally looks good. I think it would be good to get some whatwg eyes on it. In particular Anne and Domenic were helpful in the EventEmitter vs. EventTarget doc.

I also think there is a big win in passing at least some of the WPT and keeping track of how good our implementation is, even if we don't claim compliance. Neither of these things has to happen right now though.

I would strongly prefer not exposing EventTarget as a global at the first stage and let users create it because that sounds like a potential big footgun (like Matteo mentioned).

I think that would let us land EventTarget sooner. I am obviously not blocking in either case and I'd like to echo my comment from the AbortController PR:

I want to be clear that I want to be helpful here so since you took the initiative if at any point any of the things I'm doing are frustrating just tell me and I'll stop and do whatever is helpful to the effort instead. These sorts of ongoing efforts tend to become frustrating in Node.js at times, I want to be sure that I'm not doing any toe-stepping if I can help it.

doc/api/events.md

benjamingr

LGTM as is (after playing with it for 1-2 hours and carefully reading the code - so not rubber stamp) given Event and EventTarget aren't exposed and the module is marked as experimental.

I am not sure about a lot of the semantics here and am playing with it quite a bit (like the error semantic and exposing things like remoteListener) but none of them are set in stone and I'm a strong +1 on shipping this in some form.

I see a lot of next steps (running wpt, research etc) but I'm very excited to see this happen.

jasnell · 2020-05-25T19:21:14Z

The error semantics are definitely something that need to be carefully considered here. In the current implementation, if handler does error, that is not emitted on process.on('error') until process.nextTick(), which means it does not interrupt the execution of the handlers or the current execution scope.

domenic

It's pretty frustrating to see this get all these non-standard extensions, missing functionality, and semantic mismatches, as compared to the web API. This will just lead to a third type of event emitter in the ecosystem: standard EventTarget, EventEmitter, and Node-flavored EventTarget.

I'm especially sad to see this after offline conversations with @benjamingr indicated that he disliked Deno's approach of not following the standards (*), and wanted to follow the standard if Node were to adopt this.

If the plan is to not follow the standards (i.e., implement the interfaces and algorithms as-written, except omitting unobservable parts of the algorithms which only exist to support event targets with special "click" handling or with non-null "get the parent" algorithms) then I'd strongly suggest naming this something different than EventTarget.

(*): I haven't verified whether this is true or not for Deno's implementation.

doc/api/errors.md

doc/api/events.md

domenic · 2020-05-25T21:41:19Z

doc/api/events.md

+   a hierarchy of nested target objects that may each have their own set of
+   handlers for the event.
+2. In the Node.js `EventTarget`, any object with a `type` property whose value
+   is a string may be dispatched as an `event`.


That's a frustrating divergence that would lead to non-interoperable code.

Understood. This was included specifically because userland implementations support it. Whether we end up supporting it here or not is still to be determined.

@domenic how would you suggest node deals with shims and libraries?

Would a custom [Symbol.event] or something else be preferable as a protocol?

Ideally I'd want something that both doesn't create a divergence and does allow third-party libraries to create Events (like dispatchEventing an Event created from JSDom).

The ability to support third-party polyfill type events is exactly the motivation here, fwiw. Especially since we're not exporting the Event constructor right away.

I'd suggest exposing the Event constructor, so that libraries can write code that works in both Node and the DOM, instead of more Node-specific code. After all, if the goal is Node-specific code, EventEmitter is right there.

Noted. We may get there eventually but for now the experimental implementation won't be exposed to user code so we have some wiggle room here.

It'll be exposed via AbortSignal.__proto__ right?

If the goal is to be able to dispatch an Event from e.g. JSDOM, I don't see how that could work with requiring a specific constructor, since JSDOM would hardly use the global Event from Node (for isolation, and because it cannot implement all the DOM stuff JSDOM definitely needs). Exposing the constructor alone would not solve the interop problem

That doesn't make sense as a goal, since JSDOM implements its own EventTarget and would purposefully not want to interoperate with Node's (as you said, for isolation).

@jasnell I think I see Domenic's point here regarding branding, iiuc the API explicitly requires subclasses of Event to prevent this sort of interop. Supporting this would violate API guarantees.

Is it horrible? No, but it would make writing universal code harder and not easier probably.

I see value in adding interop when we have a compelling use case. If people feel strongly about not supporting it unless you feel strongly we should for the very least let's not document it in the meantime.

I also think it's fine to only support it in NodeEventEmitter with the extensions when the user does emit and not dispatchEvent for example.

doc/api/events.md

benjamingr · 2020-05-25T22:05:38Z

@domenic to clarify: the idea as far as I understand is to follow the standards and to eventually pass the WPTs (where applicable). Note that this PR doesn't actually ship EventTarget anywhere - it adds it experimentally inside Node.js without exposing the Event constructor or EventTarget constructor with the idea being initially only AbortController implementing it (probably).

As far as I understand this PR is a good step towards API compatibility.

I assumed that it is possible to ship EventTarget with these extensions (can/should node ship a subclass?). There is an ongoing discussion about this, please don't assume we are acting in bad faith here.

We are trying to figure out what's the best way to proceed and eventually have these APIs in Node.js, I asked for your opinion because I value it and want to hear what you think Node.js should do.

jasnell · 2020-05-25T22:58:39Z

Updated to separate out NodeEventTarget from EventTarget

jasnell · 2020-05-25T23:17:41Z

@domenic:

It's pretty frustrating to see this get all these non-standard extensions, missing functionality, and semantic mismatches, as compared to the web API.

Keep in mind that this is a work in progress experimental implementation and a key part of this initial discussion is to explore requirements. There is quite a bit of the browser event API that just doesn't make sense in Node.js and implementing everything is not worthwhile for anyone.

It's worthwhile going through the stuff in Event that doesn't seem to make sense here:

Event:

~~Option: bubbles - There is no concept of bubbling/propagating events in Node.js. This option would be a non-op~~ Added. We preserve it but don't use it.
~~Option: composed - There is no concept of shadow roots in Node.js. This option would be a non-op~~ Added. We preserve it but don't use it.
~~srcElement, currentTarget, target - We don't currently have a notion of this in Node.js. Once we do, we can easily add these.~~ Added.
~~composedPath() - There is no concept of an invocation path with Node.js events. It wouldn't make sense to implement this.~~ Added such that it always returns and empty array while the Event is not be dispatched and an array with the EventTarget while it is being dispatched. This differs from the behavior in Chrome but matches the behavior in Firefox so we should discuss.
~~eventPhase - There is no concept of event phases in Node.js and no propagation model. It wouldn't make sense to implement this.~~ Added such that it always returns 0 while the event is not being dispatched, 2 while it is.
~~stopPropagation()/cancelBubble() - There is no concept of propagation. It wouldn't make sense to implement this.~~ Added, as a non-op
~~stopImmediatePropagation() - This one we definitely should implement~~ Added
~~returnValue - Legacy, doesn't make sense to implement~~ Added. Returns !defaultPrevented
~~bubbles~~. Added. Always returns false
~~composed - No concept of composition~~ Added. Always returns false
~~isTrusted - No differentiation between trusted/untrusted events~~. Added. Always returns false
initEvent() - We could implement this if it makes sense to do so

domenic · 2020-05-25T23:23:17Z

with the idea being initially only AbortController implementing it (probably).

It would also be exposed through AbortSignal.__proto__.

I assumed that it is possible to ship EventTarget with these extensions (can/should node ship a subclass?)

I'm not sure what you mean by "possible to ship", but it's not possible to be spec-compliant when you add additional members to a class (static or instance).

It's worthwhile going through the stuff in Event that doesn't seem to make sense here:

I agree. I went through a similar exercise with @benjamingr over chat.

I thought the idea was to support things that are no-ops/trivial in non-hierarchical cases, so as to be spec-complaint. Not to remove them. It seems to create gratuitous incompatibilities between Node and browser if simple code like (new Event("foo", { bubbles: true }).bubbles returns different values in Node and in the browser, when it could easily return the same value in both.

jasnell · 2020-05-26T00:00:56Z

With regards to non-op properties like bubbles, regardless of whatever value is passed in at the constructor I'd rather the getters just always return false.

domenic · 2020-05-26T00:25:38Z

With regards to non-op properties like bubbles, regardless of whatever value is passed in at the constructor I'd rather the getters just always return false.

Why? In the browser, they still return what's passed in, even when the event is used with a null get-the-parent EventTarget.

In general, I'm really unclear why not just follow the spec?

bmeck

pending changes to .target dispatching per comments

See documentation changes for details Signed-off-by: James M Snell <jasnell@gmail.com>

nodejs-github-bot · 2020-05-27T20:28:41Z

CI: https://ci.nodejs.org/job/node-test-pull-request/31583/

jasnell · 2020-05-27T20:31:37Z

Updated to remove the allowance of { type: 'foo'} pseudo-Events.

domenic · 2020-05-27T20:34:52Z

Very excited with how this is turning out. Thanks for bearing with me and my comments. It's going to be amazing to have a spec-compliant EventTarget in Node (whenever this is eventually deemed ready to expose).

nodejs-github-bot · 2020-05-27T22:38:37Z

CI: https://ci.nodejs.org/job/node-test-pull-request/31584/

See documentation changes for details Signed-off-by: James M Snell <jasnell@gmail.com> PR-URL: #33556 Refs: #33527 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Bradley Farias <bradley.meck@gmail.com>

jasnell · 2020-05-28T13:34:54Z

Landed in 785842a. thanks all.

@benjamingr : I'm going to work on finishing up the AbortController PR next if you want to start pushing on the WPT and interop tweaks necessary for this. I'm still debating whether we are actually going to need the remove all functions on NodeEventTarget

See documentation changes for details Signed-off-by: James M Snell <jasnell@gmail.com> PR-URL: #33556 Refs: #33527 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Bradley Farias <bradley.meck@gmail.com>

Notable changes: build: * (SEMVER-MINOR) reset embedder string to "-node.0" (Michaël Zasso) #33376 cli: * (SEMVER-MINOR) add alias for report-directory to make it consistent (AshCripps) #33587 crypto: * (SEMVER-MINOR) allow KeyObjects in postMessage (Tobias Nießen) #33360 deps: * (SEMVER-MINOR) V8: cherry-pick 0d6debcc5f08 (Michaël Zasso) #33376 * (SEMVER-MINOR) update V8 to 8.3.110.9 (Michaël Zasso) #33376 dgram: * (SEMVER-MINOR) allow typed arrays in .send() (Sarat Addepalli) #22413 events: * (SEMVER-MINOR) initial implementation of experimental EventTarget (James M Snell) #33556 fs: * (SEMVER-MINOR) implement lutimes (Maël Nison) #33399 http: * (SEMVER-MINOR) expose host and protocol on ClientRequest (wenningplus) #33803 * (SEMVER-MINOR) add maxTotalSockets to agent class (rickyes) #33617 * (SEMVER-MINOR) return this from OutgoingMessage#destroy() (Colin Ihrig) #32789 * (SEMVER-MINOR) return this from ClientRequest#destroy() (Colin Ihrig) #32789 * (SEMVER-MINOR) return this from IncomingMessage#destroy() (Colin Ihrig) #32789 * (SEMVER-MINOR) added scheduling option to http agent (delvedor) #33278 http2: * (SEMVER-MINOR) return this for Http2ServerRequest#setTimeout (Pranshu Srivastava) #33994 * (SEMVER-MINOR) do not modify explicity set date headers (Pranshu Srivastava) #33160 process: * (SEMVER-MINOR) add unhandled-rejection throw and warn-with-error-code (Dan Fabulich) #33475 src: * (SEMVER-MINOR) store key data in separate class (Tobias Nießen) #33360 * (SEMVER-MINOR) add NativeKeyObject base class (Tobias Nießen) #33360 * (SEMVER-MINOR) rename internal key handles to KeyObjectHandle (Tobias Nießen) #33360 * (SEMVER-MINOR) add equality operators for BaseObjectPtr (Anna Henningsen) #33772 * (SEMVER-MINOR) introduce BaseObject base FunctionTemplate (Anna Henningsen) #33772 * (SEMVER-MINOR) add public APIs to manage v8::TracingController (Anna Henningsen) #33850 win: * (SEMVER-MINOR) allow skipping the supported platform check (João Reis) #33176 worker: * (SEMVER-MINOR) add public method for marking objects as untransferable (Anna Henningsen) #33979 * (SEMVER-MINOR) emit `'messagerror'` events for failed deserialization (Anna Henningsen) #33772 * (SEMVER-MINOR) allow passing JS wrapper objects via postMessage (Anna Henningsen) #33772 * (SEMVER-MINOR) allow transferring/cloning generic BaseObjects (Anna Henningsen) #33772 worker,fs: * (SEMVER-MINOR) make FileHandle transferable (Anna Henningsen) #33772 zlib: * (SEMVER-MINOR) add `maxOutputLength` option (unknown) #33516 PR-URL: #34093

Notable changes: build: * (SEMVER-MINOR) reset embedder string to "-node.0" (Michaël Zasso) #33376 cli: * (SEMVER-MINOR) add alias for report-directory to make it consistent (AshCripps) #33587 crypto: * (SEMVER-MINOR) allow KeyObjects in postMessage (Tobias Nießen) #33360 deps: * (SEMVER-MINOR) V8: cherry-pick 0d6debcc5f08 (Michaël Zasso) #33376 * (SEMVER-MINOR) update V8 to 8.3.110.9 (Michaël Zasso) #33376 dgram: * (SEMVER-MINOR) allow typed arrays in .send() (Sarat Addepalli) #22413 events: * (SEMVER-MINOR) initial implementation of experimental EventTarget (James M Snell) #33556 fs: * (SEMVER-MINOR) implement lutimes (Maël Nison) #33399 http: * (SEMVER-MINOR) expose host and protocol on ClientRequest (wenningplus) #33803 * (SEMVER-MINOR) add maxTotalSockets to agent class (rickyes) #33617 * (SEMVER-MINOR) return this from OutgoingMessage#destroy() (Colin Ihrig) #32789 * (SEMVER-MINOR) return this from ClientRequest#destroy() (Colin Ihrig) #32789 * (SEMVER-MINOR) return this from IncomingMessage#destroy() (Colin Ihrig) #32789 * (SEMVER-MINOR) added scheduling option to http agent (delvedor) #33278 http2: * (SEMVER-MINOR) return this for Http2ServerRequest#setTimeout (Pranshu Srivastava) #33994 * (SEMVER-MINOR) do not modify explicity set date headers (Pranshu Srivastava) #33160 process: * (SEMVER-MINOR) add unhandled-rejection throw and warn-with-error-code (Dan Fabulich) #33475 src: * (SEMVER-MINOR) store key data in separate class (Tobias Nießen) #33360 * (SEMVER-MINOR) add NativeKeyObject base class (Tobias Nießen) #33360 * (SEMVER-MINOR) rename internal key handles to KeyObjectHandle (Tobias Nießen) #33360 * (SEMVER-MINOR) add equality operators for BaseObjectPtr (Anna Henningsen) #33772 * (SEMVER-MINOR) introduce BaseObject base FunctionTemplate (Anna Henningsen) #33772 * (SEMVER-MINOR) add public APIs to manage v8::TracingController (Anna Henningsen) #33850 stream*: * runtime deprecate Transform._transformState (Robert Nagy) #32763 win: * (SEMVER-MINOR) allow skipping the supported platform check (João Reis) #33176 worker: * (SEMVER-MINOR) add public method for marking objects as untransferable (Anna Henningsen) #33979 * (SEMVER-MINOR) emit `'messagerror'` events for failed deserialization (Anna Henningsen) #33772 * (SEMVER-MINOR) allow passing JS wrapper objects via postMessage (Anna Henningsen) #33772 * (SEMVER-MINOR) allow transferring/cloning generic BaseObjects (Anna Henningsen) #33772 worker,fs: * (SEMVER-MINOR) make FileHandle transferable (Anna Henningsen) #33772 zlib: * (SEMVER-MINOR) add `maxOutputLength` option (unknown) #33516 PR-URL: #34093

See documentation changes for details Signed-off-by: James M Snell <jasnell@gmail.com> PR-URL: #33556 Refs: #33527 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Bradley Farias <bradley.meck@gmail.com>

Notable changes: build: * (SEMVER-MINOR) reset embedder string to "-node.0" (Michaël Zasso) #33376 cli: * (SEMVER-MINOR) add alias for report-directory to make it consistent (AshCripps) #33587 crypto: * (SEMVER-MINOR) allow KeyObjects in postMessage (Tobias Nießen) #33360 deps: * (SEMVER-MINOR) V8: cherry-pick 0d6debcc5f08 (Michaël Zasso) #33376 * (SEMVER-MINOR) update V8 to 8.3.110.9 (Michaël Zasso) #33376 dgram: * (SEMVER-MINOR) allow typed arrays in .send() (Sarat Addepalli) #22413 events: * (SEMVER-MINOR) initial implementation of experimental EventTarget (James M Snell) #33556 fs: * (SEMVER-MINOR) implement lutimes (Maël Nison) #33399 http: * (SEMVER-MINOR) expose host and protocol on ClientRequest (wenningplus) #33803 * (SEMVER-MINOR) add maxTotalSockets to agent class (rickyes) #33617 * (SEMVER-MINOR) return this from OutgoingMessage#destroy() (Colin Ihrig) #32789 * (SEMVER-MINOR) return this from ClientRequest#destroy() (Colin Ihrig) #32789 * (SEMVER-MINOR) return this from IncomingMessage#destroy() (Colin Ihrig) #32789 * (SEMVER-MINOR) added scheduling option to http agent (delvedor) #33278 http2: * (SEMVER-MINOR) return this for Http2ServerRequest#setTimeout (Pranshu Srivastava) #33994 * (SEMVER-MINOR) do not modify explicity set date headers (Pranshu Srivastava) #33160 process: * (SEMVER-MINOR) add unhandled-rejection throw and warn-with-error-code (Dan Fabulich) #33475 src: * (SEMVER-MINOR) store key data in separate class (Tobias Nießen) #33360 * (SEMVER-MINOR) add NativeKeyObject base class (Tobias Nießen) #33360 * (SEMVER-MINOR) rename internal key handles to KeyObjectHandle (Tobias Nießen) #33360 * (SEMVER-MINOR) add equality operators for BaseObjectPtr (Anna Henningsen) #33772 * (SEMVER-MINOR) introduce BaseObject base FunctionTemplate (Anna Henningsen) #33772 * (SEMVER-MINOR) add public APIs to manage v8::TracingController (Anna Henningsen) #33850 stream*: * runtime deprecate Transform._transformState (Robert Nagy) #32763 win: * (SEMVER-MINOR) allow skipping the supported platform check (João Reis) #33176 worker: * (SEMVER-MINOR) add public method for marking objects as untransferable (Anna Henningsen) #33979 * (SEMVER-MINOR) emit `'messagerror'` events for failed deserialization (Anna Henningsen) #33772 * (SEMVER-MINOR) allow passing JS wrapper objects via postMessage (Anna Henningsen) #33772 * (SEMVER-MINOR) allow transferring/cloning generic BaseObjects (Anna Henningsen) #33772 worker,fs: * (SEMVER-MINOR) make FileHandle transferable (Anna Henningsen) #33772 zlib: * (SEMVER-MINOR) add `maxOutputLength` option (unknown) #33516 PR-URL: #34093

ricardobeat · 2020-07-06T12:57:49Z

Is there a background discussion that has happened regarding this, or any docs that can be referenced?

For end users the motivation for adding a web-compatible API to Node itself, in lieu of EventEmitter, is not clear from the documentation or discussion in this PR.

jasnell · 2020-07-06T13:46:23Z

The immediate motivation was to provide support for the experimental AbortController API, which we are beginning to use in select areas to make Promise-based APIs cancelable.

addaleax · 2020-09-22T20:36:14Z

@jasnell @benjamingr I’ve marked this backport-requested-v12.x. If this is backported, I think it should probably be coupled with at least:

(Most of these will probably apply cleanly after the first backports.)

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label May 25, 2020

jasnell mentioned this pull request May 25, 2020

lib: initial experimental AbortController implementation #33527

Closed

4 tasks

mcollina reviewed May 25, 2020

View reviewed changes

lib/internal/event_target.js Outdated Show resolved Hide resolved

felixfbecker reviewed May 25, 2020

View reviewed changes

lib/internal/event_target.js Outdated Show resolved Hide resolved

ptomato reviewed May 25, 2020

View reviewed changes

doc/api/events.md Outdated Show resolved Hide resolved

doc/api/events.md Outdated Show resolved Hide resolved

felixfbecker reviewed May 25, 2020

View reviewed changes

lib/internal/event_target.js Outdated Show resolved Hide resolved