Core Data: Update method generating plural names #59881
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Mamaduka
added
[Type] Enhancement
|
Size Change: -10 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
Mamaduka
force-pushed
the
update/store-plural-method-name-generation
branch
from
ccaf2cf
to
6e77f68
Compare
Mamaduka
requested review from
a team,
richtabor,
glendaviesnz,
youknowriad,
jsnajdr,
ramonjd and
tyxla
and removed request for
a team,
richtabor and
glendaviesnz
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
youknowriad
reviewed
Mar 18, 2024
| const entityConfig = rootEntitiesConfig.find( | ||
| ( config ) => config.kind === kind && config.name === name | ||
| ); | ||
| export const getMethodName = ( kind, name, prefix = 'get' ) => { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
JSDoc would be helpful yes
tyxla
reviewed
Mar 18, 2024
There was a problem hiding this comment.
Looks like a nice simplification 👍 Thanks!
Initially, I had concerns, but at a further glance, it seems like this touches on internal bits only and shouldn't cause any issues with 3rd party consumption whatsoever.
I have a few questions, but I believe this should mostly be good to go!
| result[ getMethodName( kind, name, 'get', true ) ] = ( state, query ) => | ||
| selectors.getEntityRecords( state, kind, name, query ); | ||
|
|
||
| if ( plural ) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I guess Riad answered most of my question here - we intentionally don't want plural support for some of the entities.
There was a problem hiding this comment.
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[ pluralMethodName ].shouldInvalidate = ( action ) => | ||
| resolvers.getEntityRecords.shouldInvalidate( action, kind, name ); | ||
|
|
||
| if ( plural ) { |
| @@ -228,6 +232,7 @@ export const rootEntitiesConfig = [ | |||
| kind: 'root', | |||
| baseURL: '/wp/v2/plugins', | |||
| baseURLParams: { context: 'edit' }, | |||
| plural: 'plugins', | |||
There was a problem hiding this comment.
Can we somehow guarantee that if we add a new entity in the future it will also have a plural defined?
There was a problem hiding this comment.
It's not a required property. Maybe converting the file to TS and adding proper types could help us here.
There was a problem hiding this comment.
Since we keep the plural check, we should be good 👍
| usePlural && 'plural' in entityConfig && entityConfig?.plural | ||
| ? pascalCase( entityConfig.plural ) | ||
| : nameSuffix; | ||
| const suffix = pascalCase( name ); |
youknowriad
reviewed
Mar 18, 2024
| @@ -39,6 +39,7 @@ export const rootEntitiesConfig = [ | |||
| 'url', | |||
| ].join( ',' ), | |||
| }, | |||
| plural: '__unstableBases', | |||
There was a problem hiding this comment.
Do we need this for BC? because this particular entity doesn't support multiple records.
There was a problem hiding this comment.
Yes. This is for BC. I mentioned this in the PR description.
There was a problem hiding this comment.
I guess it could use an extra comment that explains why we need it then.
youknowriad
reviewed
Mar 18, 2024
| @@ -63,6 +64,7 @@ export const rootEntitiesConfig = [ | |||
| name: 'site', | |||
| kind: 'root', | |||
| baseURL: '/wp/v2/settings', | |||
| plural: 'sites', | |||
There was a problem hiding this comment.
Same here it seems this entity doesn't really need plural.
youknowriad
approved these changes
Mar 18, 2024
tyxla
approved these changes
Mar 18, 2024
|
@draganescu, you're too fast. I was till working on this PR 😄 |
|
Here's the follow-up with comments - #59946. |
carstingaxion
pushed a commit
to carstingaxion/gutenberg
that referenced
this pull request
Mar 27, 2024
* Core Data: Update method generating plural names * Remove plural flag
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
PR updates the internal
getMethodNamehelper and makes it "pure" by removing the reliance onrootEntitiesConfig. The entity'spluralname needs to be explicitly passed to the method.I've also updated shortcut generation logic to generate plural selectors only when the entity config specifies its existence by defining a
pluralproperty. All existing configurations have also been updated.Why?
This will allow the method to be used for entities defined outside of the
rootEntitiesConfiglist. See #59243.Not every entity supports fetching the list of items (via
getEntityRecords). Good examples are thesiteand__unstableBaseentities. They represent a single item with multiple properties. Running thegetSites()selector does nothing. Updated logic allows configs to opt-out from generating plural shortcuts when the REST API schema doesn't support them.Note: We have to keep
getSitesandgetUnstableBasesaround for backward compatibility.Testing Instructions
CI checks should be passing.
Inspect shortcuts by running
wp.data.select( 'core' )in the DevTools console. Returned selectors should match the trunk.