Interactivity API: Correctly handle lazily added, deeply nested properties with deepMerge()

[michalczaplinski]

Description

This PR addresses an issue in the Interactivity API where deeply nested properties that are initially undefined and later added with the interdeepMerge() to either the context or the global state were not being properly handled. The change ensures that effects dependent on these properties are correctly triggered when the properties are added, even if they are initially undefined.

Changes

• Added a new test case in packages/interactivity/src/proxies/test/context-proxy.ts to demonstrate the desired behavior for deeply nested properties.
• Implement the necessary changes in the deepMerge function to support this behavior.

Testing

A new test case has been added to verify the correct handling of deeply nested properties.

[michalczaplinski]

@DAreRodz both of those assertions fail:

• expect( spy ).toHaveBeenCalledTimes( 2 );

   Expected number of calls: 2
   Received number of calls: 1
  

• expect( deepValue ).toBe( 'test value' );

   Expected: "test value"
   Received: undefined
  

But I'm not 100% sure if those are precisely the failures we wanted to see.

[DAreRodz]

Thanks for the PR, @michalczaplinski. 😁

I'm not 100% sure if those are precisely the failures we wanted to see.

They are. The values received are the same as those returned in the initial call. Basically, the spy function has not been executed again because the signal for context.a has not been updated.

[michalczaplinski]

I had to stare at the test a little longer to convince myself that it was actually the case 😅.

Excellent, I'll add the fix tomorrow 🙂

[michalczaplinski]

@DAreRodz

As we discussed, I was expecting the unit test to fail, but after updating the test cases they are actually passing!

---

Here's what's going on. Let's assume I'm going through an example that follows the newly added unit tests in this PR - meaning I'm trying to follow what's going on when running the following code:

const fallback: any = proxifyContext(
	proxifyState( 'test', {} ),
	{}
);

const context: any = proxifyContext(
	proxifyState( 'test', {} ),
	fallback
);

deepMerge( context, { a: { b: { c: { d: 'test value' } } } } );

When we set a property on the context using deepMerge(), it enters into this code path and tries to set the value:

gutenberg/packages/interactivity/src/proxies/state.ts

Lines 303 to 305 in 3ef1f89

	if ( isNew ) {
	target[ key ] = {};
	}

This triggers the set() trap of the state proxy which calls Reflect.set() here:

gutenberg/packages/interactivity/src/proxies/state.ts

Line 153 in 3ef1f89

return Reflect.set( target, key, value, receiver );

At this point:

• target is a Proxied object ({})
• key is a
• value is {}

Since the key does not exist on target, this further triggers the defineProperty() trap. It is there that the value is finally set:

gutenberg/packages/interactivity/src/proxies/state.ts

Lines 165 to 166 in 3ef1f89

	const result = Reflect.defineProperty( target, key, desc );

However, the defineProperty() trap also creates a signal and proxifies the state!

gutenberg/packages/interactivity/src/proxies/state.ts

Lines 175 to 177 in 3ef1f89

	prop.setValue(
	shouldProxy( value ) ? proxifyState( ns, value ) : value
	);

After that, context.a is proxied. When the defineProperty() trap and subsequently the set() trap returns, we are back here:

gutenberg/packages/interactivity/src/proxies/state.ts

