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

Interactivity API: Prevent empty namespace or different namespaces from killing the runtime #61409

Merged
merged 10 commits into from
May 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/block-library/src/embed/util.js
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ export const removeAspectRatioClasses = ( existingClassNames ) => {
if ( ! existingClassNames ) {
// Avoids extraneous work and also, by returning the same value as
// received, ensures the post is not dirtied by a change of the block
// attribute from `undefined` to an emtpy string.
// attribute from `undefined` to an empty string.
return existingClassNames;
}
const aspectRatioClassNames = ASPECT_RATIOS.reduce(
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
{
"$schema": "https://schemas.wp.org/trunk/block.json",
"apiVersion": 2,
"name": "test-namespace/directive-bind",
"title": "E2E Interactivity tests - directive bind",
"category": "text",
"icon": "heart",
"description": "",
"supports": {
"interactivity": true
},
"textdomain": "e2e-interactivity",
"viewScriptModule": "file:./view.js",
"render": "file:./render.php"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
<?php
/**
* HTML for testing the directive `data-wp-bind`.
*
* @package gutenberg-test-interactive-blocks
*/
?>

<div data-wp-interactive="">
<a data-wp-bind--href="state.url" data-testid="empty namespace"></a>
</div>

<div data-wp-interactive="namespace">
<a data-wp-bind--href="state.url" data-testid="correct namespace"></a>
</div>

<div data-wp-interactive="{}">
<a data-wp-bind--href="state.url" data-testid="object namespace"></a>
</div>

<div data-wp-interactive>
<a data-wp-bind--href="other::state.url" data-testid="other namespace"></a>
</div>
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
<?php return array( 'dependencies' => array( '@wordpress/interactivity' ) );
16 changes: 16 additions & 0 deletions packages/e2e-tests/plugins/interactive-blocks/namespace/view.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
/**
* WordPress dependencies
*/
import { store } from '@wordpress/interactivity';

store( 'namespace', {
state: {
url: '/some-url',
},
} );

store( 'other', {
state: {
url: '/other-store-url',
},
} );
1 change: 1 addition & 0 deletions packages/interactivity/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
- 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))
- Prevent empty namespace or different namespaces from killing the runtime ([#61409](https://github.com/WordPress/gutenberg/pull/61409))

## 5.6.0 (2024-05-02)

Expand Down
10 changes: 3 additions & 7 deletions packages/interactivity/src/directives.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import { deepSignal, peek } from 'deepsignal';
import { useWatch, useInit } from './utils';
import { directive, getScope, getEvaluate } from './hooks';
import { kebabToCamelCase } from './utils/kebab-to-camelcase';
import { warn } from './utils/warn';

// Assigned objects should be ignore during proxification.
const contextAssignedObjects = new WeakMap();
Expand Down Expand Up @@ -242,13 +243,8 @@ export default () => {
if ( defaultEntry ) {
const { namespace, value } = defaultEntry;
// Check that the value is a JSON object. Send a console warning if not.
if (
typeof SCRIPT_DEBUG !== 'undefined' &&
SCRIPT_DEBUG === true &&
! isPlainObject( value )
) {
// eslint-disable-next-line no-console
console.warn(
if ( ! isPlainObject( value ) ) {
warn(
`The value of data-wp-context in "${ namespace }" store must be a valid stringified JSON object.`
);
}
Expand Down
15 changes: 12 additions & 3 deletions packages/interactivity/src/hooks.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import type { VNode, Context, RefObject } from 'preact';
* Internal dependencies
*/
import { store, stores, universalUnlock } from './store';
import { warn } from './utils/warn';
interface DirectiveEntry {
value: string | Object;
namespace: string;
Expand Down Expand Up @@ -260,18 +261,26 @@ export const directive = (

// Resolve the path to some property of the store object.
const resolve = ( path, namespace ) => {
if ( ! namespace ) {
warn(
`The "namespace" cannot be "{}", "null" or an empty string. Path: ${ path }`
);
return;
}
let resolvedStore = stores.get( namespace );
if ( typeof resolvedStore === 'undefined' ) {
resolvedStore = store( namespace, undefined, {
lock: universalUnlock,
} );
}
let current = {
const current = {
...resolvedStore,
context: getScope().context[ namespace ],
};
path.split( '.' ).forEach( ( p ) => ( current = current[ p ] ) );
return current;
try {
// TODO: Support lazy/dynamically initialized stores
return path.split( '.' ).reduce( ( acc, key ) => acc[ key ], current );
} catch ( e ) {}
};

// Generate the evaluate function.
Expand Down
21 changes: 21 additions & 0 deletions packages/interactivity/src/utils/warn.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
const logged = new Set();

export const warn = ( message ) => {
// @ts-expect-error
if ( typeof SCRIPT_DEBUG !== 'undefined' && SCRIPT_DEBUG === true ) {
Copy link
Member

Choose a reason for hiding this comment

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

Flagging as something I'll need to update in #61486

if ( logged.has( message ) ) {
return;
}

// eslint-disable-next-line no-console
console.warn( message );

// Adding a stack trace to the warning message to help with debugging.
try {
throw Error( message );
} catch ( e ) {
// Do nothing.
}
michalczaplinski marked this conversation as resolved.
Show resolved Hide resolved
logged.add( message );
}
};
11 changes: 2 additions & 9 deletions packages/interactivity/src/vdom.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { h } from 'preact';
* Internal dependencies
*/
import { directivePrefix as p } from './constants';
import { warn } from './utils/warn';

const ignoreAttr = `data-${ p }-ignore`;
const islandAttr = `data-${ p }-interactive`;
Expand Down Expand Up @@ -120,15 +121,7 @@ export function toVdom( root ) {
( obj, [ name, ns, value ] ) => {
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 }.` );
}
warn( `Invalid directive: ${ name }.` );
return obj;
}
const prefix = directiveMatch[ 1 ] || '';
Expand Down
49 changes: 49 additions & 0 deletions test/e2e/specs/interactivity/namespace.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
/**
* Internal dependencies
*/
import { test, expect } from './fixtures';

test.describe( 'Namespaces', () => {
test.beforeAll( async ( { interactivityUtils: utils } ) => {
await utils.activatePlugins();
await utils.addPostWithBlock( 'test-namespace/directive-bind' );
} );

test.beforeEach( async ( { interactivityUtils: utils, page } ) => {
await page.goto( utils.getLink( 'test-namespace/directive-bind' ) );
} );

test.afterAll( async ( { interactivityUtils: utils } ) => {
await utils.deactivatePlugins();
await utils.deleteAllPosts();
} );

test( 'Empty string as namespace should not work', async ( { page } ) => {
const el = page.getByTestId( 'empty namespace' );
await expect( el ).not.toHaveAttribute( 'href', '/some-url' );
} );

test( 'A string as namespace should work', async ( { page } ) => {
const el = page.getByTestId( 'correct namespace' );
await expect( el ).toHaveAttribute( 'href', '/some-url' );
} );

test( 'An empty object as namespace should work', async ( { page } ) => {
const el = page.getByTestId( 'object namespace' );
await expect( el ).not.toHaveAttribute( 'href', '/some-url' );
} );

test( 'A wrong namespace should not break the runtime', async ( {
page,
} ) => {
const el = page.getByTestId( 'object namespace' );
await expect( el ).not.toHaveAttribute( 'href', '/some-url' );
const correct = page.getByTestId( 'correct namespace' );
await expect( correct ).toHaveAttribute( 'href', '/some-url' );
} );

test( 'A different store namespace should work', async ( { page } ) => {
const el = page.getByTestId( 'other namespace' );
await expect( el ).toHaveAttribute( 'href', '/other-store-url' );
} );
} );
Loading