From d932de2b0689e8975a44023394bb2ba7569dbd5a Mon Sep 17 00:00:00 2001 From: David Arenas Date: Mon, 4 Mar 2024 15:49:16 +0100 Subject: [PATCH] Interactivity API: Fix context object proxy references (#59553) * Add failing test * Fix returned proxy reference * Add more reference tests * Fix assignment of context objects * Update comment * Update changelog Co-authored-by: DAreRodz Co-authored-by: c4rl0sbr4v0 --- .../directive-context/render.php | 18 +++ .../directive-context/view.js | 13 ++ packages/interactivity/CHANGELOG.md | 1 + packages/interactivity/src/directives.js | 130 +++++++++++------- .../interactivity/directive-context.spec.ts | 42 ++++++ 5 files changed, 156 insertions(+), 48 deletions(-) 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 6b20b2dba8376..c25748bc5dbe6 100644 --- a/packages/e2e-tests/plugins/interactive-blocks/directive-context/render.php +++ b/packages/e2e-tests/plugins/interactive-blocks/directive-context/render.php @@ -117,6 +117,24 @@ > obj.prop6 + +
+ Is proxy preserved? +
+
+ Is proxy preserved on copy? +

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 9437edced74a4..a6126973f9d5b 100644 --- a/packages/e2e-tests/plugins/interactive-blocks/directive-context/view.js +++ b/packages/e2e-tests/plugins/interactive-blocks/directive-context/view.js @@ -12,6 +12,15 @@ store( 'directive-context', { get selected() { const { list, selected } = getContext(); return list.find( ( obj ) => obj === selected )?.text; + }, + get isProxyPreserved() { + const ctx = getContext(); + const pointer = ctx.obj; + return pointer === ctx.obj; + }, + get isProxyPreservedOnCopy() { + const { obj, obj2 } = getContext(); + return obj === obj2; } }, actions: { @@ -34,6 +43,10 @@ store( 'directive-context', { replaceObj() { const ctx = getContext(); ctx.obj = { overwritten: true }; + }, + copyObj() { + const ctx = getContext(); + ctx.obj2 = ctx.obj; } }, } ); diff --git a/packages/interactivity/CHANGELOG.md b/packages/interactivity/CHANGELOG.md index 1e81760b8d05c..e54d34f518a58 100644 --- a/packages/interactivity/CHANGELOG.md +++ b/packages/interactivity/CHANGELOG.md @@ -5,6 +5,7 @@ ### Bug Fixes - Prevent passing state proxies as receivers to deepSignal proxy handlers. ([#57134](https://github.com/WordPress/gutenberg/pull/57134)) +- Keep the same references to objects defined inside the context. ([#59553](https://github.com/WordPress/gutenberg/pull/59553)) ## 5.1.0 (2024-02-21) diff --git a/packages/interactivity/src/directives.js b/packages/interactivity/src/directives.js index d04c1a1f74d3e..b19067bdc1ad9 100644 --- a/packages/interactivity/src/directives.js +++ b/packages/interactivity/src/directives.js @@ -17,6 +17,11 @@ import { kebabToCamelCase } from './utils/kebab-to-camelcase'; // Assigned objects should be ignore during proxification. const contextAssignedObjects = new WeakMap(); +// Store the context proxy and fallback for each object in the context. +const contextObjectToProxy = new WeakMap(); +const contextProxyToObject = new WeakMap(); +const contextObjectToFallback = new WeakMap(); + const isPlainObject = ( item ) => item && typeof item === 'object' && item.constructor === Object; @@ -36,58 +41,87 @@ const descriptor = Reflect.getOwnPropertyDescriptor; * * @return {Object} The wrapped context object. */ -const proxifyContext = ( current, inherited = {} ) => - new Proxy( current, { - get: ( target, k ) => { - // Always subscribe to prop changes in the current context. - const currentProp = target[ k ]; - - // Return the inherited prop when missing in target. - if ( ! ( k in target ) && k in inherited ) { - return inherited[ k ]; - } +const proxifyContext = ( current, inherited = {} ) => { + // Update the fallback object reference when it changes. + contextObjectToFallback.set( current, inherited ); + if ( ! contextObjectToProxy.has( current ) ) { + const proxy = new Proxy( current, { + get: ( target, k ) => { + const fallback = contextObjectToFallback.get( current ); + // Always subscribe to prop changes in the current context. + const currentProp = target[ k ]; + + // Return the inherited prop when missing in target. + if ( ! ( k in target ) && k in fallback ) { + return fallback[ k ]; + } - // 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, inherited[ k ] ); - } + // Proxify plain objects that were not directly assigned. + if ( + k in target && + ! contextAssignedObjects.get( target )?.has( k ) && + isPlainObject( peek( target, k ) ) + ) { + return proxifyContext( currentProp, fallback[ k ] ); + } - /* - * For other cases, return the value from target, also subscribing - * to changes in the parent context when the current prop is - * not defined. - */ - return k in target ? currentProp : inherited[ k ]; - }, - 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() ); + // Return the stored proxy for `currentProp` when it exists. + if ( contextObjectToProxy.has( currentProp ) ) { + return contextObjectToProxy.get( currentProp ); } - 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 ), - } ); + /* + * For other cases, return the value from target, also + * subscribing to changes in the parent context when the current + * prop is not defined. + */ + return k in target ? currentProp : fallback[ k ]; + }, + set: ( target, k, value ) => { + const fallback = contextObjectToFallback.get( current ); + const obj = + k in target || ! ( k in fallback ) ? target : fallback; + + /* + * Assigned object values 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 ); + } + + /* + * When the value is a proxy, it's because it comes from the + * context, so the inner value is assigned instead. + */ + if ( contextProxyToObject.has( value ) ) { + const innerValue = contextProxyToObject.get( value ); + obj[ k ] = innerValue; + } else { + obj[ k ] = value; + } + + return true; + }, + ownKeys: ( target ) => [ + ...new Set( [ + ...Object.keys( contextObjectToFallback.get( current ) ), + ...Object.keys( target ), + ] ), + ], + getOwnPropertyDescriptor: ( target, k ) => + descriptor( target, k ) || + descriptor( contextObjectToFallback.get( current ), k ), + } ); + contextObjectToProxy.set( current, proxy ); + contextProxyToObject.set( proxy, current ); + } + return contextObjectToProxy.get( current ); +}; /** * Recursively update values within a deepSignal object. diff --git a/test/e2e/specs/interactivity/directive-context.spec.ts b/test/e2e/specs/interactivity/directive-context.spec.ts index 85341774c2af4..41afa274155a2 100644 --- a/test/e2e/specs/interactivity/directive-context.spec.ts +++ b/test/e2e/specs/interactivity/directive-context.spec.ts @@ -336,4 +336,46 @@ test.describe( 'data-wp-context', () => { await expect( counterChild ).toHaveText( '1' ); await expect( changes ).toHaveText( '2' ); } ); + + test( 'references to the same context object should be preserved', async ( { + page, + } ) => { + const isProxyPreserved = page.getByTestId( 'is proxy preserved' ); + await expect( isProxyPreserved ).toHaveText( 'true' ); + } ); + + test( 'references to copied context objects should be preserved', async ( { + page, + } ) => { + await page.getByTestId( 'child copy obj' ).click(); + const isProxyPreservedOnCopy = page.getByTestId( + 'is proxy preserved on copy' + ); + await expect( isProxyPreservedOnCopy ).toHaveText( 'true' ); + } ); + + test( 'objects referenced from the context inherit properties where they are originally defined ', async ( { + page, + } ) => { + await page.getByTestId( 'child copy obj' ).click(); + + const childContextBefore = await parseContent( + page.getByTestId( 'child context' ) + ); + + expect( childContextBefore.obj2.prop4 ).toBe( 'parent' ); + expect( childContextBefore.obj2.prop5 ).toBe( 'child' ); + expect( childContextBefore.obj2.prop6 ).toBe( 'child' ); + + await page.getByTestId( 'parent replace' ).click(); + + const childContextAfter = await parseContent( + page.getByTestId( 'child context' ) + ); + + expect( childContextAfter.obj2.prop4 ).toBeUndefined(); + expect( childContextAfter.obj2.prop5 ).toBe( 'child' ); + expect( childContextAfter.obj2.prop6 ).toBe( 'child' ); + expect( childContextAfter.obj2.overwritten ).toBe( true ); + } ); } );