marmelab / react-admin

A frontend Framework for single-page applications on top of REST/GraphQL APIs, using TypeScript, React and Material Design
http://marmelab.com/react-admin
MIT License
24.65k stars 5.21k forks source link

Consider replacing "hotscript" types with "react-hook-form" types #9849

Open endoval opened 3 months ago

endoval commented 3 months ago

Hello there, we stumbled upon the fact that your typing of source property on *Field components uses hotscript types like so: Call<Objects.AllPaths, Resource> (added here: https://github.com/marmelab/react-admin/pull/8863)

We were wondering why that is the case because you are afaik using react-hook-form underneath, which provides a nice type to check paths as well with FieldPath<Resource>. Sadly, those two types require a different syntax for "array paths", which both functionally work. Consider the following example to illustrate this:

import { FC } from 'react';

import { Call, Objects } from 'hotscript';
import { TextField } from 'react-admin';
import { FieldPath } from 'react-hook-form';

interface MyRecord {
  tags: string[];
}

const MyComponent: FC = () => {
  const source1: FieldPath<MyRecord> = 'tags.0';
  const source2: Call<Objects.AllPaths, MyRecord> = 'tags[0]';

  return (<>
    <TextField label="Tag (source1)" source={source1} />
    <TextField label="Tag (source2)" source={source2} />
  </>);
}

Both versions work just fine (because the used lodash/get understands both), but of course the types are not matching. This might not seem like a problem right now, but once you start typing the *Input components all the way down to react-hook-form's useController() (within your useInput()) the types will clash. This already happens in our code base with some custom-written Inputs that use react-hook-form directly.

What we are wondering: Do you intend to stick to the hotscript types or consider switching to react-hook-form types?

Thank you! :)

slax57 commented 3 months ago

That's a very good question, thank you for raising this up! Indeed this seems to indicate we will run into issues when extending this feature to inputs. Your suggestion seems valid to me. @djhi @fzaninotto wdyt?

djhi commented 3 months ago

That would indeed be better and avoid an extra dependency! Thanks for the suggestion.

endoval commented 3 months ago

Great, thanks for the positive response. Looking forward to this change! 😌🙏

anmol-fzr commented 3 months ago

Hi 👋, I would like to work on this issue. Can you assign this issue to me.

slax57 commented 3 months ago

@anmol-fzr No need to assign the issue, you can open a PR mentioning this issue right away and we will review it carefully. Please remember to target the next branch. Thanks for offering to work on this :slightly_smiling_face:

fzaninotto commented 2 months ago

I'm -1 for adding a dependency to react-hook-form on non-form elements. Instead, we have two options:

  1. If hotscript allows tweaking the array syntax, force the one that is used by react-hook-form
  2. Copy the types from react-hook-form in react-admin and use these types in both fields and inputs.