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

Core Data: Add batched variants for start and finish resolution actions #31005

Merged
merged 2 commits into from
Apr 22, 2021
Merged

Core Data: Add batched variants for start and finish resolution actions #31005

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a40911a
Add batched variants for start/finish resolution actions
fluiddot Apr 20, 2021
9e19ef1
CHANGELOG files of core-data and data packages updated
fluiddot Apr 20, 2021
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
1 change: 1 addition & 0 deletions packages/core-data/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
<!-- Learn how to maintain this file at https://github.com/WordPress/gutenberg/tree/HEAD/packages#maintaining-changelogs. -->

## Unreleased
- The `getEntityRecords` resolver has been updated and now uses the batched variants of start and finish resolution actions.

## 2.26.0 (2021-03-17)

Expand Down
28 changes: 14 additions & 14 deletions packages/core-data/src/resolvers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,20 +219,20 @@ export function* getEntityRecords( kind, name, query = {} ) {
// See https://github.com/WordPress/gutenberg/pull/26575
if ( ! query?._fields ) {
const key = entity.key || DEFAULT_ENTITY_KEY;
for ( const record of records ) {
if ( record[ key ] ) {
yield {
type: 'START_RESOLUTION',
selectorName: 'getEntityRecord',
args: [ kind, name, record[ key ] ],
};
yield {
type: 'FINISH_RESOLUTION',
selectorName: 'getEntityRecord',
args: [ kind, name, record[ key ] ],
};
}
}
const resolutionsArgs = records
.filter( ( record ) => record[ key ] )
.map( ( record ) => [ kind, name, record[ key ] ] );

yield {
type: 'START_RESOLUTIONS',
selectorName: 'getEntityRecord',
args: resolutionsArgs,
};
yield {
type: 'FINISH_RESOLUTIONS',
selectorName: 'getEntityRecord',
args: resolutionsArgs,
};
}
} finally {
yield* __unstableReleaseStoreLock( lock );
Expand Down
8 changes: 4 additions & 4 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 @@ -136,14 +136,14 @@ describe( 'getEntityRecords', () => {

// It should mark the entity record that has an ID as resolved
expect( fulfillment.next().value ).toEqual( {
type: 'START_RESOLUTION',
type: 'START_RESOLUTIONS',
selectorName: 'getEntityRecord',
args: [ ENTITIES[ 1 ].kind, ENTITIES[ 1 ].name, 2 ],
args: [ [ ENTITIES[ 1 ].kind, ENTITIES[ 1 ].name, 2 ] ],
} );
expect( fulfillment.next().value ).toEqual( {
type: 'FINISH_RESOLUTION',
type: 'FINISH_RESOLUTIONS',
selectorName: 'getEntityRecord',
args: [ ENTITIES[ 1 ].kind, ENTITIES[ 1 ].name, 2 ],
args: [ [ ENTITIES[ 1 ].kind, ENTITIES[ 1 ].name, 2 ] ],
} );
} );
} );
Expand Down
1 change: 1 addition & 0 deletions packages/data/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
<!-- Learn how to maintain this file at https://github.com/WordPress/gutenberg/tree/HEAD/packages#maintaining-changelogs. -->

## Unreleased
- Added new `startResolutions` and `finishResolutions` actions as batched variants of `startResolution` and `finishResolutions` actions.

## 4.27.0 (2021-03-17)

Expand Down
36 changes: 36 additions & 0 deletions packages/data/src/redux-store/metadata/actions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,42 @@ export function finishResolution( selectorName, args ) {
};
}

/**
* Returns an action object used in signalling that a batch of selector resolutions has
* started.
*
* @param {string} selectorName Name of selector for which resolver triggered.
* @param {...*} args Array of arguments to associate for uniqueness, each item
* is associated to a resolution.
*
* @return {Object} Action object.
*/
export function startResolutions( selectorName, args ) {
Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible to refactor existing actions (finishResolution and finishResolution) to accept not only a single resolution but also multiple resolutions when necessary? This way we wouldn't have to create new methods.

Copy link
Member

Choose a reason for hiding this comment

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

How would we determine what the caller intends to do? args is already an array, and for finishResultions it's an array of arrays.

In my view two separate functions are easier to understand. And easier to provide types for.

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 thought about that option but I didn't want to overuse the args parameter, besides since args can be anything it can be hard to infer when it's being dispatched with single/multiple resolutions. One option we might consider is to add an extra boolean parameter that specifies if the args contain single/multiple resolutions.

Copy link
Member

@gziolo gziolo Apr 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Right, it's very hard to detect whether it's single or multiple items because args can be any number of params of any type.

In startResolutions and finishResolutions we should use a different name than args to distinguish between single and multiple items in both functions. The type is going to be more complex as well.

I think it's better to add new functions if the only alternative is a boolean param.

return {
type: 'START_RESOLUTIONS',
selectorName,
args,
};
}

/**
* Returns an action object used in signalling that a batch of selector resolutions has
* completed.
*
* @param {string} selectorName Name of selector for which resolver triggered.
* @param {...*} args Array of arguments to associate for uniqueness, each item
* is associated to a resolution.
*
* @return {Object} Action object.
*/
export function finishResolutions( selectorName, args ) {
return {
type: 'FINISH_RESOLUTIONS',
selectorName,
args,
};
}

/**
* Returns an action object used in signalling that we should invalidate the resolution cache.
*
Expand Down
11 changes: 11 additions & 0 deletions packages/data/src/redux-store/metadata/reducer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,15 @@ const subKeysIsResolved = onSubKey( 'selectorName' )(
nextState.set( action.args, isStarting );
return nextState;
}
case 'START_RESOLUTIONS':
case 'FINISH_RESOLUTIONS': {
const isStarting = action.type === 'START_RESOLUTIONS';
const nextState = new EquivalentKeyMap( state );
for ( const resolutionArgs of action.args ) {
nextState.set( resolutionArgs, isStarting );
}
return nextState;
}
case 'INVALIDATE_RESOLUTION': {
const nextState = new EquivalentKeyMap( state );
nextState.delete( action.args );
Expand Down Expand Up @@ -60,6 +69,8 @@ const isResolved = ( state = {}, action ) => {
: state;
case 'START_RESOLUTION':
case 'FINISH_RESOLUTION':
case 'START_RESOLUTIONS':
case 'FINISH_RESOLUTIONS':
case 'INVALIDATE_RESOLUTION':
return subKeysIsResolved( state, action );
}
Expand Down
Loading