React-query admin wrapper and possibily undefined value with useGetOne

[soullivaneuh]

According to the React Query documentation, we have the possibility to block the running query thanks to the enabled function if one of our parameter is not defined. On the documentation sample, we can see userId can be set on the query definition without any issue.

We also have the possibility to use this option from the useGetOne hooks as you specified on the code doc block

https://github.com/marmelab/react-admin/blob/69d1cd1af77a432e2d1a3c2215ee93a2dedcd4be/packages/ra-core/src/dataProvider/useGetOne.ts#L26

However, the typing of the function requires the id to be a valid identifier:

https://github.com/marmelab/react-admin/blob/1ddb1672fe323baffb80567c890c759b3cea379c/packages/ra-core/src/types.ts#L139

Because of that, we have to cheat by casting the typing of the undefined value, for example:

const { data: userData } = useGetOne<UserRecord>('users', { id: userId || '' }, { enabled: !!userId });

According to me, we should make that param optional to match the react-query implementation.

What do you think?

[fzaninotto]

Thanks for your proposal.

To me, what you suggest isn't an improvement in the general case. If a user doesn't use the enabled prop, they will see the id parameter as optional, which is misleading.

To make it work with TypeScript, the function should have two signatures: one with enabled enabled, where the params are optional, and one without. But I'm not even sure this is correct or feasible.

In other terms, I think the react-admin codebase shouldn't handle this case. We should at most document the gotcha in the documentation of the enabled option.

That's my opinion, I'm open to other points of view.

[soullivaneuh]

To me, what you suggest isn't an improvement in the general case. If a user doesn't use the enabled prop, they will see the id parameter as optional, which is misleading.

I understand fully that point.

To make it work with TypeScript, the function should have two signatures: one with enabled enabled, where the params are optional, and one without. But I'm not even sure this is correct or feasible.

That can be a solution. We may use Discriminating Unions for that, but I think we will have to merge the options third argument to the second one in order to achieve that. However I'm not a big expert of Typescript, we may have another solution! :wink:

In other terms, I think the react-admin codebase shouldn't handle this case. We should at most document the gotcha in the documentation of the enabled option.

This should be considered as the last solution according to me because it may misleading to on the other hands.

Thanks for the feedback!

[fzaninotto]

I'll close this one as it seems a duplicate of #7986 (for which a PR is ongoing)

[fzaninotto]

No, my bad, 7986 is for the useGetList hook, while this issue concerns useGetOne.

[ogustavo-pereira]

I don't know if I understand the case but I don't see this point as a problem in my opinion.

From what I understand this point is something that must be dealt with in the component and not something that the react-admin hook is responsible for.

I think the useGetOne hook must have its operating requirements and even though it uses react-query it shouldn't be exactly the same as react-query, otherwise just use react-query.

However, if it is a desirable change for the tool, I can try to help with pr, what do you think @fzaninotto?

[fzaninotto]

Indeed, this issue remains open, so any PR addressing it is welcome!

[djhi]

I tried to type the useGetOne function so that it accepts undefined as the id when enabled is false, however the DX is not good. The solution I found was to declare function overloads:

export function useGetOne<RecordType extends RaRecord = any>(
    resource: string,
    params: Partial<GetOneParams<RecordType>>,
    options?: UseQueryOptions<RecordType> & { enabled: false }
): UseGetOneHookValue<RecordType>;

export function useGetOne<RecordType extends RaRecord = any>(
    resource: string,
    params: GetOneParams<RecordType>,
    options?: UseQueryOptions<RecordType> & { enabled: true }
): UseGetOneHookValue<RecordType>;

export function useGetOne<RecordType extends RaRecord = any>(
    resource: string,
    { id, meta }: GetOneParams<RecordType>,
    { enabled, ...options }: UseQueryOptions<RecordType> = { enabled: true }
): UseGetOneHookValue<RecordType> {
    // implementation as before
}

It kinda works as long as you provide the enabled option either to true or false. When ommited, TS doesn't know which one to pick and allow id to be undefined so declaring id as Identifier | undefined would mostly be the same.

Besides, the error when passing an undefined id and enabled is true is cryptic:

No overload matches this call.
  Overload 1 of 2, '(resource: string, params: Partial<GetOneParams<UserRecord>>, options?: (UseQueryOptions<UserRecord, unknown, UserRecord, QueryKey> & { ...; }) | undefined): UseGetOneHookValue<...>', gave the following error.
    Type 'true' is not assignable to type 'false'.
  Overload 2 of 2, '(resource: string, params: GetOneParams<UserRecord>, options?: (UseQueryOptions<UserRecord, unknown, UserRecord, QueryKey> & { ...; }) | undefined): UseGetOneHookValue<...>', gave the following error.
    Type 'undefined' is not assignable to type 'string'.ts(2769)
types.ts(159, 5): The expected type comes from property 'id' which is declared here on type 'GetOneParams<UserRecord>'