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 <czapla@git.wordpress.org>
Co-authored-by: DAreRodz <darerodz@git.wordpress.org>
Co-authored-by: cbravobernal <cbravobernal@git.wordpress.org>
  • Loading branch information
@michalczaplinski @cbravobernal
4 people committed Oct 7, 2024
1 parent a1b696d commit dc70023
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();
} );
} );
} );

0 comments on commit dc70023

Please sign in to comment.