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

once() and on() async utilities for EventTarget #1038

Open
jasnell opened this issue Dec 1, 2021 · 23 comments
Open

once() and on() async utilities for EventTarget #1038

jasnell opened this issue Dec 1, 2021 · 23 comments
Labels
addition/proposal New features or enhancements topic: events

Comments

@jasnell
Copy link
Contributor

jasnell commented Dec 1, 2021

A while back Node.js introduced the once() and on() utility functions for Node.js' EventEmitter object. They are essentially just promise-based alternatives to adding a callback listener for an event. They've proven to be so useful in practice that I'd like to propose adding similar utilities for EventTarget

Essentially,

const ac = new AbortController();
const eventTarget = new EventTarget();
setTimeout(() => eventTarget.dispatchEvent(new Event('foo')), 1000);
// EventTarget.once() returns a Promise that is resolved the next time the event is dispatched.
await EventTarget.once(eventTarget, 'foo', { signal: ac.signal }); 

and

const ac = new AbortController();
const eventTarget = new EventTarget();
setInterval(() => eventTarget.dispatchEvent(new Event('foo')), 1000);
// EventTarget.on() returns an async iterator that iterates each time the event is dispatched.
for await (const ev of EventTarget.on(eventTarget, 'foo', { signal: ac.signal })) {
  // ...
}
@domenic domenic added topic: events addition/proposal New features or enhancements labels Dec 1, 2021
@domenic
Copy link
Member

domenic commented Dec 1, 2021

This seems like a nice addition, although it might be worth bikeshedding on the name and API surface. E.g. I'd personally do something more like eventTarget.next("foo") and eventTarget.toAsyncIterator("foo"); this style of static-methods-that-are-really-instance-methods is not something we generally do on the web platform.

If I recall, previous discussions in this vein have raised the worry that any developer code using such helpers would get access to the Event object too late to call preventDefault() or anything else that needs to work during the dispatch phase. That is kinda sad.

As some background, we've historically been "saving" EventTarget.prototype.on() for some "nicer" version of the EventTarget API. The latest attempt was #544 and I briefly floated https://glitch.com/edit/#!/angry-wound. It's been many years though.

@jasnell
Copy link
Contributor Author

jasnell commented Dec 1, 2021

Yeah, it definitely limits some of the interaction with the Event object but I think for the use cases this is intended for, that's likely ok? And where those are definitely needed then the sync addEventListener() option is still there.

next() and toAsyncIterator() work for me as names.

@Jamesernator
Copy link

Jamesernator commented Dec 2, 2021

I dunno what the general guidance is, but should web APIs that return async iterators be using ReadableStream instead? My assumption was that streams were meant to be the defacto primitive for operations that returned buffered data.

That would also cover all the behaviour of how things like the abort signal, queueing and such work, as these are all baked into the streams spec already.

@bathos
Copy link

bathos commented Dec 2, 2021

If API additions along these lines become a thing, I hope that API can be devised in a way that accounts for race/all patterns.

I've used homegrown promise/async-iterable DOM event utils similar to what's described above for a while and the most interesting thing I've learned from it has been that "race patterns" are pretty common with DOM events: "any of these events once." Usually they're events dispatched by a single target, but even so, opts.once is unhelpful for them: if anything, it makes it harder to spot the bug if you've failed to wire things up to cause removal of the other listeners when the first event is dispatched.

@annevk
Copy link
Member

annevk commented Dec 2, 2021

There's also https://github.com/tc39/proposal-emitter that has a bunch of ideas. I quite like the simplicity of this proposal though, makes it more tractable. I do think there are cases where you would want next() and cancelability, e.g., for the click event.

@Jamesernator
Copy link

I do think there are cases where you would want next() and cancelability, e.g., for the click event.

There are a few simple options, one would be just to accept a sync handler step i.e.:

const event = await target.once("some-event", { 
    handleSync: (event) => {
        if (someCondition) event.preventDefault();
    },
});

Alternatively once already works for DOM events if you immediately await it i.e.:

const event = await element.once("click");
// Still works
event.preventDefault();

