diff --git a/packages/interactivity/CHANGELOG.md b/packages/interactivity/CHANGELOG.md index d35dfa7c81ae4..f4431d108dc36 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.9.0 (2024-10-03) ## 6.8.0 (2024-09-19) 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(); + } ); } ); } );