dohomi / react-hook-form-mui

Material-UI form components ready to use with react-hook-form
https://react-hook-form-material-ui.vercel.app
MIT License
564 stars 111 forks source link

Breaking change to `Validate` in 6.5.1 #265

Closed rynoV closed 6 months ago

rynoV commented 6 months ago

Duplicates

Latest version

Current behavior 😯

Thanks for making this!

I'm mostly just filing this for documentation: there was a breaking change to the types in version 6.5.1, in this commit https://github.com/dohomi/react-hook-form-mui/commit/75d70f473029d0f93fb05466c2ce306f5944c895. When using the rules or validation props on some elements there might be a new type error like:

error TS2322: Type '(value: string) => Promise<ValidateResult>' is not assignable to type 'Validate<string | number | boolean | string[]>'.
  Types of parameters 'value' and 'value' are incompatible.
    Type 'string | number | boolean | string[]' is not assignable to type 'string'.
      Type 'number' is not assignable to type 'string'.

289           async parse(value: string): Promise<ValidateResult> {

Expected behavior 🤔

No response

Steps to reproduce 🕹

Steps:

1. 2. 3. 4.

rynoV commented 6 months ago

It seems like the issue is specific to the validate key within ControllerProps.rules. The referenced commit added the TFieldValues generic to ControllerProps['rules'], but the TName isn't present, so the Validate type isn't being narrowed to the type of the field specified by TName, and instead is defaulting to a union of all types of fields within the form.

I don't have any pressing need to upgrade the version so my solution for now is just to lock the version to 6.5.0

dohomi commented 6 months ago

could you provide a repo / codesandbox which higlights the issue that would be helpful

sadik-malik commented 6 months ago

@rynoV have you tested it with v6.8.0 or the latest version v7.0.0?

rynoV commented 6 months ago

@rynoV have you tested it with v6.8.0 or the latest version v7.0.0?

I see the issue on both of these versions

Here's a sandbox: https://codesandbox.io/p/sandbox/react-hook-form-mui-zh4mxg

It looks like somewhere past v6.5.1 the TName generic was added (at least for the Autocomplete), so I was able to fix it in 6.8.0 by specifying the TName generic with Extract<FieldPath<FormValues>, 'field'> (which works for nested fields as well, unlike my original attempt FieldPath<Pick<FormValues, 'field'>>)

I couldn't find a way to make type inference work to find the most specific TName from the name property, so I had to add the generic manually

dohomi commented 6 months ago

@rynoV you would never need to provide it in the way you did in, here is an updated version: https://codesandbox.io/p/sandbox/react-hook-form-mui-forked-8nhq9h?file=%2Fsrc%2FApp.tsx%3A39%2C45

you can also have a look at https://github.com/dohomi/react-hook-form-mui/blob/master/apps/storybook/stories/FormContainer.stories.tsx#L145 which is meant to have strict typings example.

Basically if you provide control of useForm and you set your default defaultValues the control prop knows which names are allowed within the form context.

dohomi commented 6 months ago

I add a typesafe example to the landing page of Github https://github.com/dohomi/react-hook-form-mui?tab=readme-ov-file#typesafe-form-setup

rynoV commented 6 months ago

@dohomi Thanks for the example in the docs. I couldn't access the sandbox you sent, but I tried to update my sandbox based on the example in the docs and the type error is still present until I specify the second generic: https://codesandbox.io/p/sandbox/react-hook-form-mui-zh4mxg?file=%2Fsrc%2FApp.tsx%3A10%2C32

Re the other stories link, I didn't see any examples there with the rules.validate prop, which seems to be the main affected area of the breaking change (or at least the only area I've run into issues with)

Please let me know if I'm missing anything

dohomi commented 6 months ago

you should not pass any types because control is taking care of it, do you see this example:

https://codesandbox.io/p/devbox/h3spsq?file=%2Fsrc%2FApp.tsx

import { FieldPath, useForm } from "react-hook-form";
import { AutocompleteElement } from "react-hook-form-mui";
import "./styles.css";

interface FormValues {
  field: string;
  otherField: number;
}

export default function App() {
  const { control, handleSubmit } = useForm<FormValues>({
    defaultValues: {
      field: "",
      otherField: 0,
    },
  });
  return (
    <div className="App">
      <h1>Hello CodeSandbox</h1>
      <h2>Start editing to see some magic happen!</h2>
      <AutocompleteElement
        control={control}
        name={"field"}
        options={[]}
        rules={{
          validate: {
            parse(value) {
              console.log(value);
              return true;
            },
          },
        }}
      />
    </div>
  );
}
rynoV commented 6 months ago

Ah okay I see now, don't specify any generics and pass a control. I wasn't passing control before because I was relying on FormContainer, but I suppose using useFormContext and passing the returned control is what the component is doing anyways so it should be equivalent.

It would be nice if I didn't need to change the code purely to affect things at the type level, but it seems like this would require partial type inference to stop typescript from using the default value for the TName generic. The only other solution I could see is a dummy/phantom parameter like fieldValuesType={undefined as MyFieldValues}, which I might prefer but I'm not that experienced with typescript API design

Appreciate the help