Lines 307 to 308 in 3ef1f89

	deepMergeRecursive( target[ key ], source[ key ], override );
	} else if ( override || isNew ) {

As we call deepMergeRecursive, we'll recurse one level lower and repeat the process until the whole nested object is proxified.

[DAreRodz]

Oh, I understand now. 😄

The context currently uses two proxies: one for property inheritance (proxifyContext) and a second for property reactiveness (proxifyState). To work correctly, the deepMerge function should be used with the second proxy (proxifyState). If not, it doesn't recognize the underlying object, and all property manipulations go through the context/state proxy handlers when it shouldn't

This is what we do in the data-wp-context directive:

1. Creates a state object (with proxifyState)

   gutenberg/packages/interactivity/src/directives.tsx

   Line 148 in 636710b

   const currentValue = useRef( proxifyState( ns, {} ) );

2. Updates the state object (with deepMerge)

   gutenberg/packages/interactivity/src/directives.tsx

   Lines 161 to 165 in 636710b

   	deepMerge(
   	currentValue.current,
   	deepClone( value ) as object,
   	false
   	);

3. Wraps the state object with a context proxy (with proxifyContext)

   gutenberg/packages/interactivity/src/directives.tsx

   Lines 166 to 169 in 636710b

   	result[ namespace ] = proxifyContext(
   	currentValue.current,
   	inheritedValue[ namespace ]
   	);

---

In 48433fe, I moved one of the tests to tests/deep-merge.ts and reproduced the issue.

FAIL packages/interactivity/src/proxies/test/deep-merge.ts
  ● Interactivity API › deepMerge › should handle deeply nested properties that are initially undefined and merged with deepMerge

    expect(jest.fn()).toHaveBeenCalledTimes(expected)

    Expected number of calls: 2
    Received number of calls: 1

      408 |
      409 | 			// The effect should be called again
    > 410 | 			expect( spy ).toHaveBeenCalledTimes( 2 );
          | 			              ^
      411 | 			expect( deepValue ).toBe( 'test value' );
      412 |
      413 | 			// Reading the value directly should also work

      at Object.toHaveBeenCalledTimes (packages/interactivity/src/proxies/test/deep-merge.ts:410:18)

[github-actions]

Flaky tests detected in 8f7fc13.
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/11013548741
📝 Reported issues:

• [Flaky Test] Navigates the items list via UP/DOWN arrow keys #64334 in /test/e2e/specs/site-editor/dataviews-list-layout-keyboard.spec.js

[github-actions]

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: michalczaplinski <[email protected]>
Co-authored-by: DAreRodz <[email protected]>
Co-authored-by: cbravobernal <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[cbravobernal]

I just added the changelog changes. The rest looks good to me.

[github-actions]

There was a conflict while trying to cherry-pick the commit to the wp/6.7 branch. Please resolve the conflict manually and create a PR to the wp/6.7 branch.

PRs to wp/6.7 are similar to PRs to trunk, but you should base your PR on the wp/6.7 branch instead of trunk.

# Checkout the wp/6.7 branch instead of trunk.
git checkout wp/6.7
# Create a new branch for your PR.
git checkout -b my-branch
# Cherry-pick the commit.
git cherry-pick 366cf1f8d6a2169217b82a57ee1af8ba4c1f8b42
# Check which files have conflicts.
git status
# Resolve the conflict...
# Add the resolved files to the staging area.
git status
git add .
git cherry-pick --continue
# Push the branch to the repository
git push origin my-branch
# Create a PR and set the base to the wp/6.7 branch.
# See https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/changing-the-base-branch-of-a-pull-request.

[github-actions]

There was a conflict while trying to cherry-pick the commit to the wp/6.7 branch. Please resolve the conflict manually and create a PR to the wp/6.7 branch.

PRs to wp/6.7 are similar to PRs to trunk, but you should base your PR on the wp/6.7 branch instead of trunk.

# Checkout the wp/6.7 branch instead of trunk.
git checkout wp/6.7
# Create a new branch for your PR.
git checkout -b my-branch
# Cherry-pick the commit.
git cherry-pick 366cf1f8d6a2169217b82a57ee1af8ba4c1f8b42
# Check which files have conflicts.
git status
# Resolve the conflict...
# Add the resolved files to the staging area.
git status
git add .
git cherry-pick --continue
# Push the branch to the repository
git push origin my-branch
# Create a PR and set the base to the wp/6.7 branch.
# See https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/changing-the-base-branch-of-a-pull-request.

[cbravobernal]

I just cherry-picked this PR to the release/19.4 branch to get it included in the next release: dc70023