Introduce `withEvent` and require Interactivity API actions that use the `event` object to use it

[felixarntz]

This issue is the result of https://github.com/WordPress/gutenberg/discussions/64729. It is the first issue of a few sequential ones, with the goal to automatically run Interactivity API store actions asynchronously (i.e. after yielding to the main thread) whenever possible.

What problem does this address?

Currently, the decision whether to run an action asynchronously has to be made by the person using the action in a directive (by using e.g. data-wp-on-async instead of data-wp-on). This is cumbersome, as that decision can be made purely based on the action implementation itself, independently of how it is used in a directive. The decision for whether an action should be run after yielding to the main thread should be handled by the action itself.

Additionally, the concept of yielding to the main thread is still relatively new, and many developers are not too familiar with it. Leaving the decision up to developers will therefore likely not yield (pun intended) to much adoption. On the other hand, knowing whether the action uses the event object is a very simple thing that every JS developer should understand. And because yielding to the main thread is universally a good thing, for better performance it makes sense to automatically do that whenever possible (which is the case unless synchronous methods from the event objects are used in the action).

What is your proposed solution?

A withEvent helper should be introduced that should then be adopted by every Interactivity API store actions that use the event object. This will eventually allow to yield to the main thread automatically for every single action, unless it uses withEvent.

Since the latter is technically a breaking change, we'll need to first deprecate usage of event without using withEvent. We can do so by proxying the event object if the action does not yet use withEvent, and triggering deprecation warnings when one of the synchronous methods is called.

Code example for how withEvent could be used:

import { store, withEvent } from '@wordpress/interactivity';

const { state, actions } = store( 'test', {
    actions: {
        toggleSomething() {
            // Logic to contextually toggle an element.
        } ),
        checkFormSubmission: withEvent( (event) => {
            event.preventDefault();

            // Other logic.
        } ),
    },
    // ...
} );

See https://github.com/WordPress/gutenberg/discussions/64729#discussioncomment-10462949 for the original idea.

For reference, this would trigger a deprecation warning going forward, since withEvent is not used even though the action uses event:

import { store } from '@wordpress/interactivity';

const { state, actions } = store( 'test', {
    actions: {
        checkFormSubmission: (event) => { // Not allowed, unless the action is wrapped with `withEvent`.
            event.preventDefault();

            // Other logic.
        },
    },
} );

Overall plan

1. Introduce withEvent and trigger deprecations when using event without it (via proxied event object). This is essentially to prepare the ecosystem for the async first change.
2. Have all actions run async, unless they use withEvent. We can consider still passing the proxied event object to other functions, but now the synchronous calls would simply fail with an error, so passing it would be mostly for better DX (more clear error messaging where code still does it wrong).
   • At the same time, we also start treating data-wp-on-async just the same as data-wp-on. In other words, there's no longer a reason to use data-wp-on-async.
   • The same applies to the other data-wp-on* variants for window and document.
3. Trigger deprecations warnings when using data-wp-on-async and its window and document equivalents.
   • The documentation on data-wp-on-async should be removed (or at least clearly marked as outdated/deprecated).
4. (Optional:) Remove data-wp-on-async (and the other async directives) entirely.
   • Marking this as optional, since would not be a big maintenance burden to maintain the deprecated state, and we should probably only remove them if/once we're convinced that usage has pretty much phased out completely.

This issue is only about the 1. step from the list above.

[felixarntz]

cc @luisherranz @gziolo @cbravobernal @DAreRodz @westonruter

As touched on in the discussion, can we add this to #63232?

[westonruter]

2. Have all actions run async, unless they use withEvent.

I don't think this is quite right. Even if an action uses the event object this doesn't necessitate that it be synchronous. For example, the following actions use the event object but they still use data-wp-on-async (see examples below). What matters is not whether the event object is used but rather whether a synchronous API on the event object is used (e.g. event.preventDefault()), right? So instead of withEvent should it rather be something like withBlockingCallback?

Search block

https://github.com/WordPress/gutenberg/blob/537fb1800ce2784388ec3d130a37bd26f6f2f62f/packages/block-library/src/search/index.php#L194-L195

https://github.com/WordPress/gutenberg/blob/537fb1800ce2784388ec3d130a37bd26f6f2f62f/packages/block-library/src/search/view.js#L47-L68

Navigation block

https://github.com/WordPress/gutenberg/blob/537fb1800ce2784388ec3d130a37bd26f6f2f62f/packages/block-library/src/navigation/index.php#L504

https://github.com/WordPress/gutenberg/blob/537fb1800ce2784388ec3d130a37bd26f6f2f62f/packages/block-library/src/navigation/view.js#L141-L160

[westonruter]

2. We can consider still passing the proxied event object to other functions, but now the synchronous calls would simply fail with an error

Actually, no error is emitted when event.preventDefault() is called. See: https://codepen.io/westonruter/pen/QWXYJQe?editors=1011

So passing the proxied event should still be done so this warning can be emitted to the console.

[felixarntz]

2. Have all actions run async, unless they use withEvent.

I don't think this is quite right. Even if an action uses the event object this doesn't necessitate that it be synchronous. For example, the following actions use the event object but they still use data-wp-on-async (see examples below). What matters is not whether the event object is used but rather whether a synchronous API on the event object is used (e.g. event.preventDefault()), right? So instead of withEvent should it rather be something like withBlockingCallback?

That's a good point. So it's not about whether the event object is used but about whether one of its synchronous methods is used. That makes sense to me and should technically be equally feasible: We would pass event to every action, but if not synchronous we would use the proxied instance that prevents/warns about use of the synchronous methods.

