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
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
19 changes: 16 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,30 @@ 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 emtpy string. Path: ${ path }`
michalczaplinski marked this conversation as resolved.
Show resolved Hide resolved
);
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 ) {
warn(
`The path "${ path }" could not be resolved in the "${ namespace }" store.`
Copy link
Member

Choose a reason for hiding this comment

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

Loading a store after the initial hydration is legit; I don't think we should add a warning that we must remove later. It's going to cause confusion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed in f587e3d

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, Carlos.

);
}
};

// 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