Introduce withSyncEvent action wrapper utility and proxy event object whenever it is not used
#68097
+335
−92
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
felixarntz
added
[Feature] Interactivity API
|
Flaky tests detected in 682ee6f. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/13042516552
|
I don't know the codebase, but you are on track as much as I understand the changes and their implications. Thank you for continuing to work on it. I also wanted to share that it might not be easy to get more feedback in the next 2-3 weeks as many folks, myself included, take longer holiday breaks 🎉 |
45 tasks
There was a problem hiding this comment.
I gave it an initial review and so far it's looking good. The approach of splitting evaluate into 2 functions seems sound to me. I see that you haven't implemented the proxying yet, so I'm happy to take another look once that's closer to being ready.
PS. The Interactivity API is almost entirely tested using the e2e suite in test/e2e/specs/interactivity/. For this PR, you'll probably need to modify some of the existing "test" blocks like packages/e2e-tests/plugins/interactive-blocks/directive-on/view.js or create a new "test" block in that folder.
|
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. |
|
I have implemented the event proxying logic. For now, I'm warning about accessing the following without
As discussed on the issue, there may be more properties and methods to warn about, but we could always expand later. I think these 4 are a good starting point and should cover what's most common. Keep in mind that for now there is only a warning, but no functional behavior change. We can only yield before all callbacks that aren't using Review much appreciated! |
|
Any chance I can get 1-2 reviews for the PR this week? That would be super helpful so we could ideally land this in Gutenberg soon. Thank you! 🙌 |
…y to still be recognized as generator functions.
felixarntz
requested review from
ntwb,
nerrad,
ajitbohra,
artemiomorales,
fabiankaegy,
juanmaguitar and
ndiego
as code owners
|
Some updates:
|
|
Size Change: +736 B (+0.04%) Total Size: 1.84 MB
ℹ️ View Unchanged
|
felixarntz
commented
Jan 22, 2025
| @@ -1253,6 +1256,43 @@ store( 'mySliderPlugin', { | |||
| } ); | |||
| ``` | |||
|
|
|||
| ### withSyncEvent() | |||
|
|
|||
| Actions that require synchronous access to the `event` object need to use the `withSyncEvent()` function to wrap their handler callback. This is necessary due to an ongoing effort to handle store actions asynchronously by default, unless they require synchronous event access. Therefore, as of Gutenberg `TODO: Add release number here!` / WordPress 6.8 all actions that require synchronous event access need to use the `withSyncEvent()` utility wrapper function. Otherwise a deprecation warning will be triggered, and in a future release the behavior will change accordingly. | |||
There was a problem hiding this comment.
Note: The TODO here can only be addressed once this is ready for merge and we know which Gutenberg version this will be released in.
westonruter
requested changes
Jan 28, 2025
packages/interactivity/src/utils.ts
Outdated
| * @param callback The event callback. | ||
| * @return Wrapped event callback. | ||
| */ | ||
| export const withSyncEvent = ( callback: Function ): Function => { |
There was a problem hiding this comment.
I think it's preferable to use a function rather than an anonymous function so that the function name will more reliably appear in the call stack.
Suggested change
| export const withSyncEvent = ( callback: Function ): Function => { | |
| export function withSyncEvent( callback: Function ): Function => { |
That said, it seems (in consulting with Gemini) modern debuggers are able to guess the name of the anonymous function based on context. Still, something explicit seems better.
There was a problem hiding this comment.
I was looking at the functions above which were using const, but fair point. Looks like most of the file is using functions and I agree that's better. Updating...
There was a problem hiding this comment.
Done as part of bf3dbcc
packages/interactivity/src/utils.ts
Outdated
Comment on lines
385
to
395
| if ( callback?.constructor?.name === 'GeneratorFunction' ) { | ||
| const wrapped = function* ( ...args: any[] ) { | ||
| yield* callback( ...args ); | ||
| }; | ||
| wrapped.sync = true; | ||
| return wrapped; | ||
| } | ||
|
|
||
| const wrapped = ( ...args: any[] ) => callback( ...args ); | ||
| wrapped.sync = true; | ||
| return wrapped; |
There was a problem hiding this comment.
Removing a bit of code duplication:
Suggested change
| if ( callback?.constructor?.name === 'GeneratorFunction' ) { | |
| const wrapped = function* ( ...args: any[] ) { | |
| yield* callback( ...args ); | |
| }; | |
| wrapped.sync = true; | |
| return wrapped; | |
| } | |
| const wrapped = ( ...args: any[] ) => callback( ...args ); | |
| wrapped.sync = true; | |
| return wrapped; | |
| let wrapped; | |
| if ( callback?.constructor?.name === 'GeneratorFunction' ) { | |
| wrapped = function* ( ...args: any[] ) { | |
| yield* callback( ...args ); | |
| }; | |
| } else { | |
| wrapped = ( ...args: any[] ) => callback( ...args ); | |
| } | |
| wrapped.sync = true; | |
| return wrapped; |
There was a problem hiding this comment.
I had tried to do this initially in the same way, but it doesn't work:
- The
let wrapped;will require a type. - Setting it to
Functionwill cause the.syncassignment to not work because that's not available on a function object. - Setting it to
anywould work, but not be great becauseany.
I just searched a bit further and found https://stackoverflow.com/questions/12766528/build-a-function-object-with-properties-in-typescript. That seems to work better, so I'm gonna go with that.
There was a problem hiding this comment.
Done as part of bf3dbcc.
I also identified a bug where the additional sync property was not being passed through, due to passing all store actions through the withScope() function, which happens via the proxifyStore() function.
By introducing an extra type SyncAwareFunction, this can be handled a bit more elegantly, as it allows to simply cast regular functions into that type and then set the sync property as needed (in practice, it's always true, or it's not set, i.e. a regular function).
I have tested this approach and it works well. It also ensures the original underlying JS types (e.g. Function or GeneratorFunction) are maintained.
| @@ -50,6 +50,39 @@ function deepClone< T >( source: T ): T { | |||
| return source; | |||
| } | |||
|
|
|||
| function wrapEventAsync( event: Event ) { | |||
| @@ -50,6 +50,39 @@ function deepClone< T >( source: T ): T { | |||
| return source; | |||
| } | |||
|
|
|||
| function wrapEventAsync( event: Event ) { | |||
| const handler = { | |||
| get( target: any, prop: any, receiver: any ) { | |||
There was a problem hiding this comment.
Suggested change
| get( target: any, prop: any, receiver: any ) { | |
| get( target: Event, prop: string, receiver: any ) { |
Maybe the typing of receiver could be made more specific as well.
There was a problem hiding this comment.
Done in 682ee6f
I used string | symbol for prop, in accordance with other proxy handlers already present in Interactivity API code. Those handlers also use receiver: any, so I think it's fine to leave that as-is.
| @@ -50,6 +50,39 @@ function deepClone< T >( source: T ): T { | |||
| return source; | |||
| } | |||
|
|
|||
| function wrapEventAsync( event: Event ) { | |||
There was a problem hiding this comment.
Suggested change
| function wrapEventAsync( event: Event ) { | |
| function wrapEventAsync( event: Event ): Proxy<Event> { |
There was a problem hiding this comment.
This doesn't appear to work: 'Proxy' refers to a value, but is being used as a type here.
packages/interactivity/src/utils.ts
Outdated
| return wrapped; | ||
| } | ||
|
|
||
| const wrapped = ( ...args: any[] ) => callback( ...args ); |
There was a problem hiding this comment.
Should this be preserving this context like you're doing in wrapEventAsync?
Suggested change
| const wrapped = ( ...args: any[] ) => callback( ...args ); | |
| const wrapped = function ( ...args: any[] ) { | |
| return callback.apply( this, args ) | |
| }; |
There was a problem hiding this comment.
Same for the generator variant above.
There was a problem hiding this comment.
Done as part of bf3dbcc
| if ( value instanceof Function ) { | ||
| return function ( this: any, ...args: any[] ) { | ||
| return value.apply( | ||
| this === receiver ? target : this, |
There was a problem hiding this comment.
What does this === receiver mean here? Why wouldn't it only use this?
There was a problem hiding this comment.
Co-authored-by: Weston Ruter <[email protected]>
…nberg into add/64944-with-sync-event
…tained when proxying functions via withScope.
|
I have done some extensive testing on the code changes and added testing instructions to the PR description. I identified one crucial bug, which was fixed (see #68097 (comment)). But everything is working well now, and you can work through the testing instructions to validate. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This implements the 1st step from #64944. Please see the issue for additional context on the rationale and overarching approach for this.
Relevant technical choices
evaluatefunction resolves the entry into the relevant data, but then also immediately evaluates it.evaluatefunction will not only "look up" the callback, but also call it.eventobject based on whether the callback is wrapped inwithSyncEventprior to calling the callback, theevaluatefunction is not sufficient.splitTask()before running the callback function (see 2nd step from IntroducewithSyncEventand require Interactivity API actions that use theeventobject to use it #64944).resolveEntryandevaluateResolved. If they are called right after each other, it's effectively the same asevaluate. In fact, this PR updatesevaluateto rely on these two lower-level functions, to avoid duplicate code.evaluateas is, which is more ergonomic. However theon(but noton-async) directives require the more granular access of the callback first, to only execute it later.Note to reviewers
Because several files are touched, but only some specific changes are non-trivial, I would advise to review the commits individually at first. That should help convey the bigger picture than looking at the whole PR diff at once.
Testing instructions
Tabkey and ensure no warnings are present in the browser console.handleKeydownaction of thecore/imageblock's store passes its callback throughwithSyncEvent.handleKeydownaction was doing it wrong: Editpackages/block-library/src/image/view.jsand remove thewithSyncEventusage from thehandleKeydowncallback, so that it is no longer wrapped. Don't forget to rerun the JS build.withSyncEventneeds to be used.openSearchInputaction of thecore/searchblock's store passes its callback throughwithSyncEvent.packages/block-library/src/search/view.jsand remove thewithSyncEventusage from theopenSearchInputcallback, so that it is no longer wrapped. Don't forget to rerun the JS build.withSyncEventneeds to be used.