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

Considering event.waitUntil(promise) for async listeners #9540

Closed
WebReflection opened this issue Jul 20, 2023 · 19 comments
Closed

Considering event.waitUntil(promise) for async listeners #9540

WebReflection opened this issue Jul 20, 2023 · 19 comments

Comments

@WebReflection
Copy link
Contributor

WebReflection commented Jul 20, 2023

The ExtendibleEvent kind of event is exposed only in a ServiceWorker but some of its features could be extremely handy or desirable for any sort of async listener.

Issue

Desired also for other purposes, there are cases where any kind of validation or action to perform through an event (i.e. preventDefault() or stopPropagation() or others) is not possible even if the listener has been set explicitly asynchronously.

anchor.addEventListener('click', async event => {
  // this is not possible at all right now
  if (await asyncCondition()) {
    event.preventDefault();
    alert('some action needed before going there');
  }
});

As silly as this use-case example looks like, there are more convoluted scenarios where having the ability to still operate through the event in an asynchronous way would be desirable:

  • a listener lives in a Worker and it's invoked from the main thread, inevitably asynchronously
  • some expensive computation for an action might be delegated to either a server or a Worker
  • some API might be usable only after asking for it (Media APIs, GeoLocation, IndexedDB, ... etcetera)

Proposal

Allow somehow a way to hold on, without blocking, an event so that any async logic, provided by the platform or the service itself, can still use all event features without having to deal with expiration due asynchronous operations.

I understand this might not be an easy task but if the element where the event generated is async, the bubbling queue could be of async kind instead of blocking until its top most stopPropagation().

I know I am simplifying the complexity involved, but I've thought it's worth trying as currently my way to provide some limited feature, when desired, is to pollute the third argument with some foreign field that is read and orchestrated at the main thread level, so that if I write this in a Worker:

foreign.dcument.element.addEventListener(
  'click',
  event => { /* my Worker handler for this event */ },
  {invoke: ['preventDefault', 'stopImmediatePropagation']}
);

The main thread would be orchestrated to intercept that invoke field of the third option and directly call those methods through the synchronous event generated for that click, before invoking in the Worker the attached handler.

This is already the case in coincident as there's no other way to then handle the event and forward to the Worker its details.

Thanks in advance for at least considering something similar to this proposal so that more asynchronous scenarios within events can be unlocked.

P.S. the coincident module is already used in polyscript project, which is the new core of PyScript and it allows (theoretically) any WASM runtime/interpreter to execute instead of JS and some of them can only run code asynchronously, making DOM events operations extremely limited in features + all runtimes/interpreters have same limitation if executed within a Worker.

@annevk
Copy link
Member

annevk commented Jul 25, 2023

I've read through the Issue section twice and I'm still not sure what problem is being solved.

Is the problem that you need more time to decide whether to invoke preventDefault()? As that would require changing every call site that dispatches events to account for listeners being delayed. That seems unlikely to happen.

@WebReflection
Copy link
Contributor Author

WebReflection commented Jul 26, 2023

Is the problem that you need more time to decide whether to invoke preventDefault()?

the problem is the event expiring synchronously so that these kind of operations are not possible from a worker, or after reading IndexedDB, and so on ... in short: yes

that would require changing every call site that dispatches events to account for listeners being delayed

that's why I wrote it's complex to solve, but not impossible?

That seems unlikely to happen

that means my workaround will stay around for long time then, as there's no other way to implement a preventDefault() asynchronously that migh, eventually re-dispatch the event avoiding that one to prevent default ... this is all user-land boilerplate that could be "simplified" in specs, so if that explicit proposal is unlikely to happen, I'd love to discuss a way to solve this issue as it'll be more common over time, not less.

@keithamus
Copy link
Contributor

This would have extreme negative consequences. The default operation (such as navigating to a link) would have to wait until upstream promises have resolved which would cause visible lag to the user. A way to remediate that lag would be to only allow a window of time but then that adds a whole heap of complication to the use case.

If the userland project desires this functionality, the more straightforward solution is for that userland project to inherit the complexity: always call preventDefault() synchronously, overload the function, add your async logic, then emulate the default behaviours if the async logic didn't call your overload.

@WebReflection
Copy link
Contributor Author

WebReflection commented Jul 26, 2023

This would have extreme negative consequences.

From MDN

Screenshot from 2023-07-26 21-47-33

it's always funny to see rationales for implementation details and features from vendors bashed as "extremely negative consequences" when it comes to the platform users ... I mean, the same reason it's needed for Service Worker can be easily extended to IndexedDB or any other async API (as these days they are all async) that might desire to do something else with that event.

