Interactivity API: Fix navigate() issues related to initial state m…

…erges (#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 <darerodz@git.wordpress.org> Co-authored-by: luisherranz <luisherranz@git.wordpress.org>
WordPress · Feb 26, 2024 · 3c1d9d3 · 3c1d9d3
1 parent 7b73a72
commit 3c1d9d3
Show file tree

Hide file tree

Showing 8 changed files with 251 additions and 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
@@ -15,6 +15,13 @@
 		array( 'clientNavigationDisabled' => true )
 	);
 }
+
+if ( isset( $attributes['data'] ) ) {
+	wp_interactivity_state(
+		'router',
+		array( 'data' => $attributes['data'] )
+	);
+}
 ?>
 
 <div
@@ -24,8 +31,12 @@
 	<h2 data-testid="title"><?php echo $attributes['title']; ?></h2>
 
 	<output
-		data-testid="router navigations"
-		data-wp-text="state.navigations"
+		data-testid="router navigations pending"
+		data-wp-text="state.navigations.pending"
+	>NaN</output>
+	<output
+		data-testid="router navigations count"
+		data-wp-text="state.navigations.count"
 	>NaN</output>
 	<output
 		data-testid="router status"
@@ -39,24 +50,30 @@
 		Timeout <span data-wp-text="state.timeout">NaN</span>
 	</button>
 
-	<?php
-	if ( isset( $attributes['links'] ) ) {
-		foreach ( $attributes['links'] as $key => $link ) {
-			$i = $key += 1;
-			echo <<<HTML
-			<a
-				data-testid="link $i"
-				data-wp-on--click="actions.navigate"
-				href="$link"
-			>link $i</a>
-			<a
-				data-testid="link $i with hash"
-				data-wp-on--click="actions.navigate"
-				data-force-navigation="true"
-				href="$link#link-$i-with-hash"
-			>link $i with hash</a>
+	<nav>
+		<?php
+		if ( isset( $attributes['links'] ) ) {
+			foreach ( $attributes['links'] as $key => $link ) {
+				$i = $key += 1;
+				echo <<<HTML
+				<a
+					data-testid="link $i"
+					data-wp-on--click="actions.navigate"
+					href="$link"
+				>link $i</a>
+				<a
+					data-testid="link $i with hash"
+					data-wp-on--click="actions.navigate"
+					data-force-navigation="true"
+					href="$link#link-$i-with-hash"
+				>link $i with hash</a>
 HTML;
+			}
 		}
-	}
-	?>
+		?>
+	</nav>
+	<div data-testid="getterProp" data-wp-text="state.data.getterProp"></div>
+	<div data-testid="prop1" data-wp-text="state.data.prop1"></div>
+	<div data-testid="prop2" data-wp-text="state.data.prop2"></div>
+	<div data-testid="prop3" data-wp-text="state.data.prop3"></div>
 </div>
diff --git 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
@@ -2,6 +2,10 @@
 
 ## 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)

diff --git 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
@@ -2,6 +2,10 @@
 
 ## Unreleased
 
+### 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

diff --git 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
@@ -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 );