From 3d087020e0890f92202b71f3237a294148d1693a Mon Sep 17 00:00:00 2001 From: David Arenas Date: Mon, 26 Feb 2024 17:07:35 +0100 Subject: [PATCH] Interactivity API: Fix `navigate()` issues related to initial state merges (#57134) * Use deepMerge and stores to update state * Add test case * Do not render state when `data` is not present * Move batch call inside braces * Move initial data related code together * Fix initial data tag ID * Prevent passing the state proxy as receiver * Add comment * Move links to a nav element to prevent hydration issues * Add failing test for getters merge * Simplify assignments * Add a try-catch to ignore getter assignments * Fix disabled navigation test * Test navigations to pages with csn disabled * Update changelogs Co-authored-by: DAreRodz Co-authored-by: luisherranz # Conflicts: # packages/interactivity-router/CHANGELOG.md # packages/interactivity/CHANGELOG.md --- .../router-navigate/render.php | 57 +++++--- .../router-navigate/view.js | 17 ++- packages/interactivity-router/CHANGELOG.md | 6 + packages/interactivity-router/src/index.js | 44 ++++-- packages/interactivity/CHANGELOG.md | 6 + packages/interactivity/src/index.ts | 5 + packages/interactivity/src/store.ts | 72 ++++++---- .../interactivity/router-navigate.spec.ts | 125 ++++++++++++++++-- 8 files changed, 255 insertions(+), 77 deletions(-) diff --git a/packages/e2e-tests/plugins/interactive-blocks/router-navigate/render.php b/packages/e2e-tests/plugins/interactive-blocks/router-navigate/render.php index 0b8e6e1012d1a..d1a7aa9211f10 100644 --- a/packages/e2e-tests/plugins/interactive-blocks/router-navigate/render.php +++ b/packages/e2e-tests/plugins/interactive-blocks/router-navigate/render.php @@ -15,6 +15,13 @@ array( 'clientNavigationDisabled' => true ) ); } + +if ( isset( $attributes['data'] ) ) { + wp_interactivity_state( + 'router', + array( 'data' => $attributes['data'] ) + ); +} ?>
NaN + NaN NaN - $link ) { - $i = $key += 1; - echo <<link $i - link $i with hash + +
+
+
+
diff --git a/packages/e2e-tests/plugins/interactive-blocks/router-navigate/view.js b/packages/e2e-tests/plugins/interactive-blocks/router-navigate/view.js index 1e137969936a0..b2d4ad0dc1dde 100644 --- a/packages/e2e-tests/plugins/interactive-blocks/router-navigate/view.js +++ b/packages/e2e-tests/plugins/interactive-blocks/router-navigate/view.js @@ -6,14 +6,23 @@ import { store } from '@wordpress/interactivity'; const { state } = store( 'router', { state: { status: 'idle', - navigations: 0, + navigations: { + pending: 0, + count: 0, + }, timeout: 10000, + data: { + get getterProp() { + return `value from getter (${ state.data.prop1 })`; + } + } }, actions: { *navigate( e ) { e.preventDefault(); - state.navigations += 1; + state.navigations.count += 1; + state.navigations.pending += 1; state.status = 'busy'; const force = e.target.dataset.forceNavigation === 'true'; @@ -24,9 +33,9 @@ const { state } = store( 'router', { ); yield actions.navigate( e.target.href, { force, timeout } ); - state.navigations -= 1; + state.navigations.pending -= 1; - if ( state.navigations === 0 ) { + if ( state.navigations.pending === 0 ) { state.status = 'idle'; } }, diff --git a/packages/interactivity-router/CHANGELOG.md b/packages/interactivity-router/CHANGELOG.md index b1b7afcea1355..799425e4cd9d5 100644 --- a/packages/interactivity-router/CHANGELOG.md +++ b/packages/interactivity-router/CHANGELOG.md @@ -2,6 +2,12 @@ ## Unreleased +### Bug Fixes + +- Fix navigate() issues related to initial state merges. ([#57134](https://github.com/WordPress/gutenberg/pull/57134)) + +## 1.2.0 (2024-02-21) + ## 1.1.0 (2024-02-09) ### New Features diff --git a/packages/interactivity-router/src/index.js b/packages/interactivity-router/src/index.js index 724a2660df41d..03d399338167c 100644 --- a/packages/interactivity-router/src/index.js +++ b/packages/interactivity-router/src/index.js @@ -3,10 +3,18 @@ */ import { store, privateApis, getConfig } from '@wordpress/interactivity'; -const { directivePrefix, getRegionRootFragment, initialVdom, toVdom, render } = - privateApis( - 'I acknowledge that using private APIs means my theme or plugin will inevitably break in the next version of WordPress.' - ); +const { + directivePrefix, + getRegionRootFragment, + initialVdom, + toVdom, + render, + parseInitialData, + populateInitialData, + batch, +} = privateApis( + 'I acknowledge that using private APIs means my theme or plugin will inevitably break in the next version of WordPress.' +); // The cache of visited and prefetched pages. const pages = new Map(); @@ -45,20 +53,24 @@ const regionsToVdom = ( dom, { vdom } = {} ) => { : toVdom( region ); } ); const title = dom.querySelector( 'title' )?.innerText; - return { regions, title }; + const initialData = parseInitialData( dom ); + return { regions, title, initialData }; }; // Render all interactive regions contained in the given page. const renderRegions = ( page ) => { - const attrName = `data-${ directivePrefix }-router-region`; - document.querySelectorAll( `[${ attrName }]` ).forEach( ( region ) => { - const id = region.getAttribute( attrName ); - const fragment = getRegionRootFragment( region ); - render( page.regions[ id ], fragment ); + batch( () => { + populateInitialData( page.initialData ); + const attrName = `data-${ directivePrefix }-router-region`; + document.querySelectorAll( `[${ attrName }]` ).forEach( ( region ) => { + const id = region.getAttribute( attrName ); + const fragment = getRegionRootFragment( region ); + render( page.regions[ id ], fragment ); + } ); + if ( page.title ) { + document.title = page.title; + } } ); - if ( page.title ) { - document.title = page.title; - } }; /** @@ -176,7 +188,11 @@ export const { state, actions } = store( 'core/router', { // out, and let the newer execution to update the HTML. if ( navigatingTo !== href ) return; - if ( page ) { + if ( + page && + ! page.initialData?.config?.[ 'core/router' ] + ?.clientNavigationDisabled + ) { renderRegions( page ); window.history[ options.replace ? 'replaceState' : 'pushState' diff --git a/packages/interactivity/CHANGELOG.md b/packages/interactivity/CHANGELOG.md index a707bc3b57d6f..1e81760b8d05c 100644 --- a/packages/interactivity/CHANGELOG.md +++ b/packages/interactivity/CHANGELOG.md @@ -4,6 +4,12 @@ ### Bug Fixes +- Prevent passing state proxies as receivers to deepSignal proxy handlers. ([#57134](https://github.com/WordPress/gutenberg/pull/57134)) + +## 5.1.0 (2024-02-21) + +### Bug Fixes + - Only add proxies to plain objects inside the store. ([#59039](https://github.com/WordPress/gutenberg/pull/59039)) - Improve context merges using proxies. ([59187](https://github.com/WordPress/gutenberg/pull/59187)) diff --git a/packages/interactivity/src/index.ts b/packages/interactivity/src/index.ts index 2a98900dfe379..3c91e919d91bd 100644 --- a/packages/interactivity/src/index.ts +++ b/packages/interactivity/src/index.ts @@ -2,6 +2,7 @@ * External dependencies */ import { h, cloneElement, render } from 'preact'; +import { batch } from '@preact/signals'; import { deepSignal } from 'deepsignal'; /** @@ -12,6 +13,7 @@ import { init, getRegionRootFragment, initialVdom } from './init'; import { directivePrefix } from './constants'; import { toVdom } from './vdom'; import { directive, getNamespace } from './hooks'; +import { parseInitialData, populateInitialData } from './store'; export { store, getConfig } from './store'; export { getContext, getElement } from './hooks'; @@ -43,6 +45,9 @@ export const privateApis = ( lock ): any => { cloneElement, render, deepSignal, + parseInitialData, + populateInitialData, + batch, }; } diff --git a/packages/interactivity/src/store.ts b/packages/interactivity/src/store.ts index 09341b638094a..d160e2fd1c71f 100644 --- a/packages/interactivity/src/store.ts +++ b/packages/interactivity/src/store.ts @@ -26,29 +26,20 @@ const deepMerge = ( target: any, source: any ) => { if ( typeof getter === 'function' ) { Object.defineProperty( target, key, { get: getter } ); } else if ( isObject( source[ key ] ) ) { - if ( ! target[ key ] ) Object.assign( target, { [ key ]: {} } ); + if ( ! target[ key ] ) target[ key ] = {}; deepMerge( target[ key ], source[ key ] ); } else { - Object.assign( target, { [ key ]: source[ key ] } ); + try { + target[ key ] = source[ key ]; + } catch ( e ) { + // Assignemnts fail for properties that are only getters. + // When that's the case, the assignment is simply ignored. + } } } } }; -const parseInitialData = () => { - const storeTag = document.querySelector( - `script[type="application/json"]#wp-interactivity-data` - ); - if ( storeTag?.textContent ) { - try { - return JSON.parse( storeTag.textContent ); - } catch ( e ) { - // Do nothing. - } - } - return {}; -}; - export const stores = new Map(); const rawStores = new Map(); const storeLocks = new Map(); @@ -100,13 +91,13 @@ const handlers = { } } - const result = Reflect.get( target, key, receiver ); + const result = Reflect.get( target, key ); // Check if the proxy is the store root and no key with that name exist. In // that case, return an empty object for the requested key. if ( typeof result === 'undefined' && receiver === stores.get( ns ) ) { const obj = {}; - Reflect.set( target, key, obj, receiver ); + Reflect.set( target, key, obj ); return proxify( obj, ns ); } @@ -163,6 +154,10 @@ const handlers = { return result; }, + // Prevents passing the current proxy as the receiver to the deepSignal. + set( target: any, key: string, value: any ) { + return Reflect.set( target, key, value ); + }, }; /** @@ -310,15 +305,36 @@ export function store( return stores.get( namespace ); } +export const parseInitialData = ( dom = document ) => { + const storeTag = dom.querySelector( + `script[type="application/json"]#wp-interactivity-data` + ); + if ( storeTag?.textContent ) { + try { + return JSON.parse( storeTag.textContent ); + } catch ( e ) { + // Do nothing. + } + } + return {}; +}; + +export const populateInitialData = ( data?: { + state?: Record< string, unknown >; + config?: Record< string, unknown >; +} ) => { + if ( isObject( data?.state ) ) { + Object.entries( data.state ).forEach( ( [ namespace, state ] ) => { + store( namespace, { state }, { lock: universalUnlock } ); + } ); + } + if ( isObject( data?.config ) ) { + Object.entries( data.config ).forEach( ( [ namespace, config ] ) => { + storeConfigs.set( namespace, config ); + } ); + } +}; + // Parse and populate the initial state and config. const data = parseInitialData(); -if ( isObject( data?.state ) ) { - Object.entries( data.state ).forEach( ( [ namespace, state ] ) => { - store( namespace, { state }, { lock: universalUnlock } ); - } ); -} -if ( isObject( data?.config ) ) { - Object.entries( data.config ).forEach( ( [ namespace, config ] ) => { - storeConfigs.set( namespace, config ); - } ); -} +populateInitialData( data ); diff --git a/test/e2e/specs/interactivity/router-navigate.spec.ts b/test/e2e/specs/interactivity/router-navigate.spec.ts index fafa31341f463..872fe9ea7ea52 100644 --- a/test/e2e/specs/interactivity/router-navigate.spec.ts +++ b/test/e2e/specs/interactivity/router-navigate.spec.ts @@ -12,18 +12,37 @@ test.describe( 'Router navigate', () => { } ); const link1 = await utils.addPostWithBlock( 'test/router-navigate', { alias: 'router navigate - link 1', - attributes: { title: 'Link 1' }, - } ); - await utils.addPostWithBlock( 'test/router-navigate', { - alias: 'router navigate - main', - attributes: { title: 'Main', links: [ link1, link2 ] }, + attributes: { + title: 'Link 1', + data: { + getterProp: 'value from link1', + prop1: 'link 1', + prop3: 'link 1', + }, + }, } ); - await utils.addPostWithBlock( 'test/router-navigate', { + const link3 = await utils.addPostWithBlock( 'test/router-navigate', { alias: 'router navigate - disabled', attributes: { title: 'Main (navigation disabled)', links: [ link1, link2 ], disableNavigation: true, + data: { + getterProp: 'value from main (navigation disabled)', + prop1: 'main (navigation disabled)', + }, + }, + } ); + await utils.addPostWithBlock( 'test/router-navigate', { + alias: 'router navigate - main', + attributes: { + title: 'Main', + links: [ link1, link2, link3 ], + data: { + getterProp: 'value from main', + prop1: 'main', + prop2: 'main', + }, }, } ); } ); @@ -44,7 +63,7 @@ test.describe( 'Router navigate', () => { const link1 = utils.getLink( 'router navigate - link 1' ); const link2 = utils.getLink( 'router navigate - link 2' ); - const navigations = page.getByTestId( 'router navigations' ); + const navigations = page.getByTestId( 'router navigations pending' ); const status = page.getByTestId( 'router status' ); const title = page.getByTestId( 'title' ); @@ -89,7 +108,7 @@ test.describe( 'Router navigate', () => { } ) => { const link1 = utils.getLink( 'router navigate - link 1' ); - const navigations = page.getByTestId( 'router navigations' ); + const navigations = page.getByTestId( 'router navigations pending' ); const status = page.getByTestId( 'router status' ); const title = page.getByTestId( 'title' ); @@ -175,12 +194,12 @@ test.describe( 'Router navigate', () => { } ) => { await page.goto( utils.getLink( 'router navigate - disabled' ) ); - const navigations = page.getByTestId( 'router navigations' ); + const count = page.getByTestId( 'router navigations count' ); const status = page.getByTestId( 'router status' ); const title = page.getByTestId( 'title' ); // Check some elements to ensure the page has hydrated. - await expect( navigations ).toHaveText( '0' ); + await expect( count ).toHaveText( '0' ); await expect( status ).toHaveText( 'idle' ); await page.getByTestId( 'link 1' ).click(); @@ -189,6 +208,90 @@ test.describe( 'Router navigate', () => { await expect( title ).toHaveText( 'Link 1' ); // Check that client-navigations count has not increased. - await expect( navigations ).toHaveText( '0' ); + await expect( count ).toHaveText( '0' ); + } ); + + test( 'should overwrite the state with the one serialized in the new page', async ( { + page, + } ) => { + const prop1 = page.getByTestId( 'prop1' ); + const prop2 = page.getByTestId( 'prop2' ); + const prop3 = page.getByTestId( 'prop3' ); + + await expect( prop1 ).toHaveText( 'main' ); + await expect( prop2 ).toHaveText( 'main' ); + await expect( prop3 ).toBeEmpty(); + + await page.getByTestId( 'link 1' ).click(); + + // New values for existing properties should change. + // Old values not overwritten should remain the same. + // New properties should appear. + await expect( prop1 ).toHaveText( 'link 1' ); + await expect( prop2 ).toHaveText( 'main' ); + await expect( prop3 ).toHaveText( 'link 1' ); + + await page.goBack(); + + // New added properties are preserved. + await expect( prop1 ).toHaveText( 'main' ); + await expect( prop2 ).toHaveText( 'main' ); + await expect( prop3 ).toHaveText( 'link 1' ); + } ); + + test( 'should not try to overwrite getters with values from the initial data', async ( { + page, + } ) => { + const title = page.getByTestId( 'title' ); + const getter = page.getByTestId( 'getterProp' ); + + // Title should start in 'Main' and the getter prop should be the one + // returned once hydrated. + await expect( title ).toHaveText( 'Main' ); + await expect( getter ).toHaveText( 'value from getter (main)' ); + + await page.getByTestId( 'link 1' ).click(); + + // Title should have changed. If not, that means there was an error + // during render. The getter should return the correct value. + await expect( title ).toHaveText( 'Link 1' ); + await expect( getter ).toHaveText( 'value from getter (link 1)' ); + + // Same behavior navigating back and forward. + await page.goBack(); + await expect( title ).toHaveText( 'Main' ); + await expect( getter ).toHaveText( 'value from getter (main)' ); + + await page.goForward(); + await expect( title ).toHaveText( 'Link 1' ); + await expect( getter ).toHaveText( 'value from getter (link 1)' ); + } ); + + test( 'should force a page reload when navigating to a page with `clientNavigationDisabled`', async ( { + page, + } ) => { + const count = page.getByTestId( 'router navigations count' ); + const title = page.getByTestId( 'title' ); + + // Check the cound to ensure the page has hydrated. + await expect( count ).toHaveText( '0' ); + + // Navigate to a page without clientNavigationDisabled. + await page.getByTestId( 'link 1' ).click(); + + // Check the page has updated and the navigation count has increased. + await expect( title ).toHaveText( 'Link 1' ); + await expect( count ).toHaveText( '1' ); + + await page.goBack(); + await expect( title ).toHaveText( 'Main' ); + await expect( count ).toHaveText( '1' ); + + // Navigate to a page with clientNavigationDisabled. + await page.getByTestId( 'link 3' ).click(); + + // Check the page has updated and the navigation count is zero. + await expect( title ).toHaveText( 'Main (navigation disabled)' ); + await expect( count ).toHaveText( '0' ); } ); } );