Skip to content

getEntityRecords: batch actions #60591

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

Merged
ellatrix merged 3 commits into from
Apr 9, 2024
Merged

getEntityRecords: batch actions #60591

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
20126a8
getEntityRecords: batch actions
ellatrix Apr 9, 2024
5d8bf1b
Redo
ellatrix Apr 9, 2024
d41dfef
Fix unit tests
ellatrix Apr 9, 2024
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
64 changes: 34 additions & 30 deletions packages/core-data/src/resolvers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ export const getEditedEntityRecord = forwardResolver( 'getEntityRecord' );
*/
export const getEntityRecords =
( kind, name, query = {} ) =>
async ( { dispatch } ) => {
async ( { dispatch, registry } ) => {
const configs = await dispatch( getOrLoadEntitiesConfig( kind, name ) );
const entityConfig = configs.find(
( config ) => config.name === name && config.kind === kind
Expand Down Expand Up @@ -261,37 +261,41 @@ export const getEntityRecords =
} );
}

dispatch.receiveEntityRecords(
kind,
name,
records,
query,
false,
undefined,
meta
);
registry.batch( () => {
dispatch.receiveEntityRecords(
kind,
name,
records,
query,
false,
undefined,
meta
);

// When requesting all fields, the list of results can be used to
// resolve the `getEntityRecord` selector in addition to `getEntityRecords`.
// See https://github.com/WordPress/gutenberg/pull/26575
if ( ! query?._fields && ! query.context ) {
const key = entityConfig.key || DEFAULT_ENTITY_KEY;
const resolutionsArgs = records
.filter( ( record ) => record[ key ] )
.map( ( record ) => [ kind, name, record[ key ] ] );
// When requesting all fields, the list of results can be used to
// resolve the `getEntityRecord` selector in addition to `getEntityRecords`.
// See https://github.com/WordPress/gutenberg/pull/26575
if ( ! query?._fields && ! query.context ) {
const key = entityConfig.key || DEFAULT_ENTITY_KEY;
const resolutionsArgs = records
.filter( ( record ) => record[ key ] )
.map( ( record ) => [ kind, name, record[ key ] ] );

dispatch( {
type: 'START_RESOLUTIONS',
selectorName: 'getEntityRecord',
args: resolutionsArgs,
} );
dispatch( {
type: 'FINISH_RESOLUTIONS',
selectorName: 'getEntityRecord',
args: resolutionsArgs,
} );
Copy link
Copy Markdown
Member

@jsnajdr jsnajdr Apr 9, 2024

Choose a reason for hiding this comment

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

If the start and finish actions are batched, then there's no point dispatching START_RESOLUTIONS. The state will be overwritten by FINISH_RESOLUTIONS, and no listener will see the state between start and finish because listeners are not called during a batch.

If some listener is observing the resolution state and expects to see the full cycle, undefined -> resolving -> finished, it might get suprised that it won't ever see resolving. It will go directly to finished. Not sure how much breaking this is.

Copy link
Copy Markdown
Contributor

@youknowriad youknowriad Apr 9, 2024

Choose a reason for hiding this comment

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

I think it probably doesn't really make sense to call "start" in this case, we just want to make these as resolved to avoid requests.

}

dispatch( {
type: 'START_RESOLUTIONS',
selectorName: 'getEntityRecord',
args: resolutionsArgs,
} );
dispatch( {
type: 'FINISH_RESOLUTIONS',
selectorName: 'getEntityRecord',
args: resolutionsArgs,
} );
}
} finally {
dispatch.__unstableReleaseStoreLock( lock );
} );
} catch ( e ) {
Copy link
Copy Markdown
Member

@Mamaduka Mamaduka Jun 5, 2024

Choose a reason for hiding this comment

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

I think we should switch this back to try...finally. The catching error here means it won't be passed to the resolution state (getResolutionError) and might affect the error state for useSuspenseSelect. (#37261).

But that means we won't be able to batch dispatch.__unstableReleaseStoreLock.

gutenberg/packages/data/src/redux-store/index.js

Lines 533 to 535 in 0d1943c

if ( selectors.hasResolutionFailed( selectorName, args ) ) {
throw selectors.getResolutionError( selectorName, args );
}

dispatch.__unstableReleaseStoreLock( lock );
}
};
Expand Down
7 changes: 4 additions & 3 deletions packages/core-data/src/test/resolvers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,7 @@ describe( 'getEntityRecords', () => {
baseURLParams: { context: 'edit' },
},
];
const registry = { batch: ( callback ) => callback() };

beforeEach( async () => {
triggerFetch.mockReset();
Expand All @@ -160,7 +161,7 @@ describe( 'getEntityRecords', () => {
// Provide response
triggerFetch.mockImplementation( () => POST_TYPES );

await getEntityRecords( 'root', 'postType' )( { dispatch } );
await getEntityRecords( 'root', 'postType' )( { dispatch, registry } );

// Fetch request should have been issued.
expect( triggerFetch ).toHaveBeenCalledWith( {
Expand Down Expand Up @@ -191,7 +192,7 @@ describe( 'getEntityRecords', () => {
// Provide response
triggerFetch.mockImplementation( () => POST_TYPES );

await getEntityRecords( 'root', 'postType' )( { dispatch } );
await getEntityRecords( 'root', 'postType' )( { dispatch, registry } );

// Fetch request should have been issued.
expect( triggerFetch ).toHaveBeenCalledWith( {
Expand Down Expand Up @@ -221,7 +222,7 @@ describe( 'getEntityRecords', () => {
// Provide response
triggerFetch.mockImplementation( () => POST_TYPES );

await getEntityRecords( 'root', 'postType' )( { dispatch } );
await getEntityRecords( 'root', 'postType' )( { dispatch, registry } );

// Fetch request should have been issued.
expect( triggerFetch ).toHaveBeenCalledWith( {
Expand Down