The default operation (such as navigating to a link) would have to wait until upstream promises have resolved which would cause visible lag to the user.

Only if needed/desired ... which is the current status quo of the web: form fields that can't operate until a link has been clicked or links with prevented default that do a whole async dance before actually returning, showing, or doing, anything meaningful (including buttons in forms).

A way to remediate that lag would be to only allow a window of time but then that adds a whole heap of complication to the use case.

The click has been historically a 250ms up to 300ms operation before reacting, due dblclick possible window ... I find that time annoying yet that's how the whole web has been built on ... make it a threshold for this functionality and nobody would notice any difference?

True that this applies to click and not all other events (i.e. input) but again, this was to open a discussion, not slam doors (hopefully).

the more straightforward solution is for that userland project to...

I've already implemented that, as mentioned, and it works ... it also requires me to monkey patch addEventListener because it makes no sense to preventDefault or stopPropagation by default to every event so ... once again, this request would like to be open to alternative solutions/discussions, otherwise I go as I do now, and point at this issue whenever somebody complains I've polluted addEventListener ... I don't have any other option.

@WebReflection
Copy link
Contributor Author

WebReflection commented Jun 12, 2024

not that I expect anyone suddenly realizing how broken are async listeners in the EventTarget interface/class, but yesterday I had another issue which wasn't even about preventing the default, it was about losing completely original properties attached to that event such as currentTarget:

element.addEventListener('click', async (event) => {
  if (await somethingElse(event))
    doSomethingElse(event.currentTarget);
});

