Formalise deprecation of useEntityId hook in favour of useEntityProviderId#39681
Closed
Formalise deprecation of useEntityId hook in favour of useEntityProviderId#39681
useEntityId hook in favour of useEntityProviderId#39681Conversation
Contributor
Author
|
Pinging @draganescu who is leading the release of |
youknowriad
reviewed
Mar 23, 2022
| since: '6.0', | ||
| alternative: 'the @wordpress/core-data useEntityProviderId() hook', | ||
| } ); | ||
| return useEntityProviderId( kind, type ); |
Contributor
There was a problem hiding this comment.
I'm curious why we decided to change the name here. For me "Provider" is just an implementation detail (React context) and what we really do with this hook is to retrieve the "entityId". so I think the old naming is better here.
Contributor
Author
There was a problem hiding this comment.
I don't have the context. Adam or Greg might know but they are unavailable.
Would you prefer we just renamed back to useEntityId()? That would avoid the whole deprecation thing.
Contributor
There was a problem hiding this comment.
For now we can move on with the deprecation notice as the other PR has been through longer discussions and we may miss some context.
Contributor
There was a problem hiding this comment.
I disagree, I think we should just restore the old name here.
Contributor
There was a problem hiding this comment.
I didn't find any thing clarifying the reasoning for this particular hook name change and since it's a breaking change, I'd rather revert instead of adding a deprecation and then later a deprecation that deprecate the deprecation :)
Contributor
Author
There was a problem hiding this comment.
It does seem to be the only instance where the "provider" term is included in the public API (apart from the actual Provider itself). I can't see renaming back being a huge problem.
Probably safest to rename back and then ask Adam, Dennis and Greg to re-review when they are available. If they really want to rename it they can always submit a followup with the appropriate rationale for the next release.
Contributor
Author
There was a problem hiding this comment.
OK here's the alternative. Please let me know which route we'd like to take.
Contributor
There was a problem hiding this comment.
@youknowriad @getdave @draganescu it was a part of #39349. Why? The name useEntityId() is slightly off semantically. An Entity is a higher-level idea that refers to Comments and Posts and such. An entity is not a data container, though, and does not store a numerical ID. That would be an Entity Record. In this case, though, the ID does not even come from the record but from the Entity Provider – hence the name change to clarify that.
I missed the fact that this function was a part of the public API, though. I specifically wanted to avoid any public-facing changes and even made a note about that. Thank you for the prompt reaction and a fix here!
Contributor
There was a problem hiding this comment.
I agree that "entity" is not the right semantic here but it's a bit better than "provider" for me. Ideally yet it should be useEntityRecordId the provider should be EntityRecordProvider. We can make these changes (and deprecate the old ones if deemed necessary)
|
Size Change: +60 B (0%) Total Size: 1.21 MB
ℹ️ View Unchanged
|
Contributor
Author
|
Closing in favour of #39683 |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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?
Alternative to #39683
This PR fully handles the formal deprecation of
useEntityIdin favour ofuseEntityProviderId. This was undertaken in #39349 but that PR didn't handle the deprecation process.Why?
This hook is part of the public API of
@wordpress/core-databut it has been removed in favour ofuseEntityProviderIdwithout the formal deprecation process.This was caught as part of a fix to the Navigation block which broke when the hook was removed.
How?
This PR
@wordpress/core-datadeprecation()call.I believe the change of arg signature from
typetonameis purely nomenclature, but it would be worth validating I have understood this correctly.Testing Instructions
Start emptyAdvancedpanel of block's Inspector controlstrunkit crashes)Screenshots or screencast