-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Introduce wp-on-async
directive as performant alternative over synchronous wp-on
directive
#61885
Conversation
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
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 |
Flaky tests detected in 9a350d3. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9211662096
|
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 How can we guide developers for the best approach, apart from updating the docs? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
fa2f799
to
acab48d
Compare
That would be ideal, if the back-compat breakage is acceptable. Basically, synchronous events are required when passing in an
One possibility is that we warn users about switching to use |
Humm. This doesn't seem to work as I expected because the actions/callbacks are getting wrapped in additional functions. My failed patchdiff --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 (...args) => {
(0,_hooks__WEBPACK_IMPORTED_MODULE_1__.setNamespace)(ns);
try {
return result(...args);
} finally {
(0,_hooks__WEBPACK_IMPORTED_MODULE_1__.resetNamespace)();
}
} |
We only have this week to land this change before beta 1. Should we merge this, go with the |
@cbravobernal That makes sense to me! |
Let's remember to document the new API. It should be very similar to: It would be also worth explaining the nuances and when to use |
9a350d3
to
f498789
Compare
I fixed the changelog conflict. |
I added the "Needs Dev Note label here - this feature is worth calling out for the 6.6 core release. |
I've opened #62160 to implement |
…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>
…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>
WordPress 6.6 includes three new directives aimed at improving performance:
These These directives are recommended over the sync ones ( When you must resort to using the non-async directives, the |
@cbravobernal Thanks for drafting that dev note! I've made some edits. |
I just realized that I didn't export |
I initially put "that way" and GTP4 corrected me to "this way" 😆 |
@cbravobernal I've updated the dev note comment above to incorporate discussion of the new |
Hi there. I've been looking at the docs here: I am using wp-on-window--message to listen for messages sent from an IFRAME to the parent window. 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.
The particulars:
The basics render.php
view.js
|
The async directives are new in 6.6. |
Fixes #61634.
What?
This adds an alternative async variation to the
data-wp-on
directives by tagging on an-async
to the directive name. So instead ofdata-wp-on--click
it can bedata-wp-on-async--click
. This is also done for thedata-wp-on-window
anddata-wp-on-document
directives withdata-wp-on-async-window
anddata-wp-on-async-document
respectively (which here can also be madepassive
).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
img
in an Image block.?try-slow-events=sync
to the URL and try opening the image in a lightbox. Notice the latency.?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.)