-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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.
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
Stabilize useEntityRecord and useEntityRecords #40162
Conversation
Size Change: +31 B (0%) Total Size: 1.23 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ont
* @template RecordType | ||
*/ | ||
export default function __experimentalUseEntityRecord< RecordType >( | ||
export default function useEntityRecord< RecordType >( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we not leave the __experimentalUseEntityRecord
in place for a bit but add a deprecation()
to it so people get some warning to switch over if they are using it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question, but isn't the purpose of __experimental to not require deprecation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that? I know part of the intention is to lead people to avoid using those but I think the reality is that many do.
It's not my call and I don't really know our polity (maybe a good update to the docs is in store) but personally I think we can avoid a lot of harm and loss of trust if we do give a due period of deprecation for the core APIs. If after six months we remove something that's been deprecated then at least we gave notice and opportunity to update code. Things move slow even when things move fast.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea with the __experimental
APIs is to not have people using them outside of the Gutenberg repository until they stabilize:
An experimental or unstable API is named as such to communicate instability of a function whose interface is not yet finalized. Aside from references within the code, these APIs should neither be documented nor mentioned in any CHANGELOG. They should effectively be considered to not exist from an external perspective. In most cases, they should only be exposed to satisfy requirements between packages maintained in this repository.
There's a related discussion going on in #40316
Technically it wouldn't be too hard to add an additional __experimental
export that calls the underlying function with a deprecation notice, but it is not how other __experimental APIs are handled right now. I'd move forward here as is, and then have the deprecation policy discussion separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it won't harm to add the deprecation notice for the old method name so it continues to work if someone uses it and remove it with WordPress 6.1 or 6.2.
…ds hook is disabled
ec99caf
to
dd71452
Compare
I addressed all the feedback. I think we're good now @gziolo @dmsnell @ZebulanStanphill @draganescu |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than adding deprecation notice for the old method names, I think this PR is more than expected 👍🏻
6b7449d
to
2f6b479
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
One thing made me wonder. Why those 2 new methods aren't included in any documentation? Does it mean, Let's included it also in https://github.com/WordPress/gutenberg/blob/trunk/packages/core-data/CHANGELOG.md in the "New Features" section. |
Dev note proposed at https://make.wordpress.org/core/?p=99829&preview=1&_ppp=2b1db975ea |
Description
Part of #38135, related to #40162
Stabilizes the
__experimentalUseEntityRecord
and__experimentalUseEntityRecords
hooks. They have been used in other core packages without any problems.This Pull Request does the following:
enum Status
toconst enum Status
to reduce the footprint of generated code at no additional cost, as @dmsnell proposed in Propose useEntityRecords (experimental) #38782Test plan
Confirm all the checks are green – no runtime logic is expected to change as a result of this PR aside of the EMPTY_ARRAY adjustment.