-
Notifications
You must be signed in to change notification settings - Fork 4.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Interactivity API: Correctly handle lazily added, deeply nested properties in context #65465
base: trunk
Are you sure you want to change the base?
Conversation
Add a test to ensure the context proxy correctly handles and updates deeply nested properties that are initially undefined.
// The effect should be called again | ||
expect( spy ).toHaveBeenCalledTimes( 2 ); | ||
expect( deepValue ).toBe( 'test value' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 🙂
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 gutenberg/packages/interactivity/src/proxies/state.ts Lines 303 to 305 in 3ef1f89
This triggers the
At this point:
Since the gutenberg/packages/interactivity/src/proxies/state.ts Lines 165 to 166 in 3ef1f89
However, the gutenberg/packages/interactivity/src/proxies/state.ts Lines 175 to 177 in 3ef1f89
After that, gutenberg/packages/interactivity/src/proxies/state.ts Lines 307 to 308 in 3ef1f89
As we call |
Oh, I understand now. 😄 The context currently uses two proxies: one for property inheritance ( This is what we do in the
In 48433fe, I moved one of the tests to
|
Flaky tests detected in 48433fe. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10992767768
|
Description
This PR addresses an issue in the Interactivity API where deeply nested properties that are initially undefined and later added to the context 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 initially undefined.
Changes
packages/interactivity/src/proxies/test/context-proxy.ts
to demonstrate the desired behavior for deeply nested properties.proxifyContext
ordeepMerge
function to support this behavior.Testing
A new test case has been added to verify the correct handling of deeply nested properties
TODO
proxifyContext()
ordeepMerge()
function to make the new test pass.