Skip to content
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

Draft
wants to merge 3 commits into
base: trunk
Choose a base branch
from
Draft
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions packages/interactivity/src/proxies/test/context-proxy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,30 @@
'fromFallback',
] );
} );

it( 'should handle deeply nested properties that are initially undefined', () => {
const context: any = proxifyContext( {}, {} );

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 );

Check failure on line 298 in packages/interactivity/src/proxies/test/context-proxy.ts

View workflow job for this annotation

GitHub Actions / JavaScript (Node.js 20) 2/4

Error: expect(jest.fn()).toHaveBeenCalledTimes(expected) Expected number of calls: 2 Received number of calls: 1 at Object.toHaveBeenCalledTimes (/home/runner/work/gutenberg/gutenberg/packages/interactivity/src/proxies/test/context-proxy.ts:298:19) at Promise.then.completed (/home/runner/work/gutenberg/gutenberg/node_modules/jest-circus/build/utils.js:300:28) at new Promise (<anonymous>) at callAsyncCircusFn (/home/runner/work/gutenberg/gutenberg/node_modules/jest-circus/build/utils.js:233:10) at _callCircusTest (/home/runner/work/gutenberg/gutenberg/node_modules/jest-circus/build/run.js:315:40) at processTicksAndRejections (node:internal/process/task_queues:95:5) at _runTest (/home/runner/work/gutenberg/gutenberg/node_modules/jest-circus/build/run.js:251:3) at _runTestsForDescribeBlock (/home/runner/work/gutenberg/gutenberg/node_modules/jest-circus/build/run.js:125:9) at _runTestsForDescribeBlock (/home/runner/work/gutenberg/gutenberg/node_modules/jest-circus/build/run.js:120:9) at _runTestsForDescribeBlock (/home/runner/work/gutenberg/gutenberg/node_modules/jest-circus/build/run.js:120:9) at _runTestsForDescribeBlock (/home/runner/work/gutenberg/gutenberg/node_modules/jest-circus/build/run.js:120:9) at run (/home/runner/work/gutenberg/gutenberg/node_modules/jest-circus/build/run.js:70:3) at runAndTransformResultsToJestFormat (/home/runner/work/gutenberg/gutenberg/node_modules/jest-circus/build/legacy-code-todo-rewrite/jestAdapterInit.js:122:21) at jestAdapter (/home/runner/work/gutenberg/gutenberg/node_modules/jest-circus/build/legacy-code-todo-rewrite/jestAdapter.js:79:19) at runTestInternal (/home/runner/work/gutenberg/gutenberg/node_modules/jest-runner/build/runTest.js:367:16) at runTest (/home/runner/work/gutenberg/gutenberg/node_modules/jest-runner/build/runTest.js:444:34) at Object.worker (/home/runner/work/gutenberg/gutenberg/node_modules/jest-runner/build/testWorker.js:106:12)

Check failure on line 298 in packages/interactivity/src/proxies/test/context-proxy.ts

View workflow job for this annotation

GitHub Actions / JavaScript (Node.js 22) 2/4

Error: expect(jest.fn()).toHaveBeenCalledTimes(expected) Expected number of calls: 2 Received number of calls: 1 at Object.toHaveBeenCalledTimes (/home/runner/work/gutenberg/gutenberg/packages/interactivity/src/proxies/test/context-proxy.ts:298:19) at Promise.then.completed (/home/runner/work/gutenberg/gutenberg/node_modules/jest-circus/build/utils.js:300:28) at new Promise (<anonymous>) at callAsyncCircusFn (/home/runner/work/gutenberg/gutenberg/node_modules/jest-circus/build/utils.js:233:10) at _callCircusTest (/home/runner/work/gutenberg/gutenberg/node_modules/jest-circus/build/run.js:315:40) at processTicksAndRejections (node:internal/process/task_queues:105:5) at _runTest (/home/runner/work/gutenberg/gutenberg/node_modules/jest-circus/build/run.js:251:3) at _runTestsForDescribeBlock (/home/runner/work/gutenberg/gutenberg/node_modules/jest-circus/build/run.js:125:9) at _runTestsForDescribeBlock (/home/runner/work/gutenberg/gutenberg/node_modules/jest-circus/build/run.js:120:9) at _runTestsForDescribeBlock (/home/runner/work/gutenberg/gutenberg/node_modules/jest-circus/build/run.js:120:9) at _runTestsForDescribeBlock (/home/runner/work/gutenberg/gutenberg/node_modules/jest-circus/build/run.js:120:9) at run (/home/runner/work/gutenberg/gutenberg/node_modules/jest-circus/build/run.js:70:3) at runAndTransformResultsToJestFormat (/home/runner/work/gutenberg/gutenberg/node_modules/jest-circus/build/legacy-code-todo-rewrite/jestAdapterInit.js:122:21) at jestAdapter (/home/runner/work/gutenberg/gutenberg/node_modules/jest-circus/build/legacy-code-todo-rewrite/jestAdapter.js:79:19) at runTestInternal (/home/runner/work/gutenberg/gutenberg/node_modules/jest-runner/build/runTest.js:367:16) at runTest (/home/runner/work/gutenberg/gutenberg/node_modules/jest-runner/build/runTest.js:444:34) at Object.worker (/home/runner/work/gutenberg/gutenberg/node_modules/jest-runner/build/testWorker.js:106:12)
expect( deepValue ).toBe( 'test value' );
Comment on lines +304 to +306
Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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 🙂


// Reading the value directly should also work
expect( context.a.b.c.d ).toBe( 'test value' );
} );
} );

describe( 'proxifyContext', () => {
Expand Down
Loading