Specify data provider methods as generic (not just their arguments) for type safety

[bayareacoder]

Is your feature request related to a problem? Please describe.
When manually calling data provider methods, or when they are called by react-admin behind the scenes, there is no type safety for input data, or returned data.

Describe the solution you'd like
1/ Provide the option to make all data provider methods type-safe by making them generic, so that at least when they are called manually it can be type-safe.
2/ Provide type-safety also when react-admin internally calls these methods. This would lead to a great DX where you could create a form for object create and it would err when a required property is forgotten, or a form for object update that would err if a property that cannot be updated is included.

Describe alternatives you've considered
We have coded 1/ ourselves in our data providers and how we call them manually, but it would probably be useful to enhance the docs with this (now that you have #5934)

eg. for create with ResourceName as a union of literal types which also ensures no non-existing resource names can be passed:

async create<T extends any = any, R extends RaRecord = RaRecord>(
    resource: ResourceName,
    params: CreateParams<T>,
  ): Promise<CreateResult<R>> {
// ...
}

and then call, for resource Shop as:

create<CreateShopData, Shop>('Shop', { data });

Then if you add overloaded declarations in your data provider for each of the narrowed resource types:

create(resource: 'Shop', params: CreateParams<CreateShopData>): Promise<CreateResult<Shop>>;
create(resource: 'Account', params: CreateParams<CreateAccountData>): Promise<CreateResult<Account>>;
// ...
async create<T extends any = any, R extends RaRecord = RaRecord>(
    resource: ResourceName,
    params: CreateParams<T>,
  ): Promise<CreateResult<R>> {
// ...
}

you can call without generics on the call site and get full type safety:

create('Shop', { data });

To avoid all the overloaded definitions, I tried first to do this with some kind of mapped/conditional type but that that wasn't successful so far.

Additional context
This builds further upon #5934 which introduced generics for the arguments to the data provider methods but not the methods themselves.

[fzaninotto]

I'm not sure to understand where you want to add these generics.

We already have them in useDataProvider:

interface Product extends Record {
    name: string;
    sku: string;
}

const dataProvider = useDataProvider();
dataProvider.getOne<Product>('resources', 123)
   .then(({ data }) => {
        // TS sees data as Product
    })

We can't add these generics in useQuery, useQueryWithStore, or useMutation because they are dynamic in nature (the dataProvider method called is determined at runtime).

useGetList and useGetOne are already generic:

    const { ids, data, total, loaded } = useGetList<Product>(
        'posts',
        { page: 1, perPage: 10 },
        currentSort
    );
    // data is of type RecordMap<Product>

So the only places left to add them are the remaining the specialized hooks like useUpdate, useCreate, etc. Is that where you'd expect to specify the record type? Or somewhere else?

[bayareacoder]

Yes also on the static create & update methods and allow to specify both the returned recordType and the type of the data input separately, since not always do you specify all properties of the returned record in a create (server may add some that are returned in the response, eg a timestamp, or a GQL server may resolve a lot more and add it to the record) so you'd most likely have a CreateParams<T> where T would be a subset of the RecordType, and then also allow to use a different type on UpdateParams<U> as U may be a subset of T as you may not be able to update all properties after creation.

Currently eg create is typed as:

create: <RecordType extends Record = Record>(
        resource: string,
        params: CreateParams
) => Promise<CreateResult<RecordType>>;

So suggestions are:

1. make CreateParams generic
2. make resource:string a union string literal type so you can add overloads ( as in my earlier post) to avoid the need for generic arguments each time you call the method. You can tie this type to the user specifying just the resource names in an array so it's transparent to the user:

const resources = ['Account','Shop'] as const;
type ResourceNames = typeof resources;
type ResourceName = ResourceNames[number]

create<T extends any = any, R extends Record = Record>(
    resource: ResourceName,
    params: CreateParams<T>,
  ): Promise<CreateResult<R>> {
// ...
}

And similar for update, updateMany etc. and make sure it works when the methods are retrieved from the use... hooks (understood it cannot be done for the dynamic ones)

So with the overloads in the data provider, since you don't need to specify generics on the call site, a react-admin form constructed for a given resource maybe could also show an error if a required property was forgotten or a non-existing one included as you'd be able to detect that the data provider doesn't receive data of the correct type from the Create and Update views? That would be fantastic. Makes things correct by construction.

[fzaninotto]

I've updated the specialized dataProvider hooks in #6168.

As for moving the types from components to the dataProvider and through the form, I don't think it's possible due to the dynamic programming techniques we're using.

Feel free to open a PR if you find a way to do so.

[bayareacoder]

Thanks but the typing is not as strict as what I requested. I see in the code it is now:

create = useCallback(
         (
             resource?: string | Partial<Mutation> | Event,
             data?: Partial<RecordType>,
             options?: MutationOptions
         )

So:

1. since the typing for resource allows non-literal types, it probably won't work with the overloaded function syntax in the data provider as there is no mutual exclusivity per resource.
2. typing data as Partial<RecordType> does not give any type safety to ensure you specify all required properties for a create, or protect against specifying non-updateable properties in update. We have separate types for eg UserRecord, CreateUserRecord, UpdateUserRecord for this. This is also typical when you use a GraphQL backend where you will get separate types for inputs & payloads of all operations.

[fzaninotto]

I didn't understand that sentence:

since the typing for resource allows non-literal types, it probably won't work with the overloaded function syntax in the data provider as there is no mutual exclusivity per resource.

Can you give examples of what you mean?

As for the second point, please open a PR to implement what you want, we'll see if it makes sense to merge it in the core.

[bayareacoder]

Issue 1. I thought it was explained in my original post. We declare overloaded data provider methods based on the resource. This works if the resource arg is defined as a literal union type:

type Resource = `posts` | `users`

The TS compiler will now select the correct overloaded function based on the resource and since the declarations in the data provider specify the generic params for each resource, see my original post, you don't need to specify these when you call the methods. So easier and no risk to specify wrong type for given resource. I don't think you still need to have string since the union literal type is a narrowed version of a string, so it is a string as well. And you can derive the literal union type inside react-admin from an array or resources specified by the user const resources = ['posts','users'] , see my earlier post, so your users don't need to be aware if they don't care about this.

Issue 2. I think the benefit of what I propose is clear, right and there is no downside since the 2nd generic argument (ie the CreateParams<T> generic type can be by default your current Partial<RecordType> so it is backward compatible. The change probably affects several locations so would probably be better done by you if it is deemed useful.

Example:
See attached image with example of our code where we manually call create but commented out a required property name. No generic arguments needed on the call site and TS compiler detects that we forgot a required property in the data argument on the create for the Shop resource so it shows there is no match on any of the overloaded functions.

[fzaninotto]

I understand that you want to be much more type safe than what we currently target in react-admin. So please submit a PR if you feel you can contribute what you did to the core, but we won't work further on this subject for now.

[bayareacoder]

OK understood.
FYI we actually take it 1 step further in our code by using an npm package called runtypes: https://github.com/pelotom/runtypes. In the data provider we type the expected runtypes and the package derives the static TS types automatically from that. During development at run-time the package validates that the data from the API backend is consistent with the runtype/TS type. And with the above changes we link that type safety with the manual data provider calls.

[WinstonN]

I landed here today, because I wanted to run the demo application as described here https://github.com/marmelab/react-admin/tree/master/examples/demo

I followed all the steps, and on the last step make run-demo the site loads but with an error

Compiled with problems:X

ERROR

Plugin "react" was conflicted between "../../.eslintrc" and "BaseConfig » /home/win/Projects/private/smashem/react-admin/node_modules/react-scripts/node_modules/eslint-config-react-app/base.js".


ERROR in src/reviews/ReviewEditToolbar.tsx:19:35

TS2315: Type 'ToolbarProps' is not generic.
    17 | }));
    18 |
  > 19 | const ReviewEditToolbar = (props: ToolbarProps<Review>) => {
       |                                   ^^^^^^^^^^^^^^^^^^^^
    20 |     const {
    21 |         basePath,
    22 |         handleSubmitWithRedirect,

I can click away the error, but have not found a resolution yet

[WiXSL]

@WinstonN,
Thanks for pointing this out.
This error was caused by a very recent change.
We'll try to fix it as soon as possible

[WiXSL]

Fix incoming #7045