Introduce `wp-on-async` directive as performant alternative over synchronous `wp-on` directive #61885

westonruter · 2024-05-23T00:41:04Z

What?

This adds an alternative async variation to the data-wp-on directives by tagging on an -async to the directive name. So instead of data-wp-on--click it can be data-wp-on-async--click. This is also done for the data-wp-on-window and data-wp-on-document directives with data-wp-on-async-window anddata-wp-on-async-document respectively (which here can also be made passive).

Why?

This ensures that when there are multiple directives for the same event, they do not add up to a long task.

How?

This directive yields to the main thread immediately before invoking the action/callback.

This is implemented for the eligible directives in the Image block.

Testing Instructions

Install and activate this Try Slow Events plugin which adds 10 click event directives to the img in an Image block.
Add an Image to a post and enable the click-to-expand feature.
Navigate to the post on the frontend.
First add ?try-slow-events=sync to the URL and try opening the image in a lightbox. Notice the latency.
Second, try ?try-slow-events=async to the URL and try opening the image in a lightbox. Notice the latency goes away.

Screenshots or screencast

Before (`?try-slow-events=sync`)

Screen.recording.2024-05-22.17.34.29.webm

After (`?try-slow-events=async`)

Screen.recording.2024-05-22.17.34.52.webm

(Aside: The yieldToMain() seems it could be improved yet further since a long task is still sneaking in, even though it is much better.)

github-actions · 2024-05-23T00:41:25Z

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: westonruter <westonruter@git.wordpress.org>
Co-authored-by: cbravobernal <cbravobernal@git.wordpress.org>
Co-authored-by: gziolo <gziolo@git.wordpress.org>
Co-authored-by: luisherranz <luisherranz@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

github-actions · 2024-05-23T00:42:17Z

This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress.

If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged.

If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack.

Thank you! ❤️

View changed files

❔ packages/block-library/src/image/index.php

github-actions · 2024-05-23T01:04:29Z

Flaky tests detected in 9a350d3.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9211662096
📝 Reported issues:

[Flaky Test] Clicking on inserter while on zoom-out should open the patterns tab on the inserter #61806 in /test/e2e/specs/site-editor/zoom-out.spec.js

cbravobernal · 2024-05-23T11:22:11Z

Thanks @westonruter for the PR!

My main concern is that developers won't use the async mode as such as needed.

What could be the implications of making it async by default and switching to wp-on-sync- for the rest of cases?

How can we guide developers for the best approach, apart from updating the docs?

cbravobernal

Code looks good, and tests are working as expected.

It would be nice to add some e2e or check if we can update the ones we have for the document and window directives.

We still need to update the documentation to include these new directives.

Forgot to add changelog

westonruter · 2024-05-23T15:20:34Z

@cbravobernal:

My main concern is that developers won't use the async mode as such as needed.

What could be the implications of making it async by default and switching to wp-on-sync- for the rest of cases?

That would be ideal, if the back-compat breakage is acceptable. Basically, synchronous events are required when passing in an event object and when the handler calls event.preventDefault(), event.stopPropagation(), or event.stopImmediatePropagation(). Two of the actions on the Image block call event.preventDefault() (as listed in #61634). For example, if actions.handleKeydown were to be made async, then no longer would Tab key presses get trapped in the opened lightbox since event.preventDefault() would be called too late.

How can we guide developers for the best approach, apart from updating the docs?

One possibility is that we warn users about switching to use on-async when the associated action lacks any argument. Perhaps when SCRIPT_DEBUG is enabled, when looping over the directives to add to the events Map, it could look up the referenced function and stringify it to then parse out the function's argument list. If it is empty and on is used, it could warn that the user should use on-async instead. (Maybe the function body could also be looked at to see if it contains preventDefault, stopPropagation, or stopImmediatePropagation.)

westonruter · 2024-05-23T16:40:04Z

One possibility is that we warn users about switching to use on-async when the associated action lacks any argument. Perhaps when SCRIPT_DEBUG is enabled, when looping over the directives to add to the events Map, it could look up the referenced function and stringify it to then parse out the function's argument list. If it is empty and on is used, it could warn that the user should use on-async instead. (Maybe the function body could also be looked at to see if it contains preventDefault, stopPropagation, or stopImmediatePropagation.)

Humm. This doesn't seem to work as I expected because the actions/callbacks are getting wrapped in additional functions.

My failed patch

diff --git a/packages/interactivity/src/directives.tsx b/packages/interactivity/src/directives.tsx
index a4a320eda57..7af341da558 100644
--- a/packages/interactivity/src/directives.tsx
+++ b/packages/interactivity/src/directives.tsx
@@ -19,7 +19,7 @@ import {
 	yieldToMain,
 } from './utils';
 import type { DirectiveEntry } from './hooks';
-import { directive, getScope, getEvaluate } from './hooks';
+import { directive, getScope, getEvaluate, getDereference } from './hooks';
 
 // Assigned objects should be ignored during proxification.
 const contextAssignedObjects = new WeakMap();
@@ -314,7 +314,7 @@ export default () => {
 	} );
 
 	// data-wp-on--[event]
