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

Jump to bottom

Core Data: Update method generating plural names #59881

Merged
merged 2 commits into from
Mar 18, 2024
Merged

Core Data: Update method generating plural names #59881

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
ca35372
Core Data: Update method generating plural names
Mamaduka Mar 14, 2024
6e77f68
Remove plural flag
Mamaduka Mar 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
30 changes: 11 additions & 19 deletions packages/core-data/src/entities.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ export const rootEntitiesConfig = [
'url',
].join( ',' ),
},
plural: '__unstableBases',
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this for BC? because this particular entity doesn't support multiple records.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. This is for BC. I mentioned this in the PR description.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ooops, sorry :P

Copy link
Member

Choose a reason for hiding this comment

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

I guess it could use an extra comment that explains why we need it then.

syncConfig: {
fetch: async () => {
return apiFetch( { path: '/' } );
Expand All @@ -63,6 +64,7 @@ export const rootEntitiesConfig = [
name: 'site',
kind: 'root',
baseURL: '/wp/v2/settings',
plural: 'sites',
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here it seems this entity doesn't really need plural.

Copy link
Member Author

Choose a reason for hiding this comment

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

Needed for BC as well.

getTitle: ( record ) => {
return record?.title ?? __( 'Site Title' );
},
Expand Down Expand Up @@ -92,6 +94,7 @@ export const rootEntitiesConfig = [
key: 'slug',
baseURL: '/wp/v2/types',
baseURLParams: { context: 'edit' },
plural: 'postTypes',
syncConfig: {
fetch: async ( id ) => {
return apiFetch( {
Expand Down Expand Up @@ -220,6 +223,7 @@ export const rootEntitiesConfig = [
kind: 'root',
baseURL: '/wp/v2/themes',
baseURLParams: { context: 'edit' },
plural: 'themes',
key: 'stylesheet',
},
{
Expand All @@ -228,6 +232,7 @@ export const rootEntitiesConfig = [
kind: 'root',
baseURL: '/wp/v2/plugins',
baseURLParams: { context: 'edit' },
plural: 'plugins',
Copy link
Member

Choose a reason for hiding this comment

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

Can we somehow guarantee that if we add a new entity in the future it will also have a plural defined?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not a required property. Maybe converting the file to TS and adding proper types could help us here.

Copy link
Member

Choose a reason for hiding this comment

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

Since we keep the plural check, we should be good 👍

key: 'plugin',
},
{
Expand Down Expand Up @@ -408,32 +413,19 @@ async function loadTaxonomyEntities() {
* const nameSingular = getMethodName( 'root', 'theme', 'get' );
* // nameSingular is getRootTheme
*
* const namePlural = getMethodName( 'root', 'theme', 'set' );
* const namePlural = getMethodName( 'root', 'themes', 'set' );
* // namePlural is setRootThemes
* ```
*
* @param {string} kind Entity kind.
* @param {string} name Entity name.
* @param {string} prefix Function prefix.
* @param {boolean} usePlural Whether to use the plural form or not.
* @param {string} kind Entity kind.
* @param {string} name Entity name.
* @param {string} prefix Function prefix.
*
* @return {string} Method name
*/
export const getMethodName = (
kind,
name,
prefix = 'get',
usePlural = false
) => {
const entityConfig = rootEntitiesConfig.find(
( config ) => config.kind === kind && config.name === name
);
export const getMethodName = ( kind, name, prefix = 'get' ) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

So the "name" here is actually not the name of the entity but a plural form already. It was not clear to me when reading the diff and may confuse people. Maybe we should rename the argument?

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 this is more like name or pluralName. Would updating method arg and JSDoc would be helpful? We could use nameOrPluralName or something similar.

Copy link
Contributor

Choose a reason for hiding this comment

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

JSDoc would be helpful yes

const kindPrefix = kind === 'root' ? '' : pascalCase( kind );
const nameSuffix = pascalCase( name ) + ( usePlural ? 's' : '' );
const suffix =
usePlural && 'plural' in entityConfig && entityConfig?.plural
? pascalCase( entityConfig.plural )
: nameSuffix;
const suffix = pascalCase( name );
Copy link
Member

Choose a reason for hiding this comment

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

Neat simplification here 👍

return `${ prefix }${ kindPrefix }${ suffix }`;
};

Expand Down
24 changes: 15 additions & 9 deletions packages/core-data/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,23 +22,29 @@ import { unlock } from './private-apis';
// The "kind" and the "name" of the entity are combined to generate these shortcuts.

const entitySelectors = rootEntitiesConfig.reduce( ( result, entity ) => {
const { kind, name } = entity;
const { kind, name, plural } = entity;
result[ getMethodName( kind, name ) ] = ( state, key, query ) =>
selectors.getEntityRecord( state, kind, name, key, query );
result[ getMethodName( kind, name, 'get', true ) ] = ( state, query ) =>
selectors.getEntityRecords( state, kind, name, query );

if ( plural ) {
Copy link
Member

Choose a reason for hiding this comment

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

Why is this check necessary? Don't all root entities have plural now? Is it in case we add an entity in the future and it doesn't have a plural defined?

Copy link
Member

Choose a reason for hiding this comment

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

I guess Riad answered most of my question here - we intentionally don't want plural support for some of the entities.

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 mentioned this in the description, but I probably should've added an inline comment as well. Not every entity will have "plural" selectors. Current cases are site and __unstableBase, but we have to maintain them for BC.

result[ getMethodName( kind, plural, 'get' ) ] = ( state, query ) =>
selectors.getEntityRecords( state, kind, name, query );
}
return result;
}, {} );

const entityResolvers = rootEntitiesConfig.reduce( ( result, entity ) => {
const { kind, name } = entity;
const { kind, name, plural } = entity;
result[ getMethodName( kind, name ) ] = ( key, query ) =>
resolvers.getEntityRecord( kind, name, key, query );
const pluralMethodName = getMethodName( kind, name, 'get', true );
result[ pluralMethodName ] = ( ...args ) =>
resolvers.getEntityRecords( kind, name, ...args );
result[ pluralMethodName ].shouldInvalidate = ( action ) =>
resolvers.getEntityRecords.shouldInvalidate( action, kind, name );

if ( plural ) {
Copy link
Member

Choose a reason for hiding this comment

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

Same as above.

const pluralMethodName = getMethodName( kind, plural, 'get' );
result[ pluralMethodName ] = ( ...args ) =>
resolvers.getEntityRecords( kind, name, ...args );
result[ pluralMethodName ].shouldInvalidate = ( action ) =>
resolvers.getEntityRecords.shouldInvalidate( action, kind, name );
}
return result;
}, {} );

Expand Down
8 changes: 1 addition & 7 deletions packages/core-data/src/test/entities.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,8 @@ describe( 'getMethodName', () => {
expect( methodName ).toEqual( 'setPostType' );
} );

it( 'should use the plural form', () => {
const methodName = getMethodName( 'root', 'postType', 'get', true );

expect( methodName ).toEqual( 'getPostTypes' );
} );

it( 'should use the given plural form', () => {
const methodName = getMethodName( 'root', 'taxonomy', 'get', true );
const methodName = getMethodName( 'root', 'taxonomies', 'get' );

expect( methodName ).toEqual( 'getTaxonomies' );
} );
Expand Down