However this does depend on the weird zalgo behaviour that events dispatched by the user agent have microtask checkpoints after firing event listeners but before finishing the event, this power can't be accessed by userland events in any way.

And although this behaviour is fairly zalgo, some libraries such as idb do actually depend on that exact behaviour†, so it's not particularly uncharted terrority.

† For that library specifically indexedDB fires events for transactions, once the event is over the transaction is no longer usable, however because it's a UA event one can schedule microtasks (e.g. via await) just fine and the transaction is still usable, as such the library is able to wrap things in promises and as long as the promises are immediately awaited everything works out.

Another another model could be that .once actually takes an optional callback function, however the return value of that callback is wrapped in a promise and returned i.e.:

const x = await element.once("click", async (event) => {
    // All code prior to an await is run synchronously, so event.preventDefault is fine here
    if (someCondition) event.preventDefault();
    return event.x;
});

This is considerably less zalgo than the above, but does require a somewhat different structure for code.

@jasnell
Copy link
Contributor Author

jasnell commented Dec 2, 2021

@Jamesernator:

should web APIs that return async iterators be using ReadableStream instead?

Requiring ReadableStream in this case would introduce significant additional and unnecessary overhead. I'd very much prefer to avoid that.

@bathos:

the most interesting thing I've learned from it has been that "race patterns" are pretty common with DOM events: "any of these events once."

Yes, we get those once in a while in Node.js also. The most common are 'close' and 'error' events, which tend to be emitted within the same process.nextTick() queue batch. If you're waiting on either 'close' or 'error', then you'll typically miss the other one. What we end up having to do is ensure that a synchronous event listener is set on one before we await the other. I like your suggestion here of "any of these events once", very similar to Promise.all() or Promise.race() in nature.

await eventTarget.next("foo");  // Wait asynchronously for only 'foo', other events emitted synchronously while waiting may be be missed.

await eventTarget.nextRace(["foo", "bar"]);  // Wait asynchronous for either 'foo' or 'race', other events emitted synchronously while waiting may be missed, including either 'foo' or 'bar' events.

The possibility of missing events is definitely very possible with this API and needs to be one of the main considerations when adding it.

@Jamesernator :

const x = await element.once("click", async (event) => {
   // All code prior to an await is run synchronously, so event.preventDefault is fine here
   if (someCondition) event.preventDefault();
   return event.x;
});

Hmm.. this is significantly less attractive as an option for me simply because it doesn't seem to be much better than just continuing to use addEventListener().

Great discussion tho!

@domenic
Copy link
Member

domenic commented Dec 2, 2021

+1 to async iterator over ReadableStream; ReadableStream is designed for I/O (see e.g. the FAQ, or the frequent analogy that arrays are specialized sync iterables for random access, whereas ReadableStreams are specialized async iterables for I/O).

I thought further on the naming question and think that although it's slightly verbose, if the names were toPromise() and toAsyncIterator() this would have a couple benefits:

  • Very explicit
  • Does not preclude using the clever/short names like once() or next() or on() for anything like Improving ergonomics of events with Observable #544 in the future, which would have full access to event semantics including cancelation
  • It should be somewhat obvious that, because promises and async iterators are async, event.preventDefault() won't work.

I agree that it's not really desirable to combine these proposals with cancelability; for that we should continue to use addEventListener() for now, or something cool like #544 later.

I think race and all patterns already work pretty well with the base proposal? You end up with an extra event listener hanging around that is ultimately ignored; that isn't fully optimal, but in most cases it shouldn't matter:

const e = await Promise.race([eventTarget.toPromise("foo"), eventTarget.toPromise("bar")]);

@bathos
Copy link

bathos commented Dec 2, 2021

but in most cases it shouldn't matter

That's true - probably doesn't matter for indexeddb success/error* for example - but it seems you can silently accrue a lot of extras when handling ui events for mouse/pointer/touch/drag behaviors

* in our homegrown version I eventually bit the bullet and said that "error" and "messageerror" events unilaterally map to rejections in the async-iterators-of-events, not yielded results. may not be a reasonable angle for built in behavior but fwiw, i've had no reason to regret it so far - in some cases it's radically more intuitive

