Use consistent terminology across @wordpress/core-data #39349
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
…opriate in the given context
adamziel
added
[Type] Code Quality
adamziel
commented
Mar 10, 2022
| * @param {number} props.id The entity ID. | ||
| * @param {*} props.children The children to wrap. | ||
| * | ||
| * @return {Object} The provided children, wrapped with | ||
| * the entity's context provider. | ||
| */ | ||
| export default function EntityProvider( { kind, type, id, children } ) { | ||
| const Provider = getEntity( kind, type ).context.Provider; | ||
| export default function EntityProvider( { kind, type: name, id, children } ) { |
There was a problem hiding this comment.
It would be nice to refactor it at one point, but that's a part of the public API and beyond the scope of this PR.
|
Size Change: -48 B (0%) Total Size: 1.16 MB
ℹ️ View Unchanged
|
8 tasks
|
I think you need to run I like the idea to unify the terminology. We did a similar round of changes in the |
dmsnell
reviewed
Mar 11, 2022
| @@ -299,13 +299,13 @@ export const editEntityRecord = ( | |||
| edits, | |||
| options = {} | |||
| ) => ( { select, dispatch } ) => { | |||
| const entity = select.getEntity( kind, name ); | |||
| if ( ! entity ) { | |||
| const entityConfig = select.getEntityConfig( kind, name ); | |||
There was a problem hiding this comment.
my goodness this line's change alone makes this PR worthwhile.
| const entities = await dispatch( getKindEntities( kind ) ); | ||
| const entity = find( entities, { kind, name } ); | ||
| const configs = await dispatch( getOrLoadEntitiesConfig( kind ) ); | ||
| const entityConfig = find( configs, { kind, name } ); |
There was a problem hiding this comment.
I wonder if there's a reason we dispatch this action here but rely on the getEntityConfig selector for editEntityRecord, and dispatch again on saveEntityRecord
when deleting and saving we will try and fetch more entity configs if we can't find the given one, but on edit we give up immediately.
There was a problem hiding this comment.
My guess is that by the time we edit an entity record, it must have been resolved, which means that the loading already happened in a different code path. Definitely confusing, though. It would be easier to read if we relied on getOrLoad in editEntityRecord too, even if only for consistency. Let's save it for another PR.
| since: '6.0', | ||
| alternative: "wp.data.select( 'core' ).getEntitiesConfig()", | ||
| } ); | ||
| return getEntitiesConfig( state, kind ); |
There was a problem hiding this comment.
these deprecations are so good. we have too many functions named different things that return the same data
gziolo
reviewed
Mar 11, 2022
|
It looks like this spell worked. I need to try it with my PRs 🤣 |
dmsnell
approved these changes
Mar 11, 2022
There was a problem hiding this comment.
I did some testing on this and would have expected things to collapse if there had been a bug. Specifically I tested FSE and the loading of page templates to run through the kind-loading code.
Please defer to your own judgement and external review; I'm not fully confident in my understanding of the core-data subsystem.
talldan
reviewed
Mar 23, 2022
| */ | ||
| export function useEntityId( kind, type ) { | ||
| return useContext( getEntity( kind, type ).context ); | ||
| export function useEntityProviderId( kind, name ) { |
There was a problem hiding this comment.
Good catch. Yup I can confirm this broke the Navigation block so it will have impact further down the chain.
@adamziel is AFK so I will look into restoring the original and adding a deprecation notice to point to useEntityProviderId
There was a problem hiding this comment.
Also an alternative approach here #39683
There was a problem hiding this comment.
Great catch @talldan, I must have missed the fact this was a public API. Thanks for the prompt reaction and a fix!
There was a problem hiding this comment.
Linking to the related discussion for posterity.
This was referenced Mar 23, 2022
3 tasks
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?
This PR unifies the language used in core-data, leaving only one term for each concept. The most prominent changes are:
Entity loaderreplacedkind,kind config, andentityEntity configreplacedentityandkind entity(in actions, selectors, and resolvers)Entity contextreplacedentity(in the entity provider)Entity namereplacedentity nameandentity type(in the entity provider)state.recordsreplacedstate.dataOther than two deprecations in
selectors.js, this PR aims to keep the public API of the core-data package the same. Every consumer of this package should continue to work in the exact same way.Why?
I walked through that code with @dmsnell a few days ago, and we both got super confused by all the different terms imprecisely referring sometimes to the same ideas, and sometimes to completely different ones. My intention in this PR is to make the immersion easier.
Testing Instructions
cc @gziolo @youknowriad @ellatrix @noisysocks @talldan @getdave @kevin940726 @Mamaduka