I think the main new question this raises is what a suitable more accurate name than withEvent would be? withBlockingCallback sounds a bit too abstract to me. Maybe withSynchronousEvent? Or shortened withSyncEvent?

Paging @luisherranz @gziolo @cbravobernal to get your thoughts on the name.

[westonruter]

I had a good chat with Felix about this today. Let's say that there is a withSyncEvent (name pending) which should be exclusively used for actions/callbacks which access synchronous methods (e.g. event.preventDefault()). Actions which merely access properties of the event object (e.g. event.keyCode) should not use withSyncEvent since they need not be synchronous. We can proxy the event object so that if event.preventDefault() is called and yet withSyncEvent() was not utilized, an error is thrown.

A risk with withSyncEvent() is that developers start using it all the time even when they shouldn't. How do we deal with this? We can't simply check whether a synchronous API was called and throw an error if it wasn't, since an action which calls event.preventDefault() may only do so conditionally, as seen in the Navigation block. Can we account for this with a custom ESLint rule? With static analysis we can check for withSyncEvent() usages that contain an action that doesn't reference a synchronous API. If so, then ESLint can give a warning. Granted, this wouldn't be able to detect all scenarios, like if a store is defining an action using a function from another module. But it should account for the majority of cases. This ESLint rule could also do the reverse: when using a synchronous API, proactively warn that withSyncEvent() needs to be used so that they don't discover it later at runtime.

Finally, it would also seem useful to pass a proxied event object even into actions that are defined with withSyncEvent() as well since in the case of generators we could keep track of whether the action yielded, and if so, throw an error after that point if a synchronous API is called.

[felixarntz]

@westonruter Thanks for the summary, that is in line with how I recall our conversation. A few minor notes:

We can proxy the event object so that if event.preventDefault() is called and yet withSyncEvent() was not utilized, an error is thrown.

Worth a reminder that this would only show deprecation warnings initially (i.e. this issue), and it would still call event.preventDefault(). It would only start actually failing in a subsequent WordPress release.

A risk with withSyncEvent() is that developers start using it all the time even when they shouldn't.

I think the approach of only requiring withSyncEvent when a synchronous method of event is needed makes sense. This means every action callback would still retain access to the event object to access other properties. While that is an extra cognitive distinction over "use this if you access event", it is certainly the more technically correct solution, and I think with the warnings in place (plus potentially ESLint rules), developers will immediately find out when they're not using withSyncEvent but should have. For the reverse problem, I assume it won't become a major issue in reality, so having ESLint rules would be a nice-to-have.

[gziolo]

2. We can consider still passing the proxied event object to other functions, but now the synchronous calls would simply fail with an error

Actually, no error is emitted when event.preventDefault() is called. See: https://codepen.io/westonruter/pen/QWXYJQe?editors=1011

So passing the proxied event should still be done so this warning can be emitted to the console.

The challenge is that event object has more properties that change after the code becomes async. You can compare using the following snippet:

document.querySelector( 'input.async' ).addEventListener( 'click', ( event ) => {
  const target = event.currentTarget;
  setTimeout( () => {
    event.preventDefault();
    console.info( 'Look ma, no error!' );
    console.info( 'sync', target );
    console.info( 'async', event.currentTarget );
  } );
} );

I had a good chat with Felix about this today. Let's say that there is a withSyncEvent (name pending) which should be exclusively used for actions/callbacks which access synchronous methods (e.g. event.preventDefault()).

I'm happy with the more specific name, and withSyncEvent resonates well with me. From my perspective, the important aspect of all that is that without this helper the callback/action is async by design. In the case of using withSyncEvent it always starts as sync when using regular function, but the trick there would be to ensure we can help detect these scenarios when the execution moves to async through using setTimeout like in the example shared or when using the generator function that is easier to control, as on the runtime level we exactly now when it yielded and we can put the proxied event in the proper mode.

[felixarntz]

@gziolo Great points. This means we should keep an eye out for other event properties where the behavior changes with async access. That said, we don't have to necessarily cover everything right away - since not doing so won't do any harm, it's all about providing more helpful error/warning notices to the developer. So I think this is something that we could always iterate on as we see fit or get feedback on.

Related idea (not a blocker for implementing this): Would it make sense for something like event.currentTarget to get the value during synchronous execution and then in the proxied event object still return that if event.currentTarget is accessed asynchronously? In that specific case, I'm essentially wondering why it no longer works in the default event implementation. What's the harm of still being able to know the current target upon asynchronous execution? 🤔

[gziolo]

The event object has many interfaces. We would have to investigate it further, but it might be tricky to properly handle it on our end. To give a quick example eventPhase:

document.querySelector( 'input.async' ).addEventListener( 'click', ( event ) => {
  const sync = event.eventPhase;
  setTimeout( () => {
    event.preventDefault();
    console.info( 'Look ma, no error!' );
    console.info( 'sync', sync );
    console.info( 'async', event.eventPhase );
  } );
} );

This is a good example where it's expected that the event changes over time. There will be many similar things, for example when you use mouse events, when it becomes async, the cursor (clientX, clientY) might be in a different place, every time the generator functions yields.

At the same time, the potential proxy object has enough to detect whether the sync execution has finished and show warnings for devs. Maybe it's enough to print some note that they are accessing the event in the async mode and leave a link to the docs with more details.

[felixarntz]

Thanks for providing additional context. We should probably discard my previous idea then and not try to be smart about certain event properties. Warning about async access when that doesn't work is probably all we should do in the proxied event.