From b3dcbff741c758eca71da8d2266bf8538aa54094 Mon Sep 17 00:00:00 2001 From: Carlos Bravo Date: Fri, 24 May 2024 18:41:01 +0200 Subject: [PATCH 1/6] Fix null and number strings as namespaces runtime error --- .../interactive-blocks/namespace/render.php | 8 ++++++ .../interactive-blocks/namespace/view.js | 25 +++++++++++++++++++ packages/interactivity/src/store.ts | 4 +-- packages/interactivity/src/utils.ts | 3 +++ packages/interactivity/src/vdom.ts | 5 ++-- .../e2e/specs/interactivity/namespace.spec.ts | 16 +++++++++++- 6 files changed, 55 insertions(+), 6 deletions(-) diff --git a/packages/e2e-tests/plugins/interactive-blocks/namespace/render.php b/packages/e2e-tests/plugins/interactive-blocks/namespace/render.php index 6fdc2d07e350c..c2507489f33aa 100644 --- a/packages/e2e-tests/plugins/interactive-blocks/namespace/render.php +++ b/packages/e2e-tests/plugins/interactive-blocks/namespace/render.php @@ -18,6 +18,14 @@ +
+ +
+ +
+ +
+
diff --git a/packages/e2e-tests/plugins/interactive-blocks/namespace/view.js b/packages/e2e-tests/plugins/interactive-blocks/namespace/view.js index 9225f88ce9d27..354745499fff4 100644 --- a/packages/e2e-tests/plugins/interactive-blocks/namespace/view.js +++ b/packages/e2e-tests/plugins/interactive-blocks/namespace/view.js @@ -3,6 +3,13 @@ */ import { store } from '@wordpress/interactivity'; + +store( '', { + state: { + url: '/some-url', + }, +} ); + store( 'namespace', { state: { url: '/some-url', @@ -14,3 +21,21 @@ store( 'other', { url: '/other-store-url', }, } ); + +store( 'null', { + state: { + url: '/some-url', + }, +} ); + +store( '2', { + state: { + url: '/some-url', + }, +} ); + +store( '{}', { + state: { + url: '/other-store-url', + }, +} ); diff --git a/packages/interactivity/src/store.ts b/packages/interactivity/src/store.ts index d173f2cd842dc..46aa3c62d26c8 100644 --- a/packages/interactivity/src/store.ts +++ b/packages/interactivity/src/store.ts @@ -15,9 +15,7 @@ import { setNamespace, resetNamespace, } from './hooks'; - -const isObject = ( item: unknown ): item is Record< string, unknown > => - Boolean( item && typeof item === 'object' && item.constructor === Object ); +import { isObject } from './utils'; const deepMerge = ( target: any, source: any ) => { if ( isObject( target ) && isObject( source ) ) { diff --git a/packages/interactivity/src/utils.ts b/packages/interactivity/src/utils.ts index e709ba1ce3b0e..273174d586eb3 100644 --- a/packages/interactivity/src/utils.ts +++ b/packages/interactivity/src/utils.ts @@ -334,3 +334,6 @@ export const warn = ( message: string ): void => { logged.add( message ); } }; + +export const isObject = ( item: unknown ): item is Record< string, unknown > => + Boolean( item && typeof item === 'object' && item.constructor === Object ); diff --git a/packages/interactivity/src/vdom.ts b/packages/interactivity/src/vdom.ts index cc481f89ff772..0914b8918230e 100644 --- a/packages/interactivity/src/vdom.ts +++ b/packages/interactivity/src/vdom.ts @@ -6,7 +6,7 @@ import { h, type ComponentChild, type JSX } from 'preact'; * Internal dependencies */ import { directivePrefix as p } from './constants'; -import { warn } from './utils'; +import { warn, isObject } from './utils'; const ignoreAttr = `data-${ p }-ignore`; const islandAttr = `data-${ p }-interactive`; @@ -100,7 +100,8 @@ export function toVdom( root: Node ): Array< ComponentChild > { const namespace = regexResult?.[ 1 ] ?? null; let value: any = regexResult?.[ 2 ] ?? attributeValue; try { - value = value && JSON.parse( value ); + const parsedValue = JSON.parse( value ); + value = isObject( parsedValue ) ? parsedValue : value; } catch ( e ) {} if ( attributeName === islandAttr ) { island = true; diff --git a/test/e2e/specs/interactivity/namespace.spec.ts b/test/e2e/specs/interactivity/namespace.spec.ts index fd38a7ecf6ed6..ebbaed7bb6338 100644 --- a/test/e2e/specs/interactivity/namespace.spec.ts +++ b/test/e2e/specs/interactivity/namespace.spec.ts @@ -28,7 +28,9 @@ test.describe( 'Namespaces', () => { await expect( el ).toHaveAttribute( 'href', '/some-url' ); } ); - test( 'An empty object as namespace should work', async ( { page } ) => { + test( 'An empty object as namespace should not work', async ( { + page, + } ) => { const el = page.getByTestId( 'object namespace' ); await expect( el ).not.toHaveAttribute( 'href', '/some-url' ); } ); @@ -46,4 +48,16 @@ test.describe( 'Namespaces', () => { const el = page.getByTestId( 'other namespace' ); await expect( el ).toHaveAttribute( 'href', '/other-store-url' ); } ); + + test( 'A number as a string as namespace should work', async ( { + page, + } ) => { + const el = page.getByTestId( 'number namespace' ); + await expect( el ).toHaveAttribute( 'href', '/some-url' ); + } ); + + test( 'A null as a string as namespace should work', async ( { page } ) => { + const el = page.getByTestId( 'null namespace' ); + await expect( el ).toHaveAttribute( 'href', '/some-url' ); + } ); } ); From b7b6da8f447a974daf807c8034119ccd6824fcaa Mon Sep 17 00:00:00 2001 From: Carlos Bravo Date: Mon, 27 May 2024 10:31:32 +0200 Subject: [PATCH 2/6] Update with suggestions --- .../src/components/inserter/style.scss | 13 +-------- .../interactive-blocks/namespace/render.php | 14 +++++++++ .../interactive-blocks/namespace/view.js | 29 +++++++++++++++++++ packages/interactivity/src/vdom.ts | 2 +- .../e2e/specs/interactivity/namespace.spec.ts | 24 +++++++++++++++ 5 files changed, 69 insertions(+), 13 deletions(-) diff --git a/packages/block-editor/src/components/inserter/style.scss b/packages/block-editor/src/components/inserter/style.scss index 604268fb1fe43..1bca09ec92bc1 100644 --- a/packages/block-editor/src/components/inserter/style.scss +++ b/packages/block-editor/src/components/inserter/style.scss @@ -116,18 +116,7 @@ $block-inserter-tabs-height: 44px; overflow: hidden; .block-editor-inserter__tablist { - width: 100%; - - button[role="tab"] { - flex-grow: 1; - margin-bottom: -$border-width; - &[id$="reusable"] { - flex-grow: inherit; - // These are to align the `reusable` icon with the search icon. - padding-left: $grid-unit-20; - padding-right: $grid-unit-20; - } - } + margin-bottom: -$border-width; } .block-editor-inserter__tabpanel { diff --git a/packages/e2e-tests/plugins/interactive-blocks/namespace/render.php b/packages/e2e-tests/plugins/interactive-blocks/namespace/render.php index c2507489f33aa..0b7e40b6c1653 100644 --- a/packages/e2e-tests/plugins/interactive-blocks/namespace/render.php +++ b/packages/e2e-tests/plugins/interactive-blocks/namespace/render.php @@ -29,3 +29,17 @@
+ +
+ +
+ +
+ +
+
+ +
+
+ +
diff --git a/packages/e2e-tests/plugins/interactive-blocks/namespace/view.js b/packages/e2e-tests/plugins/interactive-blocks/namespace/view.js index 354745499fff4..4fe81c4a3b530 100644 --- a/packages/e2e-tests/plugins/interactive-blocks/namespace/view.js +++ b/packages/e2e-tests/plugins/interactive-blocks/namespace/view.js @@ -39,3 +39,32 @@ store( '{}', { url: '/other-store-url', }, } ); + +store( 'true', { + state: { + url: '/some-url', + }, +} ); + +store( 'false', { + state: { + url: '/some-url', + }, +} ); + +store( '[]', { + state: { + url: '/some-url', + }, +} ); + +store( '"quoted string"', { + state: { + url: '/some-url', + }, +} ); + + + + + diff --git a/packages/interactivity/src/vdom.ts b/packages/interactivity/src/vdom.ts index 0914b8918230e..249b670d34f07 100644 --- a/packages/interactivity/src/vdom.ts +++ b/packages/interactivity/src/vdom.ts @@ -102,7 +102,7 @@ export function toVdom( root: Node ): Array< ComponentChild > { try { const parsedValue = JSON.parse( value ); value = isObject( parsedValue ) ? parsedValue : value; - } catch ( e ) {} + } catch {} if ( attributeName === islandAttr ) { island = true; const islandNamespace = diff --git a/test/e2e/specs/interactivity/namespace.spec.ts b/test/e2e/specs/interactivity/namespace.spec.ts index ebbaed7bb6338..68a36c1cd6be0 100644 --- a/test/e2e/specs/interactivity/namespace.spec.ts +++ b/test/e2e/specs/interactivity/namespace.spec.ts @@ -60,4 +60,28 @@ test.describe( 'Namespaces', () => { const el = page.getByTestId( 'null namespace' ); await expect( el ).toHaveAttribute( 'href', '/some-url' ); } ); + + test( 'A true as a string as namespace should work', async ( { page } ) => { + const el = page.getByTestId( 'true namespace' ); + await expect( el ).toHaveAttribute( 'href', '/some-url' ); + } ); + + test( 'A false as a string as namespace should work', async ( { + page, + } ) => { + const el = page.getByTestId( 'false namespace' ); + await expect( el ).toHaveAttribute( 'href', '/some-url' ); + } ); + + test( 'A [] as a string as namespace should work', async ( { page } ) => { + const el = page.getByTestId( '[] namespace' ); + await expect( el ).toHaveAttribute( 'href', '/some-url' ); + } ); + + test( 'A "quoted string" as a string as namespace should work', async ( { + page, + } ) => { + const el = page.getByTestId( 'quoted namespace' ); + await expect( el ).toHaveAttribute( 'href', '/some-url' ); + } ); } ); From 93137278649fd1aca95aab56cab11d4ef6d8b60e Mon Sep 17 00:00:00 2001 From: Carlos Bravo Date: Mon, 27 May 2024 10:32:58 +0200 Subject: [PATCH 3/6] Update changelog --- packages/interactivity/CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/interactivity/CHANGELOG.md b/packages/interactivity/CHANGELOG.md index e40c68a50180a..c3542b914de15 100644 --- a/packages/interactivity/CHANGELOG.md +++ b/packages/interactivity/CHANGELOG.md @@ -2,6 +2,10 @@ ## Unreleased +### Bug fixes + +- Fix null and number strings as namespaces runtime error. ([#61960](https://github.com/WordPress/gutenberg/pull/61960/)) + ### Breaking Changes - Variables like `process.env.IS_GUTENBERG_PLUGIN` have been replaced by `globalThis.IS_GUTENBERG_PLUGIN`. Build systems using `process.env` should be updated ([#61486](https://github.com/WordPress/gutenberg/pull/61486)). From afb5f805adaf40b399c9ba0b66248ee6eb7598c3 Mon Sep 17 00:00:00 2001 From: Carlos Bravo Date: Mon, 27 May 2024 11:22:39 +0200 Subject: [PATCH 4/6] added wrong css changes --- .../block-editor/src/components/inserter/style.scss | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/packages/block-editor/src/components/inserter/style.scss b/packages/block-editor/src/components/inserter/style.scss index 1bca09ec92bc1..604268fb1fe43 100644 --- a/packages/block-editor/src/components/inserter/style.scss +++ b/packages/block-editor/src/components/inserter/style.scss @@ -116,7 +116,18 @@ $block-inserter-tabs-height: 44px; overflow: hidden; .block-editor-inserter__tablist { - margin-bottom: -$border-width; + width: 100%; + + button[role="tab"] { + flex-grow: 1; + margin-bottom: -$border-width; + &[id$="reusable"] { + flex-grow: inherit; + // These are to align the `reusable` icon with the search icon. + padding-left: $grid-unit-20; + padding-right: $grid-unit-20; + } + } } .block-editor-inserter__tabpanel { From 430942872a78a7b47ff9525699fdabf8845da454 Mon Sep 17 00:00:00 2001 From: Carlos Bravo Date: Mon, 27 May 2024 13:07:24 +0200 Subject: [PATCH 5/6] Revert move isObject to utils --- packages/interactivity/src/store.ts | 3 ++- packages/interactivity/src/utils.ts | 3 --- packages/interactivity/src/vdom.ts | 4 +++- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/interactivity/src/store.ts b/packages/interactivity/src/store.ts index 46aa3c62d26c8..9b35192fe8b13 100644 --- a/packages/interactivity/src/store.ts +++ b/packages/interactivity/src/store.ts @@ -15,7 +15,8 @@ import { setNamespace, resetNamespace, } from './hooks'; -import { isObject } from './utils'; +const isObject = ( item: unknown ): item is Record< string, unknown > => + Boolean( item && typeof item === 'object' && item.constructor === Object ); const deepMerge = ( target: any, source: any ) => { if ( isObject( target ) && isObject( source ) ) { diff --git a/packages/interactivity/src/utils.ts b/packages/interactivity/src/utils.ts index 273174d586eb3..e709ba1ce3b0e 100644 --- a/packages/interactivity/src/utils.ts +++ b/packages/interactivity/src/utils.ts @@ -334,6 +334,3 @@ export const warn = ( message: string ): void => { logged.add( message ); } }; - -export const isObject = ( item: unknown ): item is Record< string, unknown > => - Boolean( item && typeof item === 'object' && item.constructor === Object ); diff --git a/packages/interactivity/src/vdom.ts b/packages/interactivity/src/vdom.ts index 249b670d34f07..98deca656cfa6 100644 --- a/packages/interactivity/src/vdom.ts +++ b/packages/interactivity/src/vdom.ts @@ -6,13 +6,15 @@ import { h, type ComponentChild, type JSX } from 'preact'; * Internal dependencies */ import { directivePrefix as p } from './constants'; -import { warn, isObject } from './utils'; +import { warn } from './utils'; const ignoreAttr = `data-${ p }-ignore`; const islandAttr = `data-${ p }-interactive`; const fullPrefix = `data-${ p }-`; const namespaces: Array< string | null > = []; const currentNamespace = () => namespaces[ namespaces.length - 1 ] ?? null; +const isObject = ( item: unknown ): item is Record< string, unknown > => + Boolean( item && typeof item === 'object' && item.constructor === Object ); // Regular expression for directive parsing. const directiveParser = new RegExp( From 64e275d4fdd6ff47e30a45e5933a0dba5a92f892 Mon Sep 17 00:00:00 2001 From: Carlos Bravo Date: Mon, 27 May 2024 14:27:10 +0200 Subject: [PATCH 6/6] Rename url vars --- .../interactive-blocks/namespace/view.js | 18 +++++++-------- .../e2e/specs/interactivity/namespace.spec.ts | 22 +++++++++---------- 2 files changed, 20 insertions(+), 20 deletions(-) diff --git a/packages/e2e-tests/plugins/interactive-blocks/namespace/view.js b/packages/e2e-tests/plugins/interactive-blocks/namespace/view.js index 4fe81c4a3b530..5717387395ff2 100644 --- a/packages/e2e-tests/plugins/interactive-blocks/namespace/view.js +++ b/packages/e2e-tests/plugins/interactive-blocks/namespace/view.js @@ -6,13 +6,13 @@ import { store } from '@wordpress/interactivity'; store( '', { state: { - url: '/some-url', + url: '/empty-string-url', }, } ); store( 'namespace', { state: { - url: '/some-url', + url: '/namespace-url', }, } ); @@ -24,43 +24,43 @@ store( 'other', { store( 'null', { state: { - url: '/some-url', + url: '/null-url', }, } ); store( '2', { state: { - url: '/some-url', + url: '/number-url', }, } ); store( '{}', { state: { - url: '/other-store-url', + url: '/object-url', }, } ); store( 'true', { state: { - url: '/some-url', + url: '/true-url', }, } ); store( 'false', { state: { - url: '/some-url', + url: '/false-url', }, } ); store( '[]', { state: { - url: '/some-url', + url: '/array-url', }, } ); store( '"quoted string"', { state: { - url: '/some-url', + url: '/quoted-url', }, } ); diff --git a/test/e2e/specs/interactivity/namespace.spec.ts b/test/e2e/specs/interactivity/namespace.spec.ts index 68a36c1cd6be0..9d0edc997886c 100644 --- a/test/e2e/specs/interactivity/namespace.spec.ts +++ b/test/e2e/specs/interactivity/namespace.spec.ts @@ -20,28 +20,28 @@ test.describe( 'Namespaces', () => { test( 'Empty string as namespace should not work', async ( { page } ) => { const el = page.getByTestId( 'empty namespace' ); - await expect( el ).not.toHaveAttribute( 'href', '/some-url' ); + await expect( el ).not.toHaveAttribute( 'href', '/empty-string-url' ); } ); test( 'A string as namespace should work', async ( { page } ) => { const el = page.getByTestId( 'correct namespace' ); - await expect( el ).toHaveAttribute( 'href', '/some-url' ); + await expect( el ).toHaveAttribute( 'href', '/namespace-url' ); } ); test( 'An empty object as namespace should not work', async ( { page, } ) => { const el = page.getByTestId( 'object namespace' ); - await expect( el ).not.toHaveAttribute( 'href', '/some-url' ); + await expect( el ).not.toHaveAttribute( 'href', '/object-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' ); + await expect( el ).not.toHaveAttribute( 'href', '/namespace-url' ); const correct = page.getByTestId( 'correct namespace' ); - await expect( correct ).toHaveAttribute( 'href', '/some-url' ); + await expect( correct ).toHaveAttribute( 'href', '/namespace-url' ); } ); test( 'A different store namespace should work', async ( { page } ) => { @@ -53,35 +53,35 @@ test.describe( 'Namespaces', () => { page, } ) => { const el = page.getByTestId( 'number namespace' ); - await expect( el ).toHaveAttribute( 'href', '/some-url' ); + await expect( el ).toHaveAttribute( 'href', '/number-url' ); } ); test( 'A null as a string as namespace should work', async ( { page } ) => { const el = page.getByTestId( 'null namespace' ); - await expect( el ).toHaveAttribute( 'href', '/some-url' ); + await expect( el ).toHaveAttribute( 'href', '/null-url' ); } ); test( 'A true as a string as namespace should work', async ( { page } ) => { const el = page.getByTestId( 'true namespace' ); - await expect( el ).toHaveAttribute( 'href', '/some-url' ); + await expect( el ).toHaveAttribute( 'href', '/true-url' ); } ); test( 'A false as a string as namespace should work', async ( { page, } ) => { const el = page.getByTestId( 'false namespace' ); - await expect( el ).toHaveAttribute( 'href', '/some-url' ); + await expect( el ).toHaveAttribute( 'href', '/false-url' ); } ); test( 'A [] as a string as namespace should work', async ( { page } ) => { const el = page.getByTestId( '[] namespace' ); - await expect( el ).toHaveAttribute( 'href', '/some-url' ); + await expect( el ).toHaveAttribute( 'href', '/array-url' ); } ); test( 'A "quoted string" as a string as namespace should work', async ( { page, } ) => { const el = page.getByTestId( 'quoted namespace' ); - await expect( el ).toHaveAttribute( 'href', '/some-url' ); + await expect( el ).toHaveAttribute( 'href', '/quoted-url' ); } ); } );