-	directive( 'on', ( { directives: { on }, element, evaluate } ) => {
+	directive( 'on', ( { directives: { on }, element, evaluate, dereference } ) => {
 		const events = new Map< string, Set< DirectiveEntry > >();
 		on.filter( ( { suffix } ) => suffix !== 'default' ).forEach(
 			( entry ) => {
@@ -328,6 +328,14 @@ export default () => {
 
 		events.forEach( ( entries, eventType ) => {
 			const existingHandler = element.props[ `on${ eventType }` ];
+
+			if ( typeof SCRIPT_DEBUG === 'boolean' && SCRIPT_DEBUG ) {
+				for ( const entry of entries ) {
+					const handler = dereference( entry ).value;
+					console.info(handler.toString());
+				}
+			}
+
 			element.props[ `on${ eventType }` ] = ( event: Event ) => {
 				entries.forEach( ( entry ) => {
 					if ( existingHandler ) {
diff --git a/packages/interactivity/src/hooks.tsx b/packages/interactivity/src/hooks.tsx
index d8aa43d2c0e..9fc403c7801 100644
--- a/packages/interactivity/src/hooks.tsx
+++ b/packages/interactivity/src/hooks.tsx
@@ -53,6 +53,11 @@ interface DirectiveArgs {
 	 * context.
 	 */
 	evaluate: Evaluate;
+	/**
+	 * Function that dereferences a given path to a value either in the store or the
+	 * context.
+	 */
+	dereference: Dereference;
 }
 
 interface DirectiveCallback {
@@ -71,6 +76,7 @@ interface DirectiveOptions {
 
 interface Scope {
 	evaluate: Evaluate;
+	dereference: Dereference;
 	context: object;
 	ref: RefObject< HTMLElement >;
 	attributes: createElement.JSX.HTMLAttributes;
@@ -80,10 +86,18 @@ interface Evaluate {
 	( entry: DirectiveEntry, ...args: any[] ): any;
 }
 
+interface Dereference {
+	( entry: DirectiveEntry ): any;
+}
+
 interface GetEvaluate {
 	( args: { scope: Scope } ): Evaluate;
 }
 
+interface GetDereference {
+	( args: { scope: Scope } ): Dereference;
+}
+
 type PriorityLevel = string[];
 
 interface GetPriorityLevels {
@@ -307,6 +321,24 @@ export const getEvaluate: GetEvaluate =
 		return hasNegationOperator ? ! result : result;
 	};
 
+// Generate the dereference function.
+export const getDereference: GetDereference =
+	( { scope } ) =>
+	( entry ) => {
+		let { value: path, namespace } = entry;
+		if ( typeof path !== 'string' ) {
+			throw new Error( 'The `value` prop should be a string path' );
+		}
+		// If path starts with !, remove it and save a flag.
+		const hasNegationOperator =
+			path[ 0 ] === '!' && !! ( path = path.slice( 1 ) );
+		setScope( scope );
+		const value = resolve( path, namespace );
+		const result = { value, hasNegationOperator };
+		resetScope();
+		return result;
+	};
+
 // Separate directives by priority. The resulting array contains objects
 // of directives grouped by same priority, and sorted in ascending order.
 const getPriorityLevels: GetPriorityLevels = ( directives ) => {
@@ -338,6 +370,7 @@ const Directives = ( {
 	// element ref, state and props.
 	const scope = useRef< Scope >( {} as Scope ).current;
 	scope.evaluate = useCallback( getEvaluate( { scope } ), [] );
+	scope.dereference = useCallback( getDereference( { scope } ), [] );
 	scope.context = useContext( context );
 	/* eslint-disable react-hooks/rules-of-hooks */
 	scope.ref = previousScope?.ref || useRef( null );
@@ -367,6 +400,7 @@ const Directives = ( {
 		element,
 		context,
 		evaluate: scope.evaluate,
+		dereference: scope.dereference,
 	};
 
 	setScope( scope );

When dereferencing a function, I'm seeing its source as a closure coming from handlers in store.ts:

(...args) => {
  (0,_hooks__WEBPACK_IMPORTED_MODULE_1__.setNamespace)(ns);
  try {
    return result(...args);
  } finally {
    (0,_hooks__WEBPACK_IMPORTED_MODULE_1__.resetNamespace)();
  }
}

cbravobernal · 2024-05-27T11:57:49Z

We only have this week to land this change before beta 1.

Should we merge this, go with the data-wp-on-async directives and work later on the warning @westonruter ?

westonruter · 2024-05-28T04:43:49Z

@cbravobernal That makes sense to me!

gziolo · 2024-05-28T10:36:06Z

Let's remember to document the new API. It should be very similar to:

https://github.com/WordPress/gutenberg/blob/trunk/docs/reference-guides/interactivity-api/api-reference.md#wp-on

It would be also worth explaining the nuances and when to use wp-on-async instead of wp-on. The way I read it is - always use the async version when there is no event consumed.

cbravobernal · 2024-05-28T10:51:31Z

I fixed the changelog conflict.

adamsilverstein · 2024-05-28T13:43:00Z

Let's remember to document the new API. It should be very similar to:

trunk/docs/reference-guides/interactivity-api/api-reference.md#wp-on

It would be also worth explaining the nuances and when to use wp-on-async instead of wp-on. The way I read it is - always use the async version when there is no event consumed.

I added the "Needs Dev Note label here - this feature is worth calling out for the 6.6 core release.

westonruter · 2024-05-31T00:41:59Z

I've opened #62160 to implement data-wp-on-async for the other core blocks.

…hronous `wp-on` directive (WordPress#61885) * Improve typing for data-wp-on directive * Move yieldToMain to utils * fixup! Improve typing for data-wp-on directive * Introduce on-async directives * Use on-async directives in image block * Update changelog * Fix typo in comment --------- Co-authored-by: westonruter <westonruter@git.wordpress.org> Co-authored-by: cbravobernal <cbravobernal@git.wordpress.org> Co-authored-by: gziolo <gziolo@git.wordpress.org> Co-authored-by: luisherranz <luisherranz@git.wordpress.org>

cbravobernal · 2024-06-18T15:58:22Z

Dev note

WordPress 6.6 includes three new directives aimed at improving performance:

data-wp-on-async
data-wp-on-async-document
data-wp-on-async-window

These async directives optimize event callbacks by first yielding to the main thread. That way, complex interactions won't contribute to long tasks, improving the Interaction to Next Paint (INP). You can read more about it in Optimize Interaction to Next Paint.

These directives are recommended over the sync ones (data-wp-on, data-wp-on-document, and data-wp-on-window), but you can use them only when you don't need synchronous access to the event object, specifically if you need to call event.preventDefault(), event.stopPropagation(), or event.stopImmediatePropagation(). The directives in core blocks have been updated to use async where available.

When you must resort to using the non-async directives, the @wordpress/interactivity package now exports a splitTask function which can be used to manually split yield an action to the main thread after calling the synchronous event API. Please see the documentation on async actions for how to implement this.

westonruter · 2024-06-18T16:35:38Z

@cbravobernal Thanks for drafting that dev note! I've made some edits.

westonruter · 2024-06-18T17:02:08Z

I just realized that I didn't export yieldToMain. Proposed in #62665

cbravobernal · 2024-06-18T18:44:28Z

@cbravobernal Thanks for drafting that dev note! I've made some edits.

I initially put "that way" and GTP4 corrected me to "this way" 😆

westonruter · 2024-06-21T19:08:06Z

@cbravobernal I've updated the dev note comment above to incorporate discussion of the new splitTask function and to fix the names of the directives (also updated in #62759).

janusqa · 2024-07-15T15:11:59Z

Hi there. I've been looking at the docs here:
https://developer.wordpress.org/block-editor/reference-guides/interactivity-api/api-reference/

I am using wp-on-window--message to listen for messages sent from an IFRAME to the parent window.
wp-on-window--message seems to work ok. I get the message, but when i switch over to wp-on-async-window--message no messages arrive.

I've also read this from the PR. Does it mean I'm too early to the party to be using this? The docs indicate 6.5 or higher.

WordPress 6.6 includes three new directives aimed at improving performance:

    data-wp-on-async
    data-wp-on-async-document
    data-wp-on-async-window

The particulars:

wordpress 6.5.5
"@wordpress/interactivity": "^6.3.0",
"@wordpress/scripts": "^27.9.0",

The basics

render.php

<div data-wp-on-async-window--message="actions.processPostMessage" id="<?php echo $block_unique_id ?>" class="the-view" data-wp-interactive="create-block" <?php echo wp_interactivity_data_wp_context($block_context) ?>>
<!-- some html goes here -->
</div>
<iframe class="iframe-container" name="theFrame"></iframe>

view.js

/**
 * WordPress dependencies
 */
import { store, getContext } from '@wordpress/interactivity';
import { longRunningAsyncFunction } from './thefunction';

store('create-block', {
    actions: {
        processPostMessage: function* (event) {
            if (event.origin !== 'http://localhost:9000') return;
            const context = getContext();
            event.data.elementArr.push({ theThing: context.the_thing });
            yield longRunningAsyncFunction(event);
        },
    },
    callbacks: {},
});

westonruter · 2024-07-15T16:31:39Z

The async directives are new in 6.6.

westonruter added [Type] Enhancement A suggestion for improvement. [Type] Performance Related to performance efforts [Feature] Interactivity API API to add frontend interactivity to blocks. [Packages] Interactivity /packages/interactivity labels May 23, 2024

westonruter requested review from luisherranz, DAreRodz, artemiomorales and michalczaplinski as code owners May 23, 2024 00:41

westonruter mentioned this pull request May 23, 2024

Event directives can only be synchronous without the option for them to be asynchronous/passive #61634

Closed

cbravobernal removed the [Type] Enhancement A suggestion for improvement. label May 23, 2024

cbravobernal previously approved these changes May 23, 2024

View reviewed changes

cbravobernal self-requested a review May 23, 2024 12:28

cbravobernal force-pushed the add/data-wp-on-async-directive branch from fa2f799 to acab48d Compare May 23, 2024 12:32

cbravobernal approved these changes May 23, 2024

View reviewed changes

westonruter and others added 6 commits May 28, 2024 12:49

Improve typing for data-wp-on directive

d27dbb3

Move yieldToMain to utils

b9c8f04

fixup! Improve typing for data-wp-on directive

074240f

Introduce on-async directives

9b610cc

Use on-async directives in image block

7ed67d0

Update changelog

6f371be

Fix typo in comment

f498789

cbravobernal force-pushed the add/data-wp-on-async-directive branch from 9a350d3 to f498789 Compare May 28, 2024 10:50

cbravobernal enabled auto-merge (squash) May 28, 2024 10:52

cbravobernal merged commit 9c95f91 into trunk May 28, 2024
61 checks passed

cbravobernal deleted the add/data-wp-on-async-directive branch May 28, 2024 11:29

github-actions bot added this to the Gutenberg 18.5 milestone May 28, 2024

adamsilverstein added the Needs Dev Note Requires a developer note for a major WordPress release cycle label May 28, 2024

cbravobernal mentioned this pull request May 28, 2024

Interactivity API: Iteration for WP 6.6 #60219

Closed

juanmaguitar mentioned this pull request May 29, 2024

6.6 Dev Note Tracking Issue #62096

Closed

49 tasks

westonruter mentioned this pull request May 31, 2024

Use data-wp-on-async directives in core blocks when handler does not need synchronous access to event #62160

Merged

cbravobernal mentioned this pull request Jun 18, 2024

Interactivity API docs: Add wp-async directives doc #62663

Merged

cbravobernal added has dev note when dev note is done (for upcoming WordPress release) and removed Needs Dev Note Requires a developer note for a major WordPress release cycle labels Jun 18, 2024

westonruter mentioned this pull request Jun 18, 2024

Rename yieldToMain to splitTask and export from @wordpress/interactivity #62665

Merged

sirreal mentioned this pull request Aug 19, 2024

Interactivity API: Event directives cannot be passive #58705

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce `wp-on-async` directive as performant alternative over synchronous `wp-on` directive #61885

Introduce `wp-on-async` directive as performant alternative over synchronous `wp-on` directive #61885

westonruter commented May 23, 2024 •

edited

Loading

github-actions bot commented May 23, 2024 •

edited

Loading

github-actions bot commented May 23, 2024

github-actions bot commented May 23, 2024 •

edited

Loading

cbravobernal commented May 23, 2024

cbravobernal left a comment •

edited

Loading

westonruter commented May 23, 2024 •

edited

Loading

westonruter commented May 23, 2024

cbravobernal commented May 27, 2024

westonruter commented May 28, 2024

gziolo commented May 28, 2024 •

edited

Loading

cbravobernal commented May 28, 2024

adamsilverstein commented May 28, 2024

westonruter commented May 31, 2024

cbravobernal commented Jun 18, 2024 •

edited by westonruter

Loading

westonruter commented Jun 18, 2024

westonruter commented Jun 18, 2024

cbravobernal commented Jun 18, 2024 •

edited

Loading

westonruter commented Jun 21, 2024

janusqa commented Jul 15, 2024 •

edited

Loading

westonruter commented Jul 15, 2024

Introduce wp-on-async directive as performant alternative over synchronous wp-on directive #61885

Introduce wp-on-async directive as performant alternative over synchronous wp-on directive #61885

Conversation

westonruter commented May 23, 2024 • edited Loading

What?

Why?

How?

Testing Instructions

Screenshots or screencast

Before (?try-slow-events=sync)

After (?try-slow-events=async)

github-actions bot commented May 23, 2024 • edited Loading

github-actions bot commented May 23, 2024

github-actions bot commented May 23, 2024 • edited Loading

cbravobernal commented May 23, 2024

cbravobernal left a comment • edited Loading

Choose a reason for hiding this comment

westonruter commented May 23, 2024 • edited Loading

westonruter commented May 23, 2024

cbravobernal commented May 27, 2024

westonruter commented May 28, 2024

gziolo commented May 28, 2024 • edited Loading

cbravobernal commented May 28, 2024

adamsilverstein commented May 28, 2024

westonruter commented May 31, 2024

cbravobernal commented Jun 18, 2024 • edited by westonruter Loading

westonruter commented Jun 18, 2024

westonruter commented Jun 18, 2024

cbravobernal commented Jun 18, 2024 • edited Loading

westonruter commented Jun 21, 2024

janusqa commented Jul 15, 2024 • edited Loading

westonruter commented Jul 15, 2024

Introduce `wp-on-async` directive as performant alternative over synchronous `wp-on` directive #61885

Introduce `wp-on-async` directive as performant alternative over synchronous `wp-on` directive #61885

westonruter commented May 23, 2024 •

edited

Loading

Before (`?try-slow-events=sync`)

After (`?try-slow-events=async`)

github-actions bot commented May 23, 2024 •

edited

Loading

github-actions bot commented May 23, 2024 •

edited

Loading

cbravobernal left a comment •

edited

Loading

westonruter commented May 23, 2024 •

edited

Loading

gziolo commented May 28, 2024 •

edited

Loading

cbravobernal commented Jun 18, 2024 •

edited by westonruter

Loading

cbravobernal commented Jun 18, 2024 •

edited

Loading

janusqa commented Jul 15, 2024 •

edited

Loading