@jasnell
Copy link
Contributor Author

jasnell commented Dec 3, 2021

@domenic

+1 to async iterator over ReadableStream; ReadableStream is designed for I/O

I just want to clarify this. Are you saying that we should build the async iterator piece on ReadableStream or we should not?

@domenic
Copy link
Member

domenic commented Dec 3, 2021

I meant we should not; you can s/over/instead of.

@jasnell
Copy link
Contributor Author

jasnell commented Dec 3, 2021

Ok, so I just want to summarize where we seem to be landing on this:

  1. Add a new EventTarget.prototype.toPromise(type[, options]) that returns a Promise that is resolved with undefined when the next event type is dispatched.
  • options will include:
    • signal - An AbortSignal that can be used to cancel waiting for the event type.
  • The capture and passive options would have no significant meaning here.
  1. Add a new EventTarget.prototype.toAsyncIterator(type[, options]) that returns an AsyncIterator that iterates every time the type event is dispatched.
  • options is the same as toPromise()

Using Promise.race() should handle the race conditions when there are multiple events that can be expected.

Does this seem correct?

@domenic
Copy link
Member

domenic commented Dec 3, 2021

Seems correct, although we should realize this discussion has moved fast and hasn't had time for everyone to weigh in.

Also

Using Promise.race() should handle the race conditions when there are multiple events that can be expected.

might deserve some more thought, per @bathos's comment. His footnote made me wonder about

const messageEvent = await window.toPromise("message", { errorType: "messageerror" });

or something?

... also, writing that out, toPromise() is now feeling less good as a name, because I forgot that usually EventTargets are actually objects with tons of non-event functionality (like Window), so it feels funny to say window.toPromise() as if we're converting the window itself into a promise :(. Same for toAsyncIterator(). Sigh, more bikeshedding needed...

  • window.eventAsync("message") / window.eventsAsync("message") ?
  • window.eventPromise("message") / window.eventIterator("message") ?

@jasnell
Copy link
Contributor Author

jasnell commented Dec 3, 2021

The naming issue there is why I originally was thinking about static methods off EventTarget directly... so instead of window.toPromise('event') it would be EventTarget.toPromise(window, 'event') .. but happy to keep bike shedding on it. It's important to get right.

@bathos
Copy link

bathos commented Dec 4, 2021

Spitballing follows. Not sure if maybe some of these are already unavailable due to collision given how many EventTarget subclasses there are.

Single

  • .when("blur")
  • .advent("hashchange")
  • .eventual("keydown") / .eventually("keydown")

Iterator

  • .events("message")
  • .ongoing("input")
  • .subscribe("submit")

None of the single-promise ones seem quite right: when doesn’t strongly imply “once,” eventually maybe sounds more like you’re asking the agent to schedule work, and advent, though nearly an exact match for the concept in other regards (even being from the same root word as event), too strongly implies the start of something (i.e., the very first event, not the next event). Still, maybe one of them will kick off better ideas.


(for a second I thought “oh! ‘await’!” before realizing why that’s a terrible method name — though at least no one could knock await video.await("waiting") for being unclear about its intention).

@jasnell
Copy link
Contributor Author

jasnell commented Dec 16, 2021

Again, just bikeshedding here, but we could flip this around a bit and hang the new API off Event instead.

const event = await Event.from(target, 'foo');

@Kaiido
Copy link
Member

Kaiido commented Dec 17, 2021

#1038 (comment)

that returns a Promise that is resolved with undefined when the next event type is dispatched.

Really? I can't find the rationale for this in this thread, is it to avoid .preventDefault() doing nothing and .currentTarget being null? Not being able to tell what type of Event we received which would prevent any use of Promise.race(), and not being able to tell on which target it fired nor to get any data from it seems like a bigger problem, no?

And to participate in the bikeshedding, I like to call my event promisifier waitForEvent, the iterator one could be waitForEvents.

@benlesh
Copy link

benlesh commented Jan 5, 2022

In general, I like this sort of idea. However, I'd caution against it because of the nuance involved with converting something that's entirely push-based (EventTarget) to something that is pull-push based (AsyncIterable). The latter is much more complicated and that complication/nuance is generally lost in async functions with for-await loops.

