Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
endoval opened this issue May 15, 2024 · 6 comments
Open

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

endoval opened this issue May 15, 2024 · 6 comments
Labels

Comments

@endoval
Copy link

endoval commented May 15, 2024

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: #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
Copy link
Contributor

slax57 commented May 16, 2024

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
Copy link
Collaborator

djhi commented May 16, 2024

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

@endoval
Copy link
Author

endoval commented May 16, 2024

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

@anmol-fzr
Copy link

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

@slax57
Copy link
Contributor

slax57 commented May 21, 2024

@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 🙂

@fzaninotto
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants