From da8770d258636463553ffbf8c381cd9ba9746613 Mon Sep 17 00:00:00 2001 From: Michal Date: Fri, 4 Oct 2024 16:30:25 +0100 Subject: [PATCH] Interactivity API: Correctly handle lazily added, deeply nested properties 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 Co-authored-by: DAreRodz Co-authored-by: cbravobernal --- packages/interactivity/CHANGELOG.md | 4 + packages/interactivity/src/proxies/state.ts | 81 ++++++++++--------- .../src/proxies/test/context-proxy.ts | 62 +++++++++++++- .../src/proxies/test/deep-merge.ts | 73 +++++++++++++++++ 4 files changed, 182 insertions(+), 38 deletions(-) diff --git a/packages/interactivity/CHANGELOG.md b/packages/interactivity/CHANGELOG.md index 6989bcdc0c802..cc9516bdb2cfa 100644 --- a/packages/interactivity/CHANGELOG.md +++ b/packages/interactivity/CHANGELOG.md @@ -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.8.0 (2024-09-19) ### Enhancements diff --git a/packages/interactivity/src/proxies/state.ts b/packages/interactivity/src/proxies/state.ts index c91d8f6ab90a5..f63c84738bcb1 100644 --- a/packages/interactivity/src/proxies/state.ts +++ b/packages/interactivity/src/proxies/state.ts @@ -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++; } }; diff --git a/packages/interactivity/src/proxies/test/context-proxy.ts b/packages/interactivity/src/proxies/test/context-proxy.ts index 306b3e4a8aa94..1e4a969d0f9dc 100644 --- a/packages/interactivity/src/proxies/test/context-proxy.ts +++ b/packages/interactivity/src/proxies/test/context-proxy.ts @@ -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', () => { @@ -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', () => { diff --git a/packages/interactivity/src/proxies/test/deep-merge.ts b/packages/interactivity/src/proxies/test/deep-merge.ts index bf31c4b552133..f475385a43787 100644 --- a/packages/interactivity/src/proxies/test/deep-merge.ts +++ b/packages/interactivity/src/proxies/test/deep-merge.ts @@ -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(); + } ); } ); } );