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.

Sign up for GitHub

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

Jump to bottom

Interactivity API: Prevent wrong written directives from killing the runtime #61249

Merged
merged 8 commits into from
May 7, 2024
Merged

Interactivity API: Prevent wrong written directives from killing the runtime #61249

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
ce2eb1b
Prevent error on vdom
cbravobernal Apr 30, 2024
4ab2b12
Refactor
cbravobernal Apr 30, 2024
217f623
Remove blank space
cbravobernal Apr 30, 2024
e3d6bfb
Early warn, add test
cbravobernal May 6, 2024
e7e06a4
Move test and add comment
cbravobernal May 6, 2024
b68ad28
Refactor
cbravobernal May 6, 2024
ae3b8e3
Use null
cbravobernal May 6, 2024
1df720c
Move changelog
cbravobernal May 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion packages/e2e-tests/plugins/interactive-blocks/directive-on/render.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@
*/
?>

<div data-wp-interactive="directive-on">
<?php // A wrong directive name like "data-wp-on--" should not kill the interactivity. ?>
<div data-wp-interactive="directive-on" data-wp-on--="">
<div>
<p data-wp-text="state.counter" data-testid="counter">0</p>
<button
Expand Down
2 changes: 2 additions & 0 deletions packages/interactivity/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@

- Allow multiple event handlers for the same type with `data-wp-on-document` and `data-wp-on-window`. ([#61009](https://github.com/WordPress/gutenberg/pull/61009))

- Prevent wrong written directives from killing the runtime ([#61249](https://github.com/WordPress/gutenberg/pull/61249))

## 5.6.0 (2024-05-02)

## 5.5.0 (2024-04-19)
Expand Down
20 changes: 16 additions & 4 deletions packages/interactivity/src/vdom.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,11 +118,23 @@ export function toVdom( root ) {
if ( directives.length ) {
props.__directives = directives.reduce(
( obj, [ name, ns, value ] ) => {
const [ , prefix, suffix = 'default' ] =
directiveParser.exec( name );
if ( ! obj[ prefix ] ) {
obj[ prefix ] = [];
const directiveMatch = directiveParser.exec( name );
if ( directiveMatch === null ) {
if (
// @ts-expect-error This is a debug-only warning.
typeof SCRIPT_DEBUG !== 'undefined' &&
// @ts-expect-error This is a debug-only warning.
SCRIPT_DEBUG === true
) {
// eslint-disable-next-line no-console
console.warn( `Invalid directive: ${ name }.` );
}
return obj;
}
const prefix = directiveMatch[ 1 ] || '';
const suffix = directiveMatch[ 2 ] || 'default';

obj[ prefix ] = obj[ prefix ] || [];
cbravobernal marked this conversation as resolved.
Show resolved Hide resolved
Comment on lines +134 to +137
Copy link
Member

Choose a reason for hiding this comment

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

This section could probably use some comments about how strings are being split up and what we expect to find in each part. We've even got a constant wp that's the directivePrefix (that also appears in the middle of the data attribute).

Honestly, some kind of description of what the parts of a directive are and a refactor to rename things accordingly would be helpful. prefix and suffix don't really make sense:


nothing?       suffix is everything after `prefix--`… I guess this makes sense
   |              |
vvvvvvv       vvvvvvvvvv   
data-wp-bind--on--change
        ^^^^
          |
      "prefix" in the middle?

Maybe we could talk about these things like the "directive name" or "directive type", then the rest of it could be some kind of input to the directive? I think we use these prefix/suffix names across the package and it seems confusing.

The bug fix here where we try to access a null match is good 👍

obj[ prefix ].push( {
namespace: ns ?? currentNamespace(),
value,
Expand Down