We looked at making RxJS's observable implement [Symbol.asyncIterator] but ultimately found it might be more problematic for users than beneficial. It comes down to the fact there are really 4 basic ways (and oodles of others) people will want to deal with observables in a for-await loop, and the fact that the most intuitive one of those ways has a lot of overhead (mental and technical) that people will have a hard time understanding. Ultimately, the team decided not to go ahead with the idea, and instead I published a library to convert observables to AsyncIterables.

Consider the complexity added even in this simple case:

textInput.addEventListener('input', async (e) => {
  // this is hit IMMEDIATELY every time the input is changed
  const data = await getData(textInput.value);
  render(data);
});

// vs

for await (const e of textInput.on('input')) {
  // This is hit IMMEDIATELY the FIRST time the input is changed
  // however, if the input is changed while `getData` is doing its thing,
  // then it will not be hit again until `getData` finishes doing its thing.
  // BUT, if the button is clicked two or three times while `getData` is doing 
  // its thing, THEN you have to wait for each of the previous events to be
  // processed. 
  // ADDITIONALLY, the state of `textInput.value`, and the rest of the app,
  //  will have changed by the time this is  hit in those cases, so you may
  // end up sending something you're not expecting.
  const data = await getData(textInput.value);
  render(data);
}

In short, while I think this improves the superficial ergonomics of EventTarget, ultimately I think it makes the code and the type itself harder to reason about because the underlying type and its interactions are more complex.

A pushed based type, like an observable, in likely a better choice for this use-case IMO: #544

However, I do think that there's some merit to making other things, like ReadableStream and the like, into AsyncIterables, as the complexity matches pretty much 1:1.

@benlesh
Copy link

benlesh commented Jan 5, 2022

I briefly floated https://glitch.com/edit/#!/angry-wound. It's been many years though.

@domenic: I honestly like the forEach(cb, { signal }) (no surprise, I'm sure) and I think it's worth exploring.

#544 was unfortunately proposed at a time when Edge and Chrome were two different codebases. Chrome was interested, and Google even had a contractor lined up to do the implementation, but we couldn't get "two implementers" interested, and now the director that had approved the contractor has moved on from Google. :) If only the Edge/Chromium thing had happened a year or two earlier. haha.

@tbondwilkinson
Copy link

tbondwilkinson commented Aug 15, 2022

That's an interesting example, I guess the counter-example is:

for await (const e of textInput.on('input')) {
  getData(textInput.value).then((data) => {
    render(data);
  });
}

To prevent the blocking behavior you were mentioning there, but regardless the main crux here is that returning an async iterator may guide people to using structures like for await... of that isn't what they probably intend, and if they want to use an async iterator differently they'll need to code around that in ways that aren't incredibly common.

Like say you wanted debouncing or to collect a series of touch events together, that's more intuitive with an Observable. So before we commit to an async iterator for on() we should probably figure out what's happening with the Observable proposal.

once() seems less controversial though?

And if I've opened the door to the bikeshed, I've often used "when" for things Promise factories that are not Promises themselves, because it looks nice next to "then". (when() this event fires, then() take this action).

@bakkot
Copy link

bakkot commented Aug 31, 2023

Related: proposal to add Observables and make EventTarget observable.

The once case would be await target.on('event').first(). The iterator case would require a new helper, and a choice of queuing strategy, but you can imagine something like for await (item of target.on('event').iterate('unbuffered')) ....

@annevk
Copy link
Member

annevk commented Aug 31, 2023

Well, "first" there is really "onceAsync" or some such. Not complete parity. But it's still early days and maybe that will change.

@bakkot
Copy link

bakkot commented Aug 31, 2023

What's the actual difference? The thing the OP proposes is

`EventTarget.once() returns a Promise that is resolved the next time the event is dispatched.

which sounds exactly like first from the proposal:

Observable#first() returns a Promise that resolves when the first event is fired on an EventTarget

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition/proposal New features or enhancements topic: events
Development

No branches or pull requests

9 participants