The workaround was to actually trap that property ASAP and then forward it, losing the whole event "namespace" in doing so (as in: I couldn't just pass the event with its props around as none of these are reliable?):

element.addEventListener('click', async (event) => {
  const { currentTarget } = event;
  if (await somethingElse(event))
    doSomethingElse(currentTarget);
});

This is a simplified example but basically my surprise is that:

  • events that are being dispatched can't be re-dispatched ... I assume they are immutable in their lifetime ... NO
  • events within an async listener lose randomly properties expected to be there, heck now I wonder if they leak properties instead if the tick is executed while another event is firing (hopefully not or not possible at all)

So ... the quest for a way to deal with events in asynchronous listeners remain ... I might be OK with having no ability to stop a bubbling chain or immediate propagation asynchronously, but events losing references that are supposed to be immutable was a whole new world of footgun to me.

Thanks for keeping this open though, it let me hope one day we'll be there.

@WebReflection
Copy link
Contributor Author

WebReflection commented Jun 12, 2024

As code speaks thousand words:

<!doctype html>
<html lang="en">
<head>
  <meta charset="UTF-8">
  <meta name="viewport" content="width=device-width,initial-scale=1.0">
  <script type="module">
    const pause = () => new Promise($ => setTimeout($, 100));
    click_me.addEventListener('click', async event => {
      const { currentTarget } = event;
      // of course!
      console.assert(currentTarget === event.currentTarget, 'sync');
      await pause();
      console.log({ sync: currentTarget, async: event.currentTarget });
      // of course ... NOT?! 🙃
      console.assert(currentTarget === event.currentTarget, 'async');
    });
  </script>
</head>
<body>
  <button id="click_me">Click Me</button>
</body>
</html>

From MDN though:

The currentTarget read-only property of the Event interface identifies the element to which the event handler has been attached.

So what are the "read only" properties that will fail at providing the expected value when a listener is asynchronous and the event has been used after awaiting anything else? (note the target is still there though ... what is going on here?)

I feel like this is a fundamental lack of documentation and reasoning about for the forever growing appearance of asynchronous APIs all over the Web too so if anyone would be so kind to at least list these properties or methods and explain why (this one in particular is super odd to reason about) the rest of the world might appreciate that more than you think, thank you!

@WebReflection
Copy link
Contributor Author

WebReflection commented Jun 12, 2024

To whom it might concern, I've digged further around this currentTarget absurdity and it looks like it's the only accessor (read-only) that loses its value (which is an extremely important one as that's not just a target, it's the listener owner) that makes no sense on await ... the eventPhase as read-only giving different value is instead expected and easy to reason about.

I haven't tried all non-function values but anyone could do that from here:

<!DOCTYPE html>
<html lang="en">
<head>
  <meta charset="UTF-8">
  <meta name="viewport" content="width=device-width,initial-scale=1.0">
  <script type="module">
    const { entries, getOwnPropertyDescriptors, getPrototypeOf, is } = Object;

    const getAllPropertyDescriptors = ref => {
      let descriptors = {};
      while (ref) {
        descriptors = {
          ...descriptors,
          ...getOwnPropertyDescriptors(ref),
        };
        ref = getPrototypeOf(ref);
      }
      return descriptors;
    };

    const pause = () => new Promise($ => setTimeout($, 100));
    const keyVal = event => entries(getAllPropertyDescriptors(event));

    click_me.addEventListener('click', async event => {
      const syncProps = {};
      const entries = keyVal(event);
      for (const [key, { get }] of entries)
        if (get) syncProps[key] = get.call(event);

      await pause();

      for (const [key, { get }] of entries) {
        if (get) {
          const sv = syncProps[key];
          const av = get.call(event);
          if (!is(sv, av))
            console.log(key, { sync: sv, async: av });
        }
      }
    });

    click_me.click();
  </script>
</head>
<body>
  <button id="click_me">Click Me</button>
</body>
</html>

At this point I wonder if I should file a bug everywhere around that ... because all major browsers fail the same ... so maybe this is a WHATWG thing to tackle instead or document properly and propagate to MDN ... still this makes little sense to me, thanks for expanding on that, eventually.


edit

"Why is the listener owner so important?"

Well, to whoever got the memo that an object can be a listener too, having the ability not just after once but after conditions to say currentTarget.removeEventListener(event.type, this) is an awesome feature that is fully lost when handleEvent is an async function and somebody put an await on it before trapping currentTarget.

Plus there are other thousand libraries trusting currentTarget should never be null or undefined out there so this is breaking everyone expectations with no apparent reason to do so.

@annevk
Copy link
Member

annevk commented Jun 12, 2024

currentTarget's value is only meaningful within the context of a callback as an event can have many targets over its lifetime. That's inherent in its design and not going to change.

@WebReflection
Copy link
Contributor Author

WebReflection commented Jun 12, 2024

@annevk

currentTarget's value is only meaningful ...

that's not too explicit in specs or MDN ... it should be updated

within the context of a callback as an event can have many targets

the target property doesn't change ... the currentTarget that actually dispatched the event never changed neither and I am checking its value without having any other currentTarget possible in the bubbling chain ... I understand if that re-dispatch in an outer element with a listener that should change, I don't understand why my demo/example code doesn't work though, and even a non bubbling listener would likely behave the same (to be tested) ... why is that lost in the process of a single listener that can't have any other possible currentTarget in the process?

That's inherent in its design and not going to change.

if we had event.waitUntil changes like this would get a chance to not-exist until the event is freed from its own queue ... so I think that would be desired in the near to far future.

@WebReflection
Copy link
Contributor Author

WebReflection commented Jun 12, 2024

@annevk please explain me this:

const listener = async event => {
  console.log(event.currentTarget);
  await new Promise($ => setTimeout($, 100));
  console.log(event.currentTarget);
}

const et = new EventTarget;
et.addEventListener('test', listener);
et.dispatchEvent(new Event('test'));

You will read EventTarget (et) then true then EventTarget (et) again.

Now ... enter the DOM:

document.addEventListener('test', listener);
document.dispatchEvent(new Event('test'));

You will read #document, then true, then null ...

@annevk
Copy link
Member

annevk commented Jun 12, 2024

There is no re-dispatch when bubbling. That's not how it works. Anyway, DOM is pretty clear on this. Step 7 of dispatch says:

Set event’s currentTarget attribute to null.

(It should really modify an internal field, but that's largely an editorial problem.)

As for your example, I get EventTarget and then null for the first example as well in Firefox and Safari. Seems like Chrome has a bug there they might want to fix.

@WebReflection
Copy link
Contributor Author

WebReflection commented Jun 12, 2024

@annevk it's not Chromium there then ... it's V8 as I get the same in NodeJS. This is still something to (imho) describe as possible caveats in MDN ... it's literally the only property behaving weirdly when you own both the element, the listener, and you are sure the Event is not bubbling so that currentTarget can't ever be possibly something else.

If this is meant, it was meant when async in DOM and JS didn't exist ... I wish we'll acknowledge sooner than later the landscape has chnaged and all APIs are async these days.

edit

by re-dispatch I meant the bubbling phase where the event passes through other listeners in the parent element ... I know one can't re-dispatch a dispatching event

@annevk
Copy link
Member

annevk commented Jun 12, 2024

NodeJS prolly just has the same bug. It's not like event dispatch is handled by V8. But also, eventPhase is reset, composedPath is reset, and various other booleans are reset.

@WebReflection
Copy link
Contributor Author

those are fine, the fact currentTarget is prematurely lost or lost out of the blue when nothing could interfere with it is less fine to me ... but then again, we have inconsistencies, surprises, and nothing is documented out there to warn people just adding async event listeners. Happy to help there, but I need to understand if there's really nothing that could be improved in here.

@WebReflection
Copy link
Contributor Author

WebReflection commented Jun 12, 2024

to whom it might concern, using this first test:

  • NodeJS fails (inconsistent with the DOM) ❌
  • Bun is consistent with the DOM ✔️
  • Deno is consistent with the DOM ✔️
  • Chrome/ium fails (inconsistent with the DOM), and so does Edge and others Chromium derivates ❌
  • WebKit is consistent with the DOM ✔️
  • Firefox is consistent with the DOM ✔️

Issue

The currentTarget apparently "should be" lost after any asynchronous operation awaited in any asynchronous listener.

annevk added a commit to web-platform-tests/wpt that referenced this issue Jun 12, 2024
This is apparently somewhat badly implemented as discovered here: whatwg/html#9540 (comment)
@annevk
Copy link
Member

annevk commented Jun 12, 2024

It really is unrelated to "asynchronous listeners". Those are not even a thing as far as DOM is concerned. It's just reset when dispatch is considered to be done. Anyway, I'm inclined to close this issue as it has gone completely off the rails now.

@WebReflection
Copy link
Contributor Author

It really is unrelated to "asynchronous listeners".

It's not ... if event.waitUntil would be a reality, that currentTarget without bubbling up or down would still be available within that promise scope, with no surprises, the event is suspended until it's done within the current listener, and not modified before that ends.

annevk added a commit to web-platform-tests/wpt that referenced this issue Jun 12, 2024
This is apparently somewhat badly implemented as discovered here: whatwg/html#9540 (comment)
@annevk annevk closed this as not planned Won't fix, can't repro, duplicate, stale Jun 12, 2024
@WebReflection
Copy link
Contributor Author

WebReflection commented Jun 12, 2024

Sad to see this closed. The async listener is a reality for many modern frameworks and libraries ... it's awkward to have an event awaitable only in some circumstance (Service Worker) and even more awkward to hear that can't be the case for everything else. Discrepancies are usually not good and if there was a single use case for the Service Worker to enable that API, I am pretty sure the plethora of libraries out there would have a say in here, asking for a way to await events, at least those with no bubbling use cases possible.

Hopefully "we'll meet again" in 2 to 5 years; until then, thanks for the exchange.

@annevk
Copy link
Member

annevk commented Jun 12, 2024

Service workers was designed this way from the start. And arguably shouldn't have used events because it's a bit of a hack. Asking us to redesign everything in the platform that dispatches events is just not realistic, especially without very strong evidence that this is being worked around by literally everyone at the library level. (At which point we would have heard this from many people.)

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Jun 18, 2024
…h, a=testonly

Automatic update from web-platform-tests
Test constructible EventTarget's dispatch

This is apparently somewhat badly implemented as discovered here: whatwg/html#9540 (comment)
--

wpt-commits: 33bd9cc8426a14d378cd6b6e4d314f18c7af7773
wpt-pr: 46717
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Jun 19, 2024
…h, a=testonly

Automatic update from web-platform-tests
Test constructible EventTarget's dispatch

This is apparently somewhat badly implemented as discovered here: whatwg/html#9540 (comment)
--

wpt-commits: 33bd9cc8426a14d378cd6b6e4d314f18c7af7773
wpt-pr: 46717

UltraBlame original commit: 990ddac61c6886f63ce40341a103356e48b16f2e
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Jun 19, 2024
…h, a=testonly

Automatic update from web-platform-tests
Test constructible EventTarget's dispatch

This is apparently somewhat badly implemented as discovered here: whatwg/html#9540 (comment)
--

wpt-commits: 33bd9cc8426a14d378cd6b6e4d314f18c7af7773
wpt-pr: 46717

UltraBlame original commit: 990ddac61c6886f63ce40341a103356e48b16f2e
i3roly pushed a commit to i3roly/firefox-dynasty that referenced this issue Jun 21, 2024
…h, a=testonly

Automatic update from web-platform-tests
Test constructible EventTarget's dispatch

This is apparently somewhat badly implemented as discovered here: whatwg/html#9540 (comment)
--

wpt-commits: 33bd9cc8426a14d378cd6b6e4d314f18c7af7773
wpt-pr: 46717
jamienicol pushed a commit to jamienicol/gecko that referenced this issue Jun 24, 2024
…h, a=testonly

Automatic update from web-platform-tests
Test constructible EventTarget's dispatch

This is apparently somewhat badly implemented as discovered here: whatwg/html#9540 (comment)
--

wpt-commits: 33bd9cc8426a14d378cd6b6e4d314f18c7af7773
wpt-pr: 46717
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants