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.

Sign up for GitHub

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

useEntityRecord: Do not trigger REST API requests when disabled #56108

Merged
merged 2 commits into from
Nov 17, 2023
Merged

useEntityRecord: Do not trigger REST API requests when disabled #56108

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6ad87b3
useEntityRecord: Do not trigger REST API requests when disabled
Mamaduka Nov 14, 2023
2adce93
Adjust tests
Mamaduka Nov 17, 2023
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
34 changes: 34 additions & 0 deletions packages/core-data/src/hooks/test/use-entity-record.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,4 +113,38 @@ describe( 'useEntityRecord', () => {
expect( widget.editedRecord ).toEqual( { hello: 'foo', id: 1 } );
expect( widget.edits ).toEqual( { hello: 'foo' } );
} );

it( 'does not resolve entity record when disabled via options', async () => {
// Provide response
triggerFetch.mockImplementation( () => TEST_RECORD );

let data;
const TestComponent = () => {
data = useEntityRecord( 'root', 'widget', 2, {
options: { enabled: false },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should just be:

{ enabled: false },

Passing any object still works because we're only defaulting enabled to true if options is undefined. Otherwise, options.enabled will always be falsy (undefined) unless explicitly set to true. I think this is another bug we should fix.

} );
return <div />;
};
render(
<RegistryProvider value={ registry }>
<TestComponent />
</RegistryProvider>
);

expect( data ).toEqual( {
edit: expect.any( Function ),
editedRecord: {},
hasEdits: false,
edits: {},
record: null,
save: expect.any( Function ),
} );

// Fetch request should have been issued
await waitFor( () =>
expect( triggerFetch ).not.toHaveBeenCalledWith( {
Copy link
Contributor

@ntsekouras ntsekouras Nov 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should test what we expect and not something different. Probably use expect( triggerFetch ).not.toHaveBeenCalled(). The main thing though is that this test(even with my suggestion) passes in trunk without your changes. 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's odd. This was failing without changes in my test. I'll have a loot at it tomorrow.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the tests. They should fail on trunk but pass on this branch.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really weird :). I also tried yesterday to reset the mocks etc.., and for me the issue remains, but I think it's something we miss with the testing library.

When I comment out your (!enabled) code and run the test, it still passes in trunk. When I'm changing though the second check to:

expect( triggerFetch ).toHaveBeenCalledWith( {
	path: '/wp/v2/widgets/2?context=edit',
} )

(removing the not). The tests works as expected, but the expect( triggerFetch ).not.toHaveBeenCalled(); doesn't throw, which it should. I'll approve this PR now because the fix is good, but I'm curious why this is happening. I'll cc @kevin940726 and if we need to adjust something, let's do it in a follow up.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think apiFetch isn't dispatched when we do an assertion for the first time. But I wasn't able to figure out why. This seems to work for the first test.

cc @tyxla

Copy link
Member

@kevin940726 kevin940726 Nov 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

waitFor will immediately resolve if it passes the first time, so basically there's no waiting. This is tricky for testing something that "shouldn't" happen. Instead, I think we can fallback to the good old setTimeout and set an arbitrary reasonable waiting time.

await act(
	() => new Promise( ( resolve ) => setTimeout( resolve, 0 ) )
);
expect( triggerFetch ).not.toHaveBeenCalled();

In this context, setTimeout of 0 works, but it really depends on the implementation details. Awaiting a microtask (promise) doesn't work here, for instance.

If we want to make this test more resilient to implementation details, then we can first make a "enabled" assertion (.toHaveBeenCalled()) with the setTimeout(, 0) approach then make another "disabled" assertion with the same setTimeout(, 0) setup.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

waitFor will immediately resolve if it passes the first time, so basically there's no waiting.

It makes sense. Thank you!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for coming late for this convo, but yes - what @kevin940726 said 👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want to make this test more resilient to implementation details, then we can first make a "enabled" assertion (.toHaveBeenCalled()) with the setTimeout(, 0) approach then make another "disabled" assertion with the same setTimeout(, 0) setup.

@kevin940726, can you elaborate bit more about this part? 🙇

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure! Because we want to make our unit tests as fast as possible, but we also don't want to just specify a random magic number (0) that could silently break when the implementation details change. Consider the below example:

const TestComponent = ({ enabled }) => {
  data = useEntityRecord( 'root', 'widget', 2, { enabled } );
  return <div />;
};
const UI = ({ enabled }) => (
  <RegistryProvider value={ registry }>
    <TestComponent enabled={enabled} />
  </RegistryProvider>
);

// Smoke test.
const { rerender } = render(<UI enabled={true} />);
await act(() => new Promise(resolve => setTimeout(resolve, 0))); // The minimum delay.
expect(triggerFetch).toHaveBeenCalledTimes(1);

// Real test.
rerender(<UI enabled={false} />);
await act(() => new Promise(resolve => setTimeout(resolve, 0))); // The same delay.
expect(triggerFetch).toHaveBeenCalledTimes(1);

If the minimum delay (implementation detail) is still shorter than setTimeout(,0) then every thing works correctly and the second assertion is enough to test the feature as shown in the table below.

Assertion passes? First assertion Second assertion
enabled:false works ✅ ✅
enabled:false breaks ✅ ❌

However, if the implementation detail changes to a longer delay (one animation frame for instance), the second assertion won't catch the regression and we'll need the first assertion (smoke test) to help us.

Assertion passes? First assertion Second assertion
enabled:false works ❌ ✅
enabled:false breaks ❌ ✅

I think I'm overly complicating this now 😅. In short, the first assertion is just a smoke test to help us make sure the wait is enough for the hook to call apiFetch (when it should).

path: '/wp/v2/widgets/2?context=edit',
} )
);
} );
} );
48 changes: 30 additions & 18 deletions packages/core-data/src/hooks/use-entity-record.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,8 @@ export interface Options {
enabled: boolean;
}

const EMPTY_OBJECT = {};

/**
* Resolves the specified entity record.
*
Expand Down Expand Up @@ -167,24 +169,34 @@ export default function useEntityRecord< RecordType >(
);

const { editedRecord, hasEdits, edits } = useSelect(
( select ) => ( {
editedRecord: select( coreStore ).getEditedEntityRecord(
kind,
name,
recordId
),
hasEdits: select( coreStore ).hasEditsForEntityRecord(
kind,
name,
recordId
),
edits: select( coreStore ).getEntityRecordNonTransientEdits(
kind,
name,
recordId
),
} ),
[ kind, name, recordId ]
( select ) => {
if ( ! options.enabled ) {
return {
editedRecord: EMPTY_OBJECT,
hasEdits: false,
edits: EMPTY_OBJECT,
};
}

return {
editedRecord: select( coreStore ).getEditedEntityRecord(
kind,
name,
recordId
),
hasEdits: select( coreStore ).hasEditsForEntityRecord(
kind,
name,
recordId
),
edits: select( coreStore ).getEntityRecordNonTransientEdits(
kind,
name,
recordId
),
};
},
[ kind, name, recordId, options.enabled ]
);

const { data: record, ...querySelectRest } = useQuerySelect(
Expand Down
Loading