EventEmitter/EventTarget integration

[legendecas]

Events are widely used in async operations, e.g. Host APIs like Node.js EventEmitters Server, Stream, and web browser EventTargets XMLHttpRequest.

Expectations can be that the invocation of addEventListener acts as a creation of an async resource and capture the current async context frame to be used when the event is emitted.

We could make this an option passed into addEventListener which would essentially be a convenient path for manually calling AsyncContext.wrap on the handler function.

eventTarget.addEventListener('foo', () => {}, {
  captureAsyncContext: true,
});

Originally posted by @jasnell and @jridgewell.

[benjamingr]

eventTarget.addEventListener('foo', () => {}, { captureAsyncContext: true, });

I don't think WHATWG would be strictly opposed to this (I might be wrong) and Node certainly wouldn't be opposed - but I'm wondering if the spec shouldn't provide abstract hooks the host environment can override for deciding how to propagate context and then WHATWG can specify that propagation.

Then stuff like:

Expectations can be that the invocation of addEventListener acts as a creation of an async resource and capture the current async context frame to be used when the event is emitted.

Would be specced and mandated by WHATWG using the host hooks.

[bakkot]

We could make this an option passed into addEventListener which would essentially be a convenient path for manually calling AsyncContext.wrap on the handler function.

There's actually a bit of a difference there (potentially), since removeEventListener keys off of the identity of the function, and AsyncContext.wrap gives you a thing with a different identity.

If captureAsyncContext is an option, then presumably the same option would need to be passed to removeEventListener in order to remove that function, yes? (The same as for the capture option.) That is:

target.addEventListener(evt, fn, { captureAsyncContext: true });
// ...
target.removeEventListener(evt, fn); // does nothing because the `captureAsyncContext` value doesn't match
target.removeEventListener(evt, fn, { captureAsyncContext: true }); // works

Alternatively, per https://github.com/tc39/proposal-async-context/issues/22, event listeners could be always implicitly wrapped.

[benjamingr]

There's actually a bit of a difference there (potentially), since removeEventListener keys off of the identity of the function, and AsyncContext.wrap gives you a thing with a different identity.

That's a good point

[jasnell]

The situation is really no different than once wrappers.

[benjamingr]

The situation is really no different than once wrappers.

I thought that but Kevin's point about EventTarget using function equality made me reconsider since it means removeEventListener with a reference to the (unwrapped) function doesn't work and wrapping the function outside the addEventListener is a little foot-gunny since people might call it and it'd behave unexpected isn't it?

[jasnell]

Our implementations of both EventEmitter and EventTarget already use wrappers. This really wouldn't be any different.

[benjamingr]

Our implementations of both EventEmitter and EventTarget already use wrappers. This really wouldn't be any different.

Can you elaborate on what you mean here?

[Jamesernator]

I strongly think that events should use registration time by default. Like from a user's perspective if we have some code like:

someScheduler.runTask("highest-priority", async () => {
    // some preparation work

    window.addEventListener("load", async function callback() {
        // setup...
        await someTask();
        // more setup...
    });
});

then it would be rather surprising if callback wasn't executed with the same priority as the prior code.

Further basically all other callback functions are presumed to use registration time:

const variable = new AsyncContext.Variable();

// All of these will print "REGISTRATION TIME"
variable.run("REGISTRATION TIME", () => {
   queueMictrotask(() => console.log(variable.get()));
   setTimeout(() => console.log(variable.get()));
   somePromise.then(() => console.log(variable.get()));
});

and from most user's perspectives, events are just another callback API, there's no reason they should be special in anyway to others.

Having an option like { captureAsyncContext: ... } isn't really that good because most users won't even know what async contexts are. Like the majority of async context code will be part of libraries like schedulers, tracers, etc, it shouldn't be down to the end users of these libraries to know all the nuances of what callback APIs capture contexts and which don't.

Like the core motivation of this proposal is that it is too much of a pain for users to thread certain dependencies (like tracers/schedulers) through a bunch of async code, having users need to be aware of the async context basically defeats the major point of the feature. If users can pass { captureAsyncContext: true } then it is equally easy for them to do { priority: scheduler.activePriority }, but the presumption is that users aren't going to do that.

(Highly related #22).