From 2458eb6c793c8945f6e150cafef4be6645484dc6 Mon Sep 17 00:00:00 2001 From: David Arenas Date: Thu, 22 Feb 2024 11:42:45 +0100 Subject: [PATCH] Interactivity API: Improve context merges using proxies (#59187) * First attempt using proxies * Simplify context logic * Return true from proxy `set` trap * Update wp-context and wp-each implementation * Rename `isObject` to `isPlainObject` * Refactor and document proxifyContext * Add test for new parent properties * Do not proxify assigned objects * Set the item after proxifying the context * Rename contextIgnores to contextAssignedObjects * Add comment to `udpateSignals` * Fix context values when navigating back and forward * Add more tests for navigations * Update tests to cover a tricky case * Update comment * Fix context definition in navigation block * Add test for object overwritten * Update changelog Co-authored-by: DAreRodz Co-authored-by: c4rl0sbr4v0 --- .../block-library/src/navigation/index.php | 8 +- .../directive-context/render.php | 27 +++ .../directive-context/view.js | 36 +++- packages/interactivity/CHANGELOG.md | 1 + packages/interactivity/src/directives.js | 158 ++++++++++++++---- .../interactivity/directive-context.spec.ts | 114 +++++++++++++ 6 files changed, 302 insertions(+), 42 deletions(-) diff --git a/packages/block-library/src/navigation/index.php b/packages/block-library/src/navigation/index.php index 5dcada62f6feb5..e45591661abfe4 100644 --- a/packages/block-library/src/navigation/index.php +++ b/packages/block-library/src/navigation/index.php @@ -554,7 +554,11 @@ private static function get_nav_element_directives( $is_interactive ) { // When adding to this array be mindful of security concerns. $nav_element_context = data_wp_context( array( - 'overlayOpenedBy' => array(), + 'overlayOpenedBy' => array( + 'click' => false, + 'hover' => false, + 'focus' => false, + ), 'type' => 'overlay', 'roleAttribute' => '', 'ariaLabel' => __( 'Menu' ), @@ -764,7 +768,7 @@ function block_core_navigation_add_directives_to_submenu( $tags, $block_attribut ) ) { // Add directives to the parent `
  • `. $tags->set_attribute( 'data-wp-interactive', 'core/navigation' ); - $tags->set_attribute( 'data-wp-context', '{ "submenuOpenedBy": {}, "type": "submenu" }' ); + $tags->set_attribute( 'data-wp-context', '{ "submenuOpenedBy": { "click": false, "hover": false, "focus": false }, "type": "submenu" }' ); $tags->set_attribute( 'data-wp-watch', 'callbacks.initMenu' ); $tags->set_attribute( 'data-wp-on--focusout', 'actions.handleMenuFocusout' ); $tags->set_attribute( 'data-wp-on--keydown', 'actions.handleMenuKeydown' ); diff --git a/packages/e2e-tests/plugins/interactive-blocks/directive-context/render.php b/packages/e2e-tests/plugins/interactive-blocks/directive-context/render.php index 428d47ec397957..25d2bbc692efdf 100644 --- a/packages/e2e-tests/plugins/interactive-blocks/directive-context/render.php +++ b/packages/e2e-tests/plugins/interactive-blocks/directive-context/render.php @@ -18,6 +18,7 @@ > + +
    @@ -59,6 +68,7 @@ > + +
    @@ -143,3 +158,15 @@ + +
    + + +
    +
    diff --git a/packages/e2e-tests/plugins/interactive-blocks/directive-context/view.js b/packages/e2e-tests/plugins/interactive-blocks/directive-context/view.js index aed4a3fefed07d..2267868713c41b 100644 --- a/packages/e2e-tests/plugins/interactive-blocks/directive-context/view.js +++ b/packages/e2e-tests/plugins/interactive-blocks/directive-context/view.js @@ -9,6 +9,10 @@ store( 'directive-context', { const ctx = getContext(); return JSON.stringify( ctx, undefined, 2 ); }, + get selected() { + const { list, selected } = getContext(); + return list.find( ( obj ) => obj === selected )?.text; + } }, actions: { updateContext( event ) { @@ -22,6 +26,15 @@ store( 'directive-context', { const ctx = getContext(); ctx.text = ctx.text === 'Text 1' ? 'Text 2' : 'Text 1'; }, + selectItem( event ) { + const ctx = getContext(); + const value = parseInt( event.target.value ); + ctx.selected = ctx.list.find( ( { id } ) => id === value ); + }, + replaceObj() { + const ctx = getContext(); + ctx.obj = { overwritten: true }; + } }, } ); @@ -29,12 +42,17 @@ const html = `
    +
    +
    +
    +
    - + +
    `; @@ -49,13 +67,17 @@ const { actions } = store( 'directive-context-navigate', { const ctx = getContext(); ctx.newText = 'some new text'; }, + addText2() { + const ctx = getContext(); + ctx.text2 = 'some new text'; + }, navigate() { return import( '@wordpress/interactivity-router' ).then( - ( { actions: routerActions } ) => - routerActions.navigate( - window.location, - { force: true, html }, - ) + ( { actions: routerActions } ) => { + const url = new URL( window.location.href ); + url.searchParams.set( 'next_page', 'true' ); + return routerActions.navigate( url, { force: true, html } ); + } ); }, diff --git a/packages/interactivity/CHANGELOG.md b/packages/interactivity/CHANGELOG.md index 88b59a0424dc63..8e48ead8429d3b 100644 --- a/packages/interactivity/CHANGELOG.md +++ b/packages/interactivity/CHANGELOG.md @@ -7,6 +7,7 @@ ### 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)) ## 5.0.0 (2024-02-09) diff --git a/packages/interactivity/src/directives.js b/packages/interactivity/src/directives.js index 9184fb1d6d8035..67ad50a4595807 100644 --- a/packages/interactivity/src/directives.js +++ b/packages/interactivity/src/directives.js @@ -14,23 +14,118 @@ import { useWatch, useInit } from './utils'; import { directive, getScope, getEvaluate } from './hooks'; import { kebabToCamelCase } from './utils/kebab-to-camelcase'; -const isObject = ( item ) => - item && typeof item === 'object' && ! Array.isArray( item ); +// Assigned objects should be ignore during proxification. +const contextAssignedObjects = new WeakMap(); -const mergeDeepSignals = ( target, source, overwrite ) => { +const isPlainObject = ( item ) => + item && typeof item === 'object' && item.constructor === Object; + +const descriptor = Reflect.getOwnPropertyDescriptor; + +/** + * Wrap a context object with a proxy to reproduce the context stack. The proxy + * uses the passed `inherited` context as a fallback to look up for properties + * that don't exist in the given context. Also, updated properties are modified + * where they are defined, or added to the main context when they don't exist. + * + * By default, all plain objects inside the context are wrapped, unless it is + * listed in the `ignore` option. + * + * @param {Object} current Current context. + * @param {Object} inherited Inherited context, used as fallback. + * + * @return {Object} The wrapped context object. + */ +const proxifyContext = ( current, inherited = {} ) => + new Proxy( current, { + get: ( target, k ) => { + // Subscribe to the inherited and current props. + const inheritedProp = inherited[ k ]; + const currentProp = target[ k ]; + + // Return the inherited prop when missing in target. + if ( ! ( k in target ) && k in inherited ) { + return inheritedProp; + } + + // Proxify plain objects that are not listed in `ignore`. + if ( + k in target && + ! contextAssignedObjects.get( target )?.has( k ) && + isPlainObject( peek( target, k ) ) + ) { + return proxifyContext( currentProp, inheritedProp ); + } + + // For other cases, return the value from target. + return currentProp; + }, + set: ( target, k, value ) => { + const obj = + k in target || ! ( k in inherited ) ? target : inherited; + + // Values that are objects should not be proxified so they point to + // the original object and don't inherit unexpected properties. + if ( value && typeof value === 'object' ) { + if ( ! contextAssignedObjects.has( obj ) ) { + contextAssignedObjects.set( obj, new Set() ); + } + contextAssignedObjects.get( obj ).add( k ); + } + + obj[ k ] = value; + return true; + }, + ownKeys: ( target ) => [ + ...new Set( [ + ...Object.keys( inherited ), + ...Object.keys( target ), + ] ), + ], + getOwnPropertyDescriptor: ( target, k ) => + descriptor( target, k ) || descriptor( inherited, k ), + } ); + +/** + * Recursively update values within a deepSignal object. + * + * @param {Object} target A deepSignal instance. + * @param {Object} source Object with properties to update in `target` + */ +const updateSignals = ( target, source ) => { for ( const k in source ) { - if ( isObject( peek( target, k ) ) && isObject( peek( source, k ) ) ) { - mergeDeepSignals( - target[ `$${ k }` ].peek(), - source[ `$${ k }` ].peek(), - overwrite - ); - } else if ( overwrite || typeof peek( target, k ) === 'undefined' ) { - target[ `$${ k }` ] = source[ `$${ k }` ]; + if ( + isPlainObject( peek( target, k ) ) && + isPlainObject( peek( source, k ) ) + ) { + updateSignals( target[ `$${ k }` ].peek(), source[ k ] ); + } else { + target[ k ] = source[ k ]; } } }; +/** + * Recursively clone the passed object. + * + * @param {Object} source Source object. + * @return {Object} Cloned object. + */ +const deepClone = ( source ) => { + if ( isPlainObject( source ) ) { + return Object.fromEntries( + Object.entries( source ).map( ( [ key, value ] ) => [ + key, + deepClone( value ), + ] ) + ); + } + if ( Array.isArray( source ) ) { + return source.map( ( i ) => deepClone( i ) ); + } + return source; +}; + const newRule = /(?:([\u0080-\uFFFF\w-%@]+) *:? *([^{;]+?);|([^;}{]*?) *{)|(}\s*)/g; const ruleClean = /\/\*[^]*?\*\/| +/g; @@ -105,22 +200,18 @@ export default () => { ( { suffix } ) => suffix === 'default' ); - currentValue.current = useMemo( () => { - if ( ! defaultEntry ) return null; - const { namespace, value } = defaultEntry; - const newValue = deepSignal( { [ namespace ]: value } ); - mergeDeepSignals( newValue, inheritedValue ); - mergeDeepSignals( currentValue.current, newValue, true ); - return currentValue.current; - }, [ inheritedValue, defaultEntry ] ); + // No change should be made if `defaultEntry` does not exist. + const contextStack = useMemo( () => { + if ( defaultEntry ) { + const { namespace, value } = defaultEntry; + updateSignals( currentValue.current, { + [ namespace ]: deepClone( value ), + } ); + } + return proxifyContext( currentValue.current, inheritedValue ); + }, [ defaultEntry, inheritedValue ] ); - if ( currentValue.current ) { - return ( - - { children } - - ); - } + return { children }; }, { priority: 5 } ); @@ -358,15 +449,16 @@ export default () => { const list = evaluate( entry ); return list.map( ( item ) => { - const mergedContext = deepSignal( {} ); - const itemProp = suffix === 'default' ? 'item' : kebabToCamelCase( suffix ); - const newValue = deepSignal( { - [ namespace ]: { [ itemProp ]: item }, - } ); - mergeDeepSignals( newValue, inheritedValue ); - mergeDeepSignals( mergedContext, newValue, true ); + const itemContext = deepSignal( { [ namespace ]: {} } ); + const mergedContext = proxifyContext( + itemContext, + inheritedValue + ); + + // Set the item after proxifying the context. + mergedContext[ namespace ][ itemProp ] = item; const scope = { ...getScope(), context: mergedContext }; const key = eachKey diff --git a/test/e2e/specs/interactivity/directive-context.spec.ts b/test/e2e/specs/interactivity/directive-context.spec.ts index 95300dc53bf864..3a566e7c2d9619 100644 --- a/test/e2e/specs/interactivity/directive-context.spec.ts +++ b/test/e2e/specs/interactivity/directive-context.spec.ts @@ -136,6 +136,27 @@ test.describe( 'data-wp-context', () => { expect( parentContext.obj.prop5 ).toBe( 'modifiedFromParent' ); } ); + test( 'new inherited properties update child contexts', async ( { + page, + } ) => { + const childContextBefore = await parseContent( + page.getByTestId( 'child context' ) + ); + expect( childContextBefore.new ).toBeUndefined(); + + await page.getByTestId( 'parent new' ).click(); + + const childContextAfter = await parseContent( + page.getByTestId( 'child context' ) + ); + expect( childContextAfter.new ).toBe( 'modifiedFromParent' ); + + const parentContext = await parseContent( + page.getByTestId( 'parent context' ) + ); + expect( parentContext.new ).toBe( 'modifiedFromParent' ); + } ); + test( 'Array properties are shadowed', async ( { page } ) => { const parentContext = await parseContent( page.getByTestId( 'parent context' ) @@ -149,6 +170,52 @@ test.describe( 'data-wp-context', () => { expect( childContext.array ).toMatchObject( [ 4, 5, 6 ] ); } ); + test( 'overwritten objects updates inherited values', async ( { + page, + } ) => { + await page.getByTestId( 'parent replace' ).click(); + + const childContext = await parseContent( + page.getByTestId( 'child context' ) + ); + + expect( childContext.obj.prop4 ).toBeUndefined(); + expect( childContext.obj.prop5 ).toBe( 'child' ); + expect( childContext.obj.prop6 ).toBe( 'child' ); + expect( childContext.obj.overwritten ).toBe( true ); + + const parentContext = await parseContent( + page.getByTestId( 'parent context' ) + ); + + expect( parentContext.obj.prop4 ).toBeUndefined(); + expect( parentContext.obj.prop5 ).toBeUndefined(); + expect( parentContext.obj.prop6 ).toBeUndefined(); + expect( parentContext.obj.overwritten ).toBe( true ); + } ); + + test( 'overwritten objects do not inherit values', async ( { page } ) => { + await page.getByTestId( 'child replace' ).click(); + + const childContext = await parseContent( + page.getByTestId( 'child context' ) + ); + + expect( childContext.obj.prop4 ).toBeUndefined(); + expect( childContext.obj.prop5 ).toBeUndefined(); + expect( childContext.obj.prop6 ).toBeUndefined(); + expect( childContext.obj.overwritten ).toBe( true ); + + const parentContext = await parseContent( + page.getByTestId( 'parent context' ) + ); + + expect( parentContext.obj.prop4 ).toBe( 'parent' ); + expect( parentContext.obj.prop5 ).toBe( 'parent' ); + expect( parentContext.obj.prop6 ).toBeUndefined(); + expect( parentContext.obj.overwritten ).toBeUndefined(); + } ); + test( 'can be accessed in other directives on the same element', async ( { page, } ) => { @@ -181,6 +248,39 @@ test.describe( 'data-wp-context', () => { await expect( element ).toHaveText( 'some new text' ); } ); + test( 'should update values when navigating back or forward', async ( { + page, + } ) => { + const element = page.getByTestId( 'navigation text' ); + await page.getByTestId( 'navigate' ).click(); + await expect( element ).toHaveText( 'second page' ); + await page.goBack(); + await expect( element ).toHaveText( 'first page' ); + await page.goForward(); + await expect( element ).toHaveText( 'second page' ); + } ); + + test( 'should inherit values on navigation', async ( { page } ) => { + const text = page.getByTestId( 'navigation inherited text' ); + const text2 = page.getByTestId( 'navigation inherited text2' ); + await expect( text ).toHaveText( 'first page' ); + await expect( text2 ).toBeEmpty(); + await page.getByTestId( 'toggle text' ).click(); + await expect( text ).toHaveText( 'changed dynamically' ); + await page.getByTestId( 'add text2' ).click(); + await expect( text2 ).toHaveText( 'some new text' ); + await page.getByTestId( 'navigate' ).click(); + await expect( text ).toHaveText( 'second page' ); + await expect( text2 ).toHaveText( 'second page' ); + await page.goBack(); + await expect( text ).toHaveText( 'first page' ); + // text2 maintains its value as it is not defined in the first page. + await expect( text2 ).toHaveText( 'second page' ); + await page.goForward(); + await expect( text ).toHaveText( 'second page' ); + await expect( text2 ).toHaveText( 'second page' ); + } ); + test( 'should maintain the same context reference on async actions', async ( { page, } ) => { @@ -199,4 +299,18 @@ test.describe( 'data-wp-context', () => { const element = page.getByTestId( 'non-default suffix context' ); await expect( element ).toHaveText( '' ); } ); + + test( 'references to objects are kept', async ( { page } ) => { + const selected = page.getByTestId( 'selected' ); + const select1 = page.getByTestId( 'select 1' ); + const select2 = page.getByTestId( 'select 2' ); + + await expect( selected ).toBeEmpty(); + + await select1.click(); + await expect( selected ).toHaveText( 'Text 1' ); + + await select2.click(); + await expect( selected ).toHaveText( 'Text 2' ); + } ); } );