Propose useEntityRecords (experimental) #38782
Merged
Conversation
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
adamziel
added
[Package] Core data
This was referenced Feb 14, 2022
Closed
adamziel
commented
Feb 14, 2022
| if ( isResolving ) { | ||
| status = Status.Resolving; | ||
| } else if ( hasResolved ) { | ||
| if ( data ) { |
Contributor
Author
There was a problem hiding this comment.
That's far from ideal, but it's the best we can do until there's a better resolution failure handling in place. The only possible upgrade I see at the moment is checking for === undefined or === null.
|
Size Change: +639 B (0%) Total Size: 1.15 MB
ℹ️ View Unchanged
|
gziolo
approved these changes
Feb 15, 2022
Member
gziolo
left a comment
gziolo
left a comment
There was a problem hiding this comment.
This is looking great. I left some non-blocking comments where I had some minor concerns. Anyway, I would love to see the code refactored to see how useEntityRecords and useEntityRecord work in action so we could learn about the next steps. Maybe it's good enough to call it stable and remove __experimental prefix? 😄
Contributor
|
@dmsnell I'm curious if you've had a chance to look at this and have any insight from a typing perspective? |
Contributor
Author
|
@sarayourfriend related discussion about typing core entity records: #38666 |
Contributor
|
Oh wonderful, thanks for sharing that! |
dmsnell
reviewed
Feb 15, 2022
dmsnell
reviewed
Feb 15, 2022
dmsnell
reviewed
Feb 15, 2022
dmsnell
reviewed
Feb 15, 2022
dmsnell
reviewed
Feb 15, 2022
dmsnell
reviewed
Feb 15, 2022
dmsnell
reviewed
Feb 15, 2022
| export default function __experimentalUseEntityRecords< RecordType >( | ||
| kind: string, | ||
| name: string, | ||
| queryArgs: unknown = {} |
Member
There was a problem hiding this comment.
how hard would it be to surface a type of queryArgs here from wherever it's consumed? if not, can we add a link in the JSDoc comment to point people to where they can find out? I know it's passed-through to getEntityRecords but unknown isn't that helpful.
Given that these are just passed through we might be able to reuse the types from getEntityRecords. I'm curious if the rename from data to records is fully worth it considering that it incurs a needless object clone, but I get how it's nice to match with useEntityRecord
interface EntityRecordHook<RecordType> {
(kind: string; name: string; queryArgs: HttpQuery) => EntityRecordResolution<RecordType>;
}
// use-entity-record.ts
export default useEntityRecord: EntityRecordHook = (kind, name, queryArgs) => …;
// use-entity-records.ts
export default useEntityRecords: EntityRecordHook = …EntityRecordHook.mov
Contributor
Author
There was a problem hiding this comment.
how hard would it be to surface a type of queryArgs here from wherever it's consumed? if not, can we add a link in the JSDoc comment to point people to where they can find out? I know it's passed-through to getEntityRecords but unknown isn't that helpful. Given that these are just passed through we might be able to reuse the types from getEntityRecords. I
Good idea! The EntityQuery type from #40024 would make a good fit here once that PR lands.
I'm curious if the rename from data to records is fully worth it considering that it incurs a needless object clone, but I get how it's nice to match with useEntityRecord
It doesn't match, though 😅 it is record and records. The reason I went for that versus data is for the sake of the consumer of this hook. Referring to a record inside of a component is clearer than referring to an opaque data. I am not a fan of that destructuring, though.
Contributor
Author
|
Great notes @dmsnell, thank you! I will address them in the upcoming PR turning this into a stable API. |
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.
Description
This PR is a minimal subset of #38135 focused solely on the
useEntityRecordshook. It directly follows up on the singular useEntityRecord:The goal of these new APIs is to lower the barrier of entry for new contributors and make the life of existing contributors easier.
Example usage:
Before
After:
See also how the combination of all proposed hooks makes the navigation block ~200 lines leaner.
Test plan
Other considerations
useQuerySelectis used as an internal API for bothuseEntityRecordanduseEntityRecordshooks. It might be useful in the potentialusePermissions()anduseEntityMutation()hooks (to be proposed). There is a separate discussion about making it a public API on@wordpress/datagoing on in #38134cc @kevin940726 @talldan @gziolo @draganescu @ellatrix @noisysocks @jsnajdr @getdave @Mamaduka @spencerfinnell