Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Interactivity API: Fix context object proxy references #59553

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,24 @@
>
obj.prop6
</button>
<button
data-testid="child copy obj"
data-wp-on--click="actions.copyObj"
>
Copy obj
</button>
<div>
Is proxy preserved? <span
data-testid="is proxy preserved"
data-wp-text="state.isProxyPreserved"
></span>
</div>
<div>
Is proxy preserved on copy? <span
data-testid="is proxy preserved on copy"
data-wp-text="state.isProxyPreservedOnCopy"
></span>
</div>
</div>
<br />

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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: {
Expand All @@ -34,6 +43,10 @@ store( 'directive-context', {
replaceObj() {
const ctx = getContext();
ctx.obj = { overwritten: true };
},
copyObj() {
const ctx = getContext();
ctx.obj2 = ctx.obj;
}
},
} );
Expand Down
1 change: 1 addition & 0 deletions packages/interactivity/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
130 changes: 82 additions & 48 deletions packages/interactivity/src/directives.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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.
Expand Down
42 changes: 42 additions & 0 deletions test/e2e/specs/interactivity/directive-context.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 );
} );
} );
Loading