Optimizing yielding prior to actions out of the box

[felixarntz]

Over the past few months, the Interactivity API was enhanced with several data-wp-on-async* directives (see #61885), which allow invoking actions asynchronously, i.e. yielding to the main thread prior to execution the action, which helps performance by avoiding contention.

First of all, it's worth clarifying that asynchronous actions and the data-wp-on-async directives are separate concepts, even though they are somewhat related:

• Asynchronous actions are generator functions, which for instance allows breaking up their logic into distinct tasks by yielding to the main thread.
• Any actions invoked by data-wp-on-async directives will additionally yield to the main thread before even starting to run the action. The critical consideration here is that this is only reasonable if the action does not make use of certain synchronous event methods, such as event.preventDefault() or event.stopPropagation().
• In other words: You can invoke both asynchronous and non-asynchronous actions with either data-wp-on or data-wp-on-async, as long as the action does not use one of those synchronous event methods. If it does, you must use data-wp-on.

The purpose of this discussion is focused mostly on the yielding prior to executing an action, i.e. the functionality enabled by the data-wp-on-async directives.

Current problems

While the data-wp-on-async approach is a decent start in preparing the Interactivity API to be more responsible about competing interactions, it currently puts the onus unnecessarily on the developers consuming the store, i.e. the ones that write the HTML containing the directives. It has a few drawbacks:

• The decision on whether an action can be called with data-wp-on-async or not purely depends on whether the action does not use one of the synchronous event methods. So it is not a great DX to have to know about whether this is the case for each action as the developer writing the HTML directive.
  • See below code examples to better illustrate this.
• Generally, if an action does not use one of the synchronous event methods, it's always better for performance to call it with data-wp-on-async so that it yields to the main thread prior to executing. So this makes it questionable why the person writing the HTML directive even needs to consider this at all.
• Last but not least, the difference between data-wp-on and data-wp-on-async is complicated to understand for people not terribly familiar with JavaScript. I believe one of the value propositions of the Interactivity API is that in principle it can allow adding interactivity even without writing any JavaScript (if consuming actions from a reusable store from somewhere else, e.g. something core provides, or a 3P package). Having these two directives is IMO unreasonably complex from that perspective, which most likely will lead to a large portion of people using data-wp-on in the long run because they don't understand when to use data-wp-on-async.
  • This is made worse because the majority of actions does not need to use the synchronous event methods. But because data-wp-on will probably be used by more folks (if in doubt), it will lead to a lot of unnecessary synchronous action calls.

Proposed solution

The overall idea is the following: Whether an action can be called after yielding to the main thread or whether it must be called immediately is purely dependent on the action implementation, so it should be on the developer implementing the action in the store rather than the developer using it.

One important consideration: It is not reasonably possible for the Interactivity API to automatically detect whether an action calls a synchronous event methods or not. Therefore, the developers has to explicitly clarify that themselves.

This leads to the following concrete proposal:

• Introduce a helper function, or a set of helper functions that allow annotating the action implementation with whether it can be run asynchronously (after yielding to the main thread) or not.
• If the action is explicitly annotated in that way (to either allow or disallow asynchronous calling), the Interactivity API should ignore whether the directive is data-wp-on or data-wp-on-async and instead automatically make the right decision:
  • If the action allows asynchronous calling, always treat it as if data-wp-on-async was used.
  • If the action requires synchronous calling, always treat it as if data-wp-on was used.
  • If the action is not annotated, handle it like today. This is purely for backward compatibility.
• Short-term to long-term vision after launch:
  1. Promote the use of these functions going forward for every action implementation to establish this as the new best practice.
  2. Further down the line, sing the new functions for implementing actions should become more or less required, i.e. any action not annotated should trigger a deprecation warning.
  3. Finally, the data-wp-on-async directives should be deprecated too in favor of all actions being annotated and thus allowing the Interactivity API to make the right decision automatically.

Proof of concept example

Just a quick example to illustrate what this could look like (let's not get hung up on function names/signatures for now):

Interactivity API code example:

function createSyncAction( action ) {
    action.sync = true;
    return action;
}

function createAsyncAction( action ) {
    action.sync = false;
    return action;
}

// Then update directive handlers for all data-wp-on* directives to call `yieldToMain()`
// prior to the action if `action.sync === false`, or to not call it if `action.sync === true`.

Store example:

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

const { state, actions } = store( 'test', {
    actions: {
        toggleSomething: createAsyncAction( () => {
            // Logic to contextually toggle an element.
        } ),

        checkFormSubmission: createSyncAction( ( event ) => {
            if ( ! funcThatChecksValidity() ) {
                event.preventDefault();
            }

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

Directive example:

<!-- The caller doesn't need to worry about it and can always use data-wp-on. -->
<button data-wp-on--click="actions.toggleSomething" >...</button>
<form data-wp-on--submit="actions.checkFormSubmission" >...</form>

I discussed this with @westonruter earlier this week, and we believe such an approach would lead to a much better long-term performance outcome, while simplifying the complexity by directly connecting the decision how to call the action with its implementation and decoupling it from directives.

Curious to get feedback on this proposal and shape this further.

[felixarntz]

@luisherranz Related to your comment #61634 (comment).

[felixarntz]

Sounds like a plan!

I opened #64944 and #64946 for now. The first one is what we should focus on for now (and for WP 6.7, if possible).

I'm happy to help where I can, including documentation.

[felixarntz]

Should we close this discussion in favor of using the issues going forward?

[gziolo]

Thank you so much @felixarntz. Yes, fell free to close it as I don’t see more action items here.

[westonruter]

Sorry to be late to the party. I was on vacation when these discussions were happening. I just wanted to draw attention to #64944 (comment) which I left on the issue so as to not re-open this discussion (unless warranted). But I'll quote here for convenience:

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

gutenberg/packages/block-library/src/search/index.php

Lines 194 to 195 in 537fb18

	data-wp-on-async--keydown="actions.handleSearchKeydown"
	data-wp-on-async--focusout="actions.handleSearchFocusout"

gutenberg/packages/block-library/src/search/view.js

Lines 47 to 68 in 537fb18

	handleSearchKeydown( event ) {
	const { ref } = getElement();
	// If Escape close the menu.
	if ( event?.key === 'Escape' ) {
	actions.closeSearchInput();
	ref.querySelector( 'button' ).focus();
	}
	},
	handleSearchFocusout( event ) {
	const { ref } = getElement();
	// If focus is outside search form, and in the document, close menu
	// event.target === The element losing focus
	// event.relatedTarget === The element receiving focus (if any)
	// When focusout is outside the document,
	// `window.document.activeElement` doesn't change.
	if (
	! ref.contains( event.relatedTarget ) &&
	event.target !== window.document.activeElement
	) {
	actions.closeSearchInput();
	}
	},

Navigation block

gutenberg/packages/block-library/src/navigation/index.php

Line 504 in 537fb18

data-wp-on-async--focusout="actions.handleMenuFocusout"

gutenberg/packages/block-library/src/navigation/view.js

Lines 141 to 160 in 537fb18

	handleMenuFocusout( event ) {
	const { modal, type } = getContext();
	// If focus is outside modal, and in the document, close menu
	// event.target === The element losing focus
	// event.relatedTarget === The element receiving focus (if any)
	// When focusout is outside the document,
	// `window.document.activeElement` doesn't change.
	// The event.relatedTarget is null when something outside the navigation menu is clicked. This is only necessary for Safari.
	if (
	event.relatedTarget === null ||
	( ! modal?.contains( event.relatedTarget ) &&
	event.target !== window.document.activeElement &&
	type === 'submenu' )
	) {
	actions.closeMenu( 'click' );
	actions.closeMenu( 'focus' );
	}
	},

Also #64944 (comment):

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.

Probably best to continue the discussion on #64944, but I wanted to cross-post here for good measure!

[felixarntz]

Thanks @westonruter, will reply on the issue.

[cbravobernal]

I commented in the Slack channel, but my thoughts are very similar to Luis's.

• Developers may confuse async await functions with createAsyncAction functions.
• They will still need to determine the action.
• "any action not annotated should trigger a deprecation warning" this is vital. I'm afraid most actions won't be annotated. We could include with independence of this approach. Maybe an eslint rule?

[felixarntz]

Developers may confuse async await functions with createAsyncAction functions.

Definitely, that's fair. I was trying to indicate to not focus too much on the naming yet, I'm sure we could come up with something better in terms of naming and terminology. :)

They will still need to determine the action.

Can you clarify what you mean by this? Not sure I understand.

"any action not annotated should trigger a deprecation warning" this is vital. I'm afraid most actions won't be annotated. We could include with independence of this approach. Maybe an eslint rule?

To clarify, this was a longer-term plan proposal. I think at this point the Interactivity API is still quite fresh and hasn't seen too much developer adoption yet, so I think we're still at a point where we can pivot things to a certain degree. I definitely didn't mean to suggest we deprecate the "not annotated" functions right away, but maybe more like in let's say 6 months, once Gutenberg and Core have been updated and we have documented the new requirement and spread the word etc.

Anyways, based on the conversations in the above thread, there may be a much better solution in the first place. It's probably worth centering the conversation in there to not get lost in too many threads :)

[cbravobernal]

They will still need to determine the action.

Sorry for not being clear. This means that the developer still needs to know the difference between async actions, asynchronous calls, and when to use them. I'd prefer an approach that prevents overwhelming by automating the selection, like the webpack auto-fix mentioned.

Anyways, based on the conversations in the above thread, there may be a much better solution in the first place. It's probably worth centering the conversation in there to not get lost in too many threads :)

Yep! Let's keep the discussion there.

[luisherranz]

Resharing the approach I shared here, as it still might be worth considering:

I had an idea to make wp-on synchronous by default. I don't know WordPress's backward compatibility policy, but it is possible to transition wp-on from synchronous to asynchronous by informing users over a period of time in the JS console, similar to a deprecation.

It would work as follows. Instead of adding wp-on-async, we add wp-on-sync. In wp-on, we proxy or wrap the event passed to the callback so that the functions preventDefault, stopPropagation, etc. display a warning in the console urging users to replace wp-on with wp-on-sync. If we maintain this warning message for several WordPress versions (again, I'm not entirely sure what the policy would be here - three major versions? one year?), we could make the default behavior asynchronous. Then, when someone uses a synchronous method on wp-on, an error message would be displayed telling them to change the wp-on directive to wp-on-sync.

The only downside would be that, until several WordPress versions later, wp-on (and wp-on-sync) would need to be synchronous.

[felixarntz]

I had an idea to make wp-on synchronous by default.

From what I understand, is this a typo and you mean "asynchronous" in this sentence?

This is another potential option, honestly when I first thought about this a few months back, my initial idea was something along those lines, definitely involving proxying the event object to emit warnings like you're saying.

The downside of this approach is that it still decouples whether to call the action async or sync from the actual action implementation, which is why at this point I'm not a fan of this approach anymore. Plus, it's a much more "aggressive" approach, with changing the default. Again, that was my initial thought too, but I think the other ideas discussed above are equally promising in terms of adoption and impact with less BC breakage. That said, if we run into other blockers or problems with the alternatives discussed above, I think this would be worth considering given that at least there's clear messages/warnings to the callers. But I am currently most leaning towards the approach of yield asyncCallback() and automatically adding it with Webpack if not already there, that is discussed above.

[luisherranz]

The downside of this approach is that it still decouples whether to call the action async or sync from the actual action implementation, which is why at this point I'm not a fan of this approach anymore

I agree 🙂