useEntityRecord (experimental) #38522
Conversation
|
Size Change: +895 B (0%) Total Size: 1.15 MB
ℹ️ View Unchanged
|
447b5f2 to
5ae67d7
Compare
906b53c to
d6b4daf
Compare
| hasEdits, | ||
| isMissing, | ||
| isResolving, | ||
| hasResolved, |
There was a problem hiding this comment.
I think we need to also expose error for error handling. Or if it's handled in data then I think it makes sense to separate them into different variables.
There was a problem hiding this comment.
Do you mean isError or error details? I don't think we can access the latter, any ideas?
There was a problem hiding this comment.
I mean the error details. If we don't have access to the error then I think we should definitely add it 😆 , not in this PR though.
There was a problem hiding this comment.
Agreed! It is a quite old problem, too – it would be great to get it fixed:
gutenberg/packages/core-data/src/resolvers.js
Lines 107 to 111 in 70730e6
I wonder what is the right level of abstraction here. Core-data could be selfish and introduce an action like setEntityResolutionError, but that name is already very telling. Resolution is a @wordpress/data concept, so perhaps there could be a meta action available like failResolution, similarly to how we have finishResolution?
Taking it even further, we lack try/catch around the resolver call:
gutenberg/packages/data/src/redux-store/index.js
Lines 414 to 428 in 70730e6
This sends all exceptions straight into the browser's console. I don't see how that's useful. Perhaps we could wrap it in try/catch and handle exceptions at that level? cc @gziolo @youknowriad
There was a problem hiding this comment.
I agree that error handling in core-data is not great at the moment, we need to add the errors to the resolution metadata state, add some selectors for it and yeah seems like a good addition to useEntityRecord or useQuerySelect as well.
There was a problem hiding this comment.
I started some explorations in #38669 – let's move the discussion there
|
This is close. I would be happy to see it landed once we finalize all the little things. Thank you for breaking the previous work into smaller pieces that should speed up the process once we land this PR ❤️ |
| @@ -0,0 +1,7 @@ | |||
| /* eslint-disable-next-line no-shadow */ | |||
There was a problem hiding this comment.
Why do we have to disable this rule, a bug in eslint?
There was a problem hiding this comment.
I think so, it just keeps saying ESLint: 'Status' is already declared in the upper scope on line 1 column 13.(no-shadow
There's a related discussion on SO: https://stackoverflow.com/questions/63961803/eslint-says-all-enums-in-typescript-app-are-already-declared-in-the-upper-scope
Perhaps we could disable the no-shadow and enable @typescript-eslint/no-shadow at the global level, but that seems beyond the scope of this PR so I went for eslint-disable for now.
There was a problem hiding this comment.
Perhaps we could disable the no-shadow and enable @typescript-eslint/no-shadow at the global level, but that seems beyond the scope of this PR so I went for eslint-disable for now.
It isn't the first time we would do use the rule from TS version of ESLint 👍🏻
There was a problem hiding this comment.
Here's a PR to adjust the eslint rules: #38665
|
|
||
| interface EntityRecordResolution { | ||
| /** The requested entity record */ | ||
| record: Object; |
There was a problem hiding this comment.
Object is not a type that we'd like to use in this context, it behaves similarly to any that it just silent the errors without any actual gains.
Ideally, we should already have all the types of core entities available somewhere so that we can do this automatically. Given that it'd be a huge amount of work, we could instead try to use generics here to let the users specify the correct types.
const { record } = useEntityRecord<PageRecord>( 'postType', 'page', id );
// record: PageRecordIt's a bit more complicated to get right though, I'm willing to help if you encounter any problems!
There was a problem hiding this comment.
Also, if the data is still resolving, I think the type of this value will be null?
There was a problem hiding this comment.
Object is not a type that we'd like to use in this context, it behaves similarly to any that it just silent the errors without any actual gains.
The gains are that we're able to use TypeScript :-) I went for the minimal typing on purpose for the reasons you have mentioned – there aren't many types defined in core-data. Thinking about it more, it shouldn't be a big problem for the JavaScript consumers of the code, and if it is then any may be moved to the consumer code. So yes, generics sound like a good approach 👍
There was a problem hiding this comment.
Hey @kevin940726, you will like this PR: TypeScript definitions for core-data entity records
| isResolving: boolean; | ||
|
|
||
| /** Is the record resolved by now? */ | ||
| hasResolved: boolean; |
There was a problem hiding this comment.
I guess we don't need this either, as this is essentially the same as !isResolving.
There was a problem hiding this comment.
Not exactly, isResolving and hasResolved may both be true when the record is being re-requested. It is how the meta selectors in @wordpress/data work so I'd rather not change that logic here. I thought it's confusing and now your comment reassured me, so I'll add that to the docstring.
There was a problem hiding this comment.
isResolvingandhasResolvedmay both be true when the record is being re-requested.
Huh, interesting! Then what's the difference between hasResolved and !!data?
There was a problem hiding this comment.
Just checked the implementation in:
gutenberg/packages/data/src/redux-store/metadata/selectors.js
Lines 53 to 69 in b51dbd6
isResolvingandhasResolvedmay both be true
I think this is not possible? Or maybe I'm missing something elsewhere 🤔 . I guess something like isSuccess and isError might be more useful than hasResolved. It's also arguably more approachable given that the name directly inherits the status.
There was a problem hiding this comment.
When I said it's confusing, I didn't realize I am the one who's confused :D I think you're right – hasResolved and isResolving can't both be true. I'm not sure how I concluded they can – thanks for staying alert!
Let's figure out what do to do next. We have three boolean flags: hasStarted, hasFinished, and isResolving. The following combinations are possible:
- false, false, false – the resolution hasn't started yet, the initial state
- true, false, true – the resolution is in progress
- true, true, false – the resolution has finished
so that's three values to represent. I think we need at least hasStarted and isResolving then, but hasFinished and isResolving works just as well.
what's the difference between hasResolved and !!data?
The resolved data can be any value at all, including null, undefined, false, and so on. We can have hasResolved === trueand!! data === false`.
|
This one is ready for another review 🎉 |
…ntity record is being re-requested.
5bbab93 to
7d259e2
Compare
There was a problem hiding this comment.
All the concerns here appear to have been addressed but we don't have a ✅
I'm going to approve on that basis along with the fact that this is an experimental hook which gives us some flexibility to merge and iterate.
Thanks for all the work you've put in here @adamziel.
|
Great work, @adamziel 🥇 Based on the latest commits, it looks like this PR also contains the newly proposed I don't think this is a blocker, but maybe something we can address as a follow-up. |
|
@Mamaduka good point! I think the discussion in #38134 needs some more time so I brought this in as a private API to unblock this PR. I considered using just |
|
Awesome work, @adamziel. Keep up PRs coming with the rest of functionality to fully cover CRUD operations for WordPress REST API endpoints 👏 I think that |
Description
This PR is a minimal subset of #38135 focused solely on the
useEntityRecordhook.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.
Out of scope for this PR
runIfoptionrunIfwas originally proposed in #38135 to address the following use case from the navigation block:Thinking more about that, maybe if would be better to have
getEntityRecord()in a separate, nested component. Either way, this can be a separate discussion in a follow-up PR.An optional id argument that can be provided by an EntityProvider
@talldan proposed to make
useEntityRecordmore consistent withuseEntityPropand have an optionalidthat would be backfilled byEntityProvider:gutenberg/packages/core-data/src/entity-provider.js
Lines 96 to 98 in 8fcd7f2
This looks great! My one concern is "What about passing an empty id to reason about a new entity record?" I propose to hold on for now and discuss what an empty
idargument should do later on once we're on the mutation hooks.Test plan
cc @kevin940726 @talldan @gziolo @draganescu @ellatrix @noisysocks @jsnajdr @getdave @Mamaduka @spencerfinnell