Skip to content
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 withSyncEvent action wrapper utility and proxy event object whenever it is not used #68097

Open
wants to merge 4 commits into
base: trunk
Choose a base branch
from

Conversation

felixarntz
Copy link
Member

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

  • In order for directive implementations to access resolved data from the entries, a few new functions are needed. This is because the evaluate function resolves the entry into the relevant data, but then also immediately evaluates it.
    • For the case of an event listener callback, this means the evaluate function will not only "look up" the callback, but also call it.
    • Since we need to conditionally alter the event object based on whether the callback is wrapped in withSyncEvent prior to calling the callback, the evaluate function is not sufficient.
    • In a future PR, we would then also need to call splitTask() before running the callback function (see 2nd step from Introduce withSyncEvent and require Interactivity API actions that use the event object to use it #64944).
  • As a solution, this PR introduces two new functions resolveEntry and evaluateResolved. If they are called right after each other, it's effectively the same as evaluate. In fact, this PR updates evaluate to rely on these two lower-level functions, to avoid duplicate code.
  • Most directives can still use evaluate as is, which is more ergonomic. However the on (but not on-async) directives require the more granular access of the callback first, to only execute it later.

Work in progress, but early reviews are appreciated.

So far, I have not implemented the actual proxying yet. I would like to get a sense of whether this minor expansion of the Interactivity API infrastructure makes sense before continuing.

@felixarntz felixarntz added [Feature] Interactivity API API to add frontend interactivity to blocks. [Packages] Interactivity /packages/interactivity [Type] Enhancement A suggestion for improvement. [Type] Performance Related to performance efforts and removed [Feature] Interactivity API API to add frontend interactivity to blocks. [Type] Enhancement A suggestion for improvement. labels Dec 18, 2024
Copy link

github-actions bot commented Dec 18, 2024

Flaky tests detected in dad7083.
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/12589925664
📝 Reported issues:

@gziolo
Copy link
Member

gziolo commented Dec 23, 2024

So far, I have not implemented the actual proxying yet. I would like to get a sense of whether this minor expansion of the Interactivity API infrastructure makes sense before continuing.

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 🎉

Copy link
Contributor

@michalczaplinski michalczaplinski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@felixarntz felixarntz marked this pull request as ready for review January 2, 2025 23:35
Copy link

github-actions bot commented Jan 2, 2025

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: felixarntz <[email protected]>
Co-authored-by: michalczaplinski <[email protected]>
Co-authored-by: gziolo <[email protected]>

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

@felixarntz
Copy link
Member Author

felixarntz commented Jan 2, 2025

I have implemented the event proxying logic. For now, I'm warning about accessing the following without withSyncEvent():

  • event.currentTarget
  • event.preventDefault()
  • event.stopImmediatePropagation()
  • event.stopPropagation()

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 withSyncEvent() after at least one release cycle (which is why the warning message mentions WordPress 6.9, as this enhancement ideally launches as part of WordPress 6.8).

Review much appreciated!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Interactivity API API to add frontend interactivity to blocks. [Packages] Interactivity /packages/interactivity [Type] Performance Related to performance efforts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants