From b5717bb0f8406f65a24f743b0b8bcd0739dcb269 Mon Sep 17 00:00:00 2001 From: David Arenas Date: Wed, 16 Oct 2024 20:25:30 +0200 Subject: [PATCH 1/6] Fix types in test --- packages/interactivity/src/proxies/test/deep-merge.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/packages/interactivity/src/proxies/test/deep-merge.ts b/packages/interactivity/src/proxies/test/deep-merge.ts index f475385a437876..99b86497fb49c3 100644 --- a/packages/interactivity/src/proxies/test/deep-merge.ts +++ b/packages/interactivity/src/proxies/test/deep-merge.ts @@ -379,7 +379,10 @@ describe( 'Interactivity API', () => { const target = proxifyState< any >( 'test', { a: 1, b: 2 } ); const source = { a: 1, b: 2, c: 3 }; - const spy = jest.fn( () => Object.keys( target ) ); + let keys: any; + const spy = jest.fn( () => { + keys = Object.keys( target ); + } ); effect( spy ); expect( spy ).toHaveBeenCalledTimes( 1 ); @@ -387,7 +390,7 @@ describe( 'Interactivity API', () => { deepMerge( target, source, false ); expect( spy ).toHaveBeenCalledTimes( 2 ); - expect( spy ).toHaveLastReturnedWith( [ 'a', 'b', 'c' ] ); + expect( keys ).toEqual( [ 'a', 'b', 'c' ] ); } ); it( 'should handle deeply nested properties that are initially undefined', () => { From e113a3bc82ada60009ddf5c529cb38affd73b372 Mon Sep 17 00:00:00 2001 From: David Arenas Date: Wed, 16 Oct 2024 20:25:42 +0200 Subject: [PATCH 2/6] Add failing tests --- .../src/proxies/test/deep-merge.ts | 56 +++++++++++++++++++ 1 file changed, 56 insertions(+) diff --git a/packages/interactivity/src/proxies/test/deep-merge.ts b/packages/interactivity/src/proxies/test/deep-merge.ts index 99b86497fb49c3..2797a7ce7349fe 100644 --- a/packages/interactivity/src/proxies/test/deep-merge.ts +++ b/packages/interactivity/src/proxies/test/deep-merge.ts @@ -465,5 +465,61 @@ describe( 'Interactivity API', () => { expect( target.message.content ).toBeUndefined(); expect( target.message.fontStyle ).toBeUndefined(); } ); + + it( 'should keep reactivity of objects that are initially undefined', () => { + const target: any = proxifyState( 'test', {} ); + + let deepValue: any; + const spy = jest.fn( () => { + deepValue = target.obj?.nested; + } ); + 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, { obj: { nested: 'value 1' } } ); + + // The effect should be called again + expect( spy ).toHaveBeenCalledTimes( 2 ); + expect( deepValue ).toBe( 'value 1' ); + + // Modify the nested value + target.obj.nested = 'value 2'; + + // The effect should be called again + expect( spy ).toHaveBeenCalledTimes( 3 ); + expect( deepValue ).toBe( 'value 2' ); + } ); + + it( 'should keep reactivity of arrays that are initially undefined', () => { + const target: any = proxifyState( 'test', {} ); + + let deepValue: any; + const spy = jest.fn( () => { + deepValue = target.array?.[ 0 ]; + } ); + effect( spy ); + + // Initial call, the deep value is undefined + expect( spy ).toHaveBeenCalledTimes( 1 ); + expect( deepValue ).toBeUndefined(); + + // Use deepMerge to add an array to the target + deepMerge( target, { array: [ 'value 1' ] } ); + + // The effect should be called again + expect( spy ).toHaveBeenCalledTimes( 2 ); + expect( deepValue ).toBe( 'value 1' ); + + // Modify the array value + target.array[ 0 ] = 'value 2'; + + // The effect should be called again + expect( spy ).toHaveBeenCalledTimes( 3 ); + expect( deepValue ).toBe( 'value 2' ); + } ); } ); } ); From 72a47cd6389f80c5e9c08a821453bf30acc60c54 Mon Sep 17 00:00:00 2001 From: David Arenas Date: Wed, 16 Oct 2024 20:26:23 +0200 Subject: [PATCH 3/6] Fix the bug --- packages/interactivity/src/proxies/state.ts | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/packages/interactivity/src/proxies/state.ts b/packages/interactivity/src/proxies/state.ts index f63c84738bcb1a..d4d573acba8847 100644 --- a/packages/interactivity/src/proxies/state.ts +++ b/packages/interactivity/src/proxies/state.ts @@ -335,7 +335,10 @@ const deepMergeRecursive = ( if ( isNew || ( override && ! isPlainObject( target[ key ] ) ) ) { target[ key ] = {}; if ( propSignal ) { - propSignal.setValue( target[ key ] ); + const ns = getNamespaceFromProxy( proxy ); + propSignal.setValue( + proxifyState( ns, target[ key ] as Object ) + ); } } if ( isPlainObject( target[ key ] ) ) { @@ -344,7 +347,11 @@ const deepMergeRecursive = ( } else if ( override || isNew ) { Object.defineProperty( target, key, desc ); if ( propSignal ) { - propSignal.setValue( desc.value ); + const { value } = desc; + const ns = getNamespaceFromProxy( proxy ); + propSignal.setValue( + shouldProxy( value ) ? proxifyState( ns, value ) : value + ); } } } From 067d6eb1457969286e7d4bcade3df8739e9bdfed Mon Sep 17 00:00:00 2001 From: David Arenas Date: Wed, 16 Oct 2024 21:14:50 +0200 Subject: [PATCH 4/6] Update changelog --- packages/interactivity/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/interactivity/CHANGELOG.md b/packages/interactivity/CHANGELOG.md index b5a8fbcb9dde50..477a786a93e555 100644 --- a/packages/interactivity/CHANGELOG.md +++ b/packages/interactivity/CHANGELOG.md @@ -12,6 +12,7 @@ - Fix an issue where "default" could not be used as a directive suffix ([#65815](https://github.com/WordPress/gutenberg/pull/65815)). - Correctly handle lazily added, deeply nested properties with `deepMerge()` ([#65465](https://github.com/WordPress/gutenberg/pull/65465)). +- Fix reactivity of undefined objects and arrays added with `deepMerge()` ([#66183](https://github.com/WordPress/gutenberg/pull/66183)). ## 6.9.0 (2024-10-03) From bf3893033b878d17fad6a55c17b91e27042504bf Mon Sep 17 00:00:00 2001 From: David Arenas Date: Thu, 17 Oct 2024 11:16:55 +0200 Subject: [PATCH 5/6] Fix changelog --- packages/interactivity/CHANGELOG.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/interactivity/CHANGELOG.md b/packages/interactivity/CHANGELOG.md index 477a786a93e555..73212578ac109c 100644 --- a/packages/interactivity/CHANGELOG.md +++ b/packages/interactivity/CHANGELOG.md @@ -2,6 +2,10 @@ ## Unreleased +### Bug Fixes + +- Fix reactivity of undefined objects and arrays added with `deepMerge()` ([#66183](https://github.com/WordPress/gutenberg/pull/66183)). + ## 6.10.0 (2024-10-16) ### Internal @@ -12,7 +16,6 @@ - Fix an issue where "default" could not be used as a directive suffix ([#65815](https://github.com/WordPress/gutenberg/pull/65815)). - Correctly handle lazily added, deeply nested properties with `deepMerge()` ([#65465](https://github.com/WordPress/gutenberg/pull/65465)). -- Fix reactivity of undefined objects and arrays added with `deepMerge()` ([#66183](https://github.com/WordPress/gutenberg/pull/66183)). ## 6.9.0 (2024-10-03) From fcd5547a4d38c407e03f4e1464c77057d24f7408 Mon Sep 17 00:00:00 2001 From: David Arenas Date: Fri, 18 Oct 2024 12:19:21 +0200 Subject: [PATCH 6/6] Update tests --- .../src/proxies/test/deep-merge.ts | 35 ++++--------------- 1 file changed, 7 insertions(+), 28 deletions(-) diff --git a/packages/interactivity/src/proxies/test/deep-merge.ts b/packages/interactivity/src/proxies/test/deep-merge.ts index 2797a7ce7349fe..267e4850f9af91 100644 --- a/packages/interactivity/src/proxies/test/deep-merge.ts +++ b/packages/interactivity/src/proxies/test/deep-merge.ts @@ -415,6 +415,13 @@ describe( 'Interactivity API', () => { // Reading the value directly should also work expect( target.a.b.c.d ).toBe( 'test value' ); + + // Modify the nested value + target.a.b.c.d = 'new test value'; + + // The effect should be called again + expect( spy ).toHaveBeenCalledTimes( 3 ); + expect( deepValue ).toBe( 'new test value' ); } ); it( 'should overwrite values that become objects', () => { @@ -466,34 +473,6 @@ describe( 'Interactivity API', () => { expect( target.message.fontStyle ).toBeUndefined(); } ); - it( 'should keep reactivity of objects that are initially undefined', () => { - const target: any = proxifyState( 'test', {} ); - - let deepValue: any; - const spy = jest.fn( () => { - deepValue = target.obj?.nested; - } ); - 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, { obj: { nested: 'value 1' } } ); - - // The effect should be called again - expect( spy ).toHaveBeenCalledTimes( 2 ); - expect( deepValue ).toBe( 'value 1' ); - - // Modify the nested value - target.obj.nested = 'value 2'; - - // The effect should be called again - expect( spy ).toHaveBeenCalledTimes( 3 ); - expect( deepValue ).toBe( 'value 2' ); - } ); - it( 'should keep reactivity of arrays that are initially undefined', () => { const target: any = proxifyState( 'test', {} );