Core Data: Don't use 'useQuerySelect' in 'useEntityRecord(s)' hooks#76198
Open
Core Data: Don't use 'useQuerySelect' in 'useEntityRecord(s)' hooks#76198
Conversation
Mamaduka
added
[Type] Code Quality
|
Size Change: -6 B (0%) Total Size: 7.69 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 7d5779c. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/23191201475
|
tyxla
reviewed
Mar 16, 2026
|
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. |
Mamaduka
force-pushed
the
refactor/use-entity-record-hooks
branch
from
8d897f5 to
641a083
Compare
tyxla
reviewed
Mar 17, 2026
Member
tyxla
left a comment
tyxla
left a comment
There was a problem hiding this comment.
Thanks for improving the types @Mamaduka, they're much better now that we're using the status mapping for runtime and types 🙌
I have a few more suggestions for simplifying and potentially deduplicating some of the logic. Let me know what you think.
Comment on lines
+2
to
+5
| Idle: 'IDLE', | ||
| Resolving: 'RESOLVING', | ||
| Error: 'ERROR', | ||
| Success: 'SUCCESS', |
Member
There was a problem hiding this comment.
Thanks for improving the types. It does make sense to use both as types and in runtime.
I feel like we might be able to improve this a bit, bear with me:
- Since this is now a regular object that we also use in runtime, can we use lowercase for the keys? That will be more consistent with how we name variables and specifically object keys across the codebase.
- If we consider keys to be lowercase, they will map almost perfectly to the possible resolution states. We just have to rename
successtofinished. - If the statuses here correspond to actual resolution statuses under the hood, we should be able to remove the
switchboilerplate below, and we will only have a fallback for when the state isundefined.
WDYT?
Member
Author
There was a problem hiding this comment.
Good idea, pushed changes based on your feedback. Let me know what you think.
Comment on lines
+194
to
+208
| switch ( resolutionStatus ) { | ||
| case 'resolving': | ||
| status = Status.Resolving; | ||
| break; | ||
| case 'finished': | ||
| status = Status.Success; | ||
| break; | ||
| case 'error': | ||
| status = Status.Error; | ||
| break; | ||
| case undefined: | ||
| default: | ||
| status = Status.Idle; | ||
| break; | ||
| } |
Member
There was a problem hiding this comment.
One thing I liked about using useQuerySelect() was that it was able to contain some of the boilerplate that we're manually adding here (the resolution status mapping, and the isResolving / hasStarted / hasResolved and the rest of the similar flags. However, with the new approach, we're essentially duplicating that logic. Can we figure out a way to avoid that duplication? I've already suggested a potential improvement that might simplify the mapping, but we still need to handle the flags, and duplicating that isn't ideal.
Member
Author
There was a problem hiding this comment.
I think it has pros and cons. For example, because the resolution status was automatically available, it was made into the useResourcePermissions hook. Using the resolution state with permission isn't a good idea; the user either has permission or does not. Apps don't display an intermediate state until waiting for the server.
I plan to address it in a separate PR with useResourcePermissions refactoring.
Mamaduka
force-pushed
the
refactor/use-entity-record-hooks
branch
from
7d5779c to
af01843
Compare
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
2 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?
Part one of #64170.
PR moves
useQuerySelectfrom 'useEntityRecord(s)' hooks in favor of plainuseSelect.Why?
useSelectthat query select abstraction. I think having a seconduseSelectin both hooks demonstrates that.Testing Instructions
AI usage
Use Claude to fix type errors.