Interactivity API: Allow multiple event handlers for the same type with data-wp-on-document and data-wp-on-window.#61009
Merged
Conversation
|
👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @cbravobernal! In case you missed it, we'd love to have you join us in our Slack community. If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information. |
github-actions
Bot
added
the
First-time Contributor
cbravobernal
added
[Feature] Interactivity API
|
Size Change: +12 B (0%) Total Size: 1.74 MB
ℹ️ View Unchanged
|
cbravobernal
requested review from
DAreRodz,
ajitbohra,
luisherranz,
nerrad and
ntwb
as code owners
|
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. |
|
Flaky tests detected in 656a2d6. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/8803811809
|
sirreal
reviewed
Apr 24, 2024
Comment on lines
+214
to
+218
| const event = entry.suffix.split( '--' )[ 0 ]; | ||
| if ( ! events.has( event ) ) events.set( event, new Set() ); | ||
| events.get( event ).add( entry ); | ||
| } ); | ||
| events.forEach( ( entries, eventType ) => { |
Member
There was a problem hiding this comment.
What's the motivation for doing this in two passes (iterate and collect in Map, then iterate on the map) instead of setting the event listeners in a single pass? I see this is the approach used in #60661 as well.
Contributor
Author
There was a problem hiding this comment.
I did the same approach as #60661 to keep consistency in the codebase. Do you want me to refactor both on this PR or make a follow-up one?
Member
There was a problem hiding this comment.
If the collection and second iteration is redundant, it seems preferable to do it on a single pass.
Do you want me to refactor both on this PR or make a follow-up one?
In this PR is fine with me, but I'll leave it up to whatever you prefer.
Member
There was a problem hiding this comment.
It turns out this PR is different from the other PR. Here, we're using addEventListener to add listeners to window or document, but that other PR is adding multiple event listeners effectively via props. That PR therefor creates a single listener that needs to call all of the listeners internally and passes it via the on${Event} prop:
gutenberg/packages/interactivity/src/directives.js
Lines 283 to 287 in cbdf540
We don't have that same constraint here, each event listener can be added and removed on its own, so I think we'll need to keep the approach used in the other PR, while this PR can remove the collection + iteration and add the listeners in a single pass.
sirreal
reviewed
Apr 29, 2024
| directives[ `on-${ type }` ] | ||
| .filter( ( { suffix } ) => suffix !== 'default' ) | ||
| .forEach( ( entry ) => { | ||
| const event = entry.suffix.split( '--' )[ 0 ]; |
Member
There was a problem hiding this comment.
Can entry.suffix ever be "" or "--…"? That may be something to improve in some earlier processing. We shouldn't be attaching event listeners to events like "".
Contributor
Author
There was a problem hiding this comment.
Suffix initially cannot be "" . If you add a directive like data-wp-on-document----a, it will an empty string as an event, as you mention.
data-wp-on-document-- directly crashes the Interactivity runtime.
Member
There was a problem hiding this comment.
Thanks for opening #61249, let's use that to make sure only "good" suffixes reach this point.
Do we want to use a limit to avoid splitting more than we'd use?
Suggested change
| const event = entry.suffix.split( '--' )[ 0 ]; | |
| const event = entry.suffix.split( '--', 1 )[ 0 ]; |
Contributor
There was a problem hiding this comment.
+1 to only split the suffix once.
sirreal
requested changes
May 6, 2024
| const event = entry.suffix.split( '--', 1 )[ 0 ]; | ||
| useInit( () => { | ||
| const cb = ( event ) => evaluate( entry, event ); | ||
| const cb = () => evaluate( entry, event ); |
Member
There was a problem hiding this comment.
This breaks the event handlers, it's replacing the event passed to the event handler with the name of the event in an outer scope:
Suggested change
| const cb = () => evaluate( entry, event ); | |
| const cb = ( event ) => evaluate( entry, event ); |
sirreal
approved these changes
May 7, 2024
Comment on lines
+213
to
+215
| const event = entry.suffix.split( '--', 1 )[ 0 ]; | ||
| useInit( () => { | ||
| const cb = ( event ) => evaluate( entry, event ); | ||
| const cb = ( cbEvent ) => evaluate( entry, cbEvent ); |
Member
There was a problem hiding this comment.
Removing the name shadowing is good there 👍 Small nit - I would prefer eventName for the outer and event for the inner which seem more descriptive.
Member
|
Please be sure to add a changelog entry for this. |
cbravobernal
force-pushed
the
feature/allow-multiple-data-wp-on-w-d
branch
from
a0d3398 to
19cd289
Compare
ajlende
added
the
[Feature] Interactivity API
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
What?
Fixes #60683 by allowing multiple
data-wp-on-documentanddata-wp-on-windowevents in the same element.This PR allows declaring multiple event listeners with
data-wp-on-windowanddata-wp-on-documentfor the same event type in the same element. Example:The syntax is the same as the rest of the directives (i.e., a unique ID is appended at the end of the directive name).
Why?
This could be considered a bug fix. The limitation of the number of event handlers that can be registered (only one per element & event type) affects the extensibility of interactive blocks, as well as the reuse of code (e.g., store callbacks).
In addition, all other directives already allow this syntax for the same reason.
How?
The directive internally groups all the event handlers with the same type and assigns a function to the corresponding
on-document-${event}on-window-${event}or property in the element that runs all the event handlers consecutively.This fix has some limitations that could be addressed later on:
directives.on-windowordirectives.on-domentries array that thedata-wp-on-documentanddata-wp-on-windowdirectives function receive. That depends on the order they appear in the HTML, which is not guaranteed.Testing Instructions
I've added a couple of E2E tests that should pass.