Skip to content

Commit

Permalink
Interactivity API: Correctly handle lazily added, deeply nested prope…
Browse files Browse the repository at this point in the history 
…rties with `deepMerge()` (#65465)

* test: Add case for deeply nested undefined properties in context proxy

Add a test to ensure the context proxy correctly handles and updates
deeply nested properties that are initially undefined.

* Update the test case to use `proxifyState()`

* Add test for deepMerge with nested undefined properties

* Add a failing test in `deep-merge.ts`

* Add another failing test

* Make all the tests pass

* Simplify code

* Fix test case name

* Add one extra test

* Update test in `context-proxy`

* Update changelog

---------

Co-authored-by: michalczaplinski <[email protected]>
Co-authored-by: DAreRodz <[email protected]>
Co-authored-by: cbravobernal <[email protected]>
  • Loading branch information
@michalczaplinski @cbravobernal
4 people authored Oct 4, 2024
1 parent 9b1ee51 commit 366cf1f
Show file tree
Hide file tree
Showing 4 changed files with 182 additions and 38 deletions.
4 changes: 4 additions & 0 deletions packages/interactivity/CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

## Unreleased

### Bug Fixes

- Correctly handle lazily added, deeply nested properties with `deepMerge()` ([#65465](https://github.com/WordPress/gutenberg/pull/65465)).

## 6.9.0 (2024-10-03)

## 6.8.0 (2024-09-19)
Expand Down
81 changes: 44 additions & 37 deletions packages/interactivity/src/proxies/state.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -300,50 +300,57 @@ const deepMergeRecursive = (
source: any,
override: boolean = true
) => {
if ( isPlainObject( target ) && isPlainObject( source ) ) {
let hasNewKeys = false;
for ( const key in source ) {
const isNew = ! ( key in target );
hasNewKeys = hasNewKeys || isNew;
if ( ! ( isPlainObject( target ) && isPlainObject( source ) ) ) {
return;
}

const desc = Object.getOwnPropertyDescriptor( source, key );
if (
typeof desc?.get === 'function' ||
typeof desc?.set === 'function'
) {
if ( override || isNew ) {
Object.defineProperty( target, key, {
...desc,
configurable: true,
enumerable: true,
} );

const proxy = getProxyFromObject( target );
if ( desc?.get && proxy && hasPropSignal( proxy, key ) ) {
const propSignal = getPropSignal( proxy, key );
propSignal.setGetter( desc.get );
}
}
} else if ( isPlainObject( source[ key ] ) ) {
if ( isNew ) {
target[ key ] = {};
}
let hasNewKeys = false;

deepMergeRecursive( target[ key ], source[ key ], override );
} else if ( override || isNew ) {
Object.defineProperty( target, key, desc! );
for ( const key in source ) {
const isNew = ! ( key in target );
hasNewKeys = hasNewKeys || isNew;

const proxy = getProxyFromObject( target );
if ( desc?.value && proxy && hasPropSignal( proxy, key ) ) {
const propSignal = getPropSignal( proxy, key );
propSignal.setValue( desc.value );
const desc = Object.getOwnPropertyDescriptor( source, key )!;
const proxy = getProxyFromObject( target );
const propSignal =
!! proxy &&
hasPropSignal( proxy, key ) &&
getPropSignal( proxy, key );

if (
typeof desc.get === 'function' ||
typeof desc.set === 'function'
) {
if ( override || isNew ) {
Object.defineProperty( target, key, {
...desc,
configurable: true,
enumerable: true,
} );
if ( desc.get && propSignal ) {
propSignal.setGetter( desc.get );
}
}
} else if ( isPlainObject( source[ key ] ) ) {
if ( isNew || ( override && ! isPlainObject( target[ key ] ) ) ) {
target[ key ] = {};
if ( propSignal ) {
propSignal.setValue( target[ key ] );
}
}
if ( isPlainObject( target[ key ] ) ) {
deepMergeRecursive( target[ key ], source[ key ], override );
}
} else if ( override || isNew ) {
Object.defineProperty( target, key, desc );
if ( propSignal ) {
propSignal.setValue( desc.value );
}
}
}

if ( hasNewKeys && objToIterable.has( target ) ) {
objToIterable.get( target )!.value++;
}
if ( hasNewKeys && objToIterable.has( target ) ) {
objToIterable.get( target )!.value++;
}
};

Expand Down
62 changes: 61 additions & 1 deletion packages/interactivity/src/proxies/test/context-proxy.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { effect } from '@preact/signals';
/**
* Internal dependencies
*/
import { proxifyContext, proxifyState } from '../';
import { proxifyContext, proxifyState, deepMerge } from '../';

describe( 'Interactivity API', () => {
describe( 'context proxy', () => {
Expand Down Expand Up @@ -277,6 +277,66 @@ describe( 'Interactivity API', () => {
'fromFallback',
] );
} );

it( 'should handle deeply nested properties that are initially undefined', () => {
const fallback: any = proxifyContext(
proxifyState( 'test', {} ),
{}
);
const context: any = proxifyContext(
proxifyState( 'test', {} ),
fallback
);

let deepValue: any;
const spy = jest.fn( () => {
deepValue = context.a?.b?.c?.d;
} );
effect( spy );

// Initial call, the deep value is undefined
expect( spy ).toHaveBeenCalledTimes( 1 );
expect( deepValue ).toBeUndefined();

// Add a deeply nested object to the context
context.a = { b: { c: { d: 'test value' } } };

// The effect should be called again
expect( spy ).toHaveBeenCalledTimes( 2 );
expect( deepValue ).toBe( 'test value' );

// Reading the value directly should also work
expect( context.a.b.c.d ).toBe( 'test value' );
} );

it( 'should handle deeply nested properties that are initially undefined and merged with deepMerge', () => {
const fallbackState = proxifyState( 'test', {} );
const fallback: any = proxifyContext( fallbackState, {} );
const contextState = proxifyState( 'test', {} );
const context: any = proxifyContext( contextState, fallback );

let deepValue: any;
const spy = jest.fn( () => {
deepValue = context.a?.b?.c?.d;
} );
effect( spy );

// Initial call, the deep value is undefined
expect( spy ).toHaveBeenCalledTimes( 1 );
expect( deepValue ).toBeUndefined();

// Use deepMerge to add a deeply nested object to the context
deepMerge( contextState, {
a: { b: { c: { d: 'test value' } } },
} );

// The effect should be called again
expect( spy ).toHaveBeenCalledTimes( 2 );
expect( deepValue ).toBe( 'test value' );

// Reading the value directly should also work
expect( context.a.b.c.d ).toBe( 'test value' );
} );
} );

describe( 'proxifyContext', () => {
Expand Down
73 changes: 73 additions & 0 deletions packages/interactivity/src/proxies/test/deep-merge.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -389,5 +389,78 @@ describe( 'Interactivity API', () => {
expect( spy ).toHaveBeenCalledTimes( 2 );
expect( spy ).toHaveLastReturnedWith( [ 'a', 'b', 'c' ] );
} );

it( 'should handle deeply nested properties that are initially undefined', () => {
const target: any = proxifyState( 'test', {} );

let deepValue: any;
const spy = jest.fn( () => {
deepValue = target.a?.b?.c?.d;
} );
effect( spy );

// Initial call, the deep value is undefined
expect( spy ).toHaveBeenCalledTimes( 1 );
expect( deepValue ).toBeUndefined();

// Use deepMerge to add a deeply nested object to the target
deepMerge( target, { a: { b: { c: { d: 'test value' } } } } );

// The effect should be called again
expect( spy ).toHaveBeenCalledTimes( 2 );
expect( deepValue ).toBe( 'test value' );

// Reading the value directly should also work
expect( target.a.b.c.d ).toBe( 'test value' );
} );

it( 'should overwrite values that become objects', () => {
const target: any = proxifyState( 'test', { message: 'hello' } );

let message: any;
const spy = jest.fn( () => ( message = target.message ) );
effect( spy );

expect( spy ).toHaveBeenCalledTimes( 1 );
expect( message ).toBe( 'hello' );

deepMerge( target, {
message: { content: 'hello', fontStyle: 'italic' },
} );

expect( spy ).toHaveBeenCalledTimes( 2 );
expect( message ).toEqual( {
content: 'hello',
fontStyle: 'italic',
} );

expect( target.message ).toEqual( {
content: 'hello',
fontStyle: 'italic',
} );
} );

it( 'should not overwrite values that become objects if `override` is false', () => {
const target: any = proxifyState( 'test', { message: 'hello' } );

let message: any;
const spy = jest.fn( () => ( message = target.message ) );
effect( spy );

expect( spy ).toHaveBeenCalledTimes( 1 );
expect( message ).toBe( 'hello' );

deepMerge(
target,
{ message: { content: 'hello', fontStyle: 'italic' } },
false
);

expect( spy ).toHaveBeenCalledTimes( 1 );
expect( message ).toBe( 'hello' );
expect( target.message ).toBe( 'hello' );
expect( target.message.content ).toBeUndefined();
expect( target.message.fontStyle ).toBeUndefined();
} );
} );
} );

1 comment on commit 366cf1f

@github-actions
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Flaky tests detected in 366cf1f.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11182712386
📝 Reported issues:

Please sign in to comment.