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

Formalise deprecation of useEntityId hook in favour of useEntityProviderId #39681

Closed
wants to merge 2 commits into from
Closed

Formalise deprecation of useEntityId hook in favour of useEntityProviderId #39681

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5aed539
Deprecate and proxy to alternative
getdave Mar 23, 2022
112d62b
Add the @deprecated comment in docblock
getdave Mar 23, 2022
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
18 changes: 18 additions & 0 deletions packages/core-data/src/entity-provider.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
} from '@wordpress/element';
import { useSelect, useDispatch } from '@wordpress/data';
import { parse, __unstableSerializeAndClean } from '@wordpress/blocks';
import deprecated from '@wordpress/deprecated';

/**
* Internal dependencies
Expand Down Expand Up @@ -71,6 +72,23 @@ export default function EntityProvider( { kind, type: name, id, children } ) {
return <Provider value={ id }>{ children }</Provider>;
}

/**
* Hook that returns the ID for the nearest
* provided entity of the specified type.
*
* @deprecated since 12.9. Callers should use the useEntityProviderId hook instead.
*
* @param {string} kind The entity kind.
* @param {string} type The entity type.
*/
export function useEntityId( kind, type ) {
deprecated( 'the @wordpress/core-data useEntityId() hook', {
since: '6.0',
alternative: 'the @wordpress/core-data useEntityProviderId() hook',
} );
return useEntityProviderId( kind, type );
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

I disagree, I think we should just restore the old name here.

Copy link
Contributor

Choose a reason for hiding this comment

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

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 :)

Copy link
Contributor Author

@getdave getdave Mar 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK here's the alternative. Please let me know which route we'd like to take.

Copy link
Contributor

@adamziel adamziel Mar 31, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@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!

Copy link
Contributor

Choose a reason for hiding this comment

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

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)

}

/**
* Hook that returns the ID for the nearest
* provided entity of the specified type.
Expand Down