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.

Do you mean isError or error details? I don't think we can access the latter, any ideas?

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.

Agreed! It is a quite old problem, too – it would be great to get it fixed:

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:

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

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.

I started some explorations in #38669 – let's move the discussion there

Why do we have to disable this rule, a bug in eslint?

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.

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 👍🏻

Here's a PR to adjust the eslint rules: #38665

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

It's a bit more complicated to get right though, I'm willing to help if you encounter any problems!

Also, if the data is still resolving, I think the type of this value will be null?

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 👍

Hey @kevin940726, you will like this PR: TypeScript definitions for core-data entity records

I guess we don't need this either, as this is essentially the same as !isResolving.

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.

isResolving and hasResolved may both be true when the record is being re-requested.

Huh, interesting! Then what's the difference between hasResolved and !!data?

Just checked the implementation in:

isResolving and hasResolved may 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.

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