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

Enforce stricter type for controller on change callback #10342

Merged

Conversation

bajormar
Copy link
Contributor

When using Controller field onChange fn, you were able to pass any value you want (because it was typed as any), even when it mismatches the type defined on the form.

So if form has defined type

{
  fieldName: number
}

You could pass

controllerProps.field.onChange('hello')

This is a huge typesafety hole, and I have patches my projects to have such behaviour. After this change a lot of error were exposed.

This implementation is still not 100% typesafe, because it still accepts event, for backwards compatibility reasons. But when event is passed, it suffer from safe issue - types can mismatch.

Ideally I would opt to either remove option to pass event at all (but this would hurt DX), or provide additional typesafe onChange which allows to pass just the exact type value

@codesandbox
Copy link

codesandbox bot commented Apr 26, 2023

CodeSandbox logoCodeSandbox logo  Open in CodeSandbox Web Editor | VS Code | VS Code Insiders

@bluebill1049
Copy link
Member

bluebill1049 commented Apr 27, 2023

thanks for the PR, hopefully, this would not break the user's app 🙏 I will add the update to the change log

@bluebill1049
Copy link
Member

close #10466

Copy link
Member

@kotarella1110 kotarella1110 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!Thanks.

@bluebill1049 bluebill1049 merged commit 747ddca into react-hook-form:master Jun 4, 2023
7 checks passed
@AurelienGasser
Copy link

AurelienGasser commented Jun 19, 2023

hopefully, this would not break the user's app 🙏

Narrator: it did break the user's app.

Unfortunately, I don't have time for a proper repro, but it did break our builds using @mui/x-date-pickers. We reverted to react-hook-form@v7.44.3.

Here's the error in case it can be useful.

Type error: Type '{ slotProps: { textField: { onBlur: (e: FocusEvent<HTMLInputElement | HTMLTextAreaElement, Element>) => void; name: TName; required: boolean; error: boolean; helperText: string | undefined; } | { ...; } | { ...; }; ... 21 more ...; mobileTransition?: Partial<...> | undefined; }; ... 52 more ...; disableOpenPicker?: ...' is not assignable to type 'DatePickerProps<Dayjs>'.
  Types of property 'onChange' are incompatible.
    Type '(event: ChangeEvent<Element> | PathValue<TFieldValues, TName>) => void' is not assignable to type '(value: Dayjs | null, context: PickerChangeHandlerContext<DateValidationError>) => void'.
      Types of parameters 'event' and 'value' are incompatible.
        Type 'Dayjs | null' is not assignable to type 'ChangeEvent<Element> | PathValue<TFieldValues, TName>'.
          Type 'null' is not assignable to type 'ChangeEvent<Element> | PathValue<TFieldValues, TName>'.

  34 |         const { name, onBlur, ...rhfFieldDatePickerProps } = field;
  35 |         return (
> 36 |           <DatePicker
     |            ^
  37 |             {...datePickerProps}

Thanks for all the good work

@bluebill1049
Copy link
Member

Perhaps we need to revert this as this probably brought breaking changes.

@pongells
Copy link

definitely a breaking change, a good one, but still unexpected to see my code break because of a minor change

@bajormar
Copy link
Contributor Author

at least from my understanding breaking change is when you change how it is working, or change how it needs to be used. In this place nothing is changed, it is more like it finally exposes issues with users code (mostly because onChange values mismatch FormValues), because previously it was any, and they could do whatever they want

@nesarares
Copy link

My case is similar to @AurelienGasser .

I have a custom component that has a "clear" button and it triggers the onChange callback with undefined . I can see how it's not "desired" as the value is required in the form's definition, but the validator will catch that upon form submission. I guess this can be solved by setting "strictNullChecks": false in the tsconfig.json but it's not a good solution. In my opinion the best approach would be to add | null | undefined in the definition but I'm not sure if it would break other stuff.

Reverted to react-hook-form@v7.44.3 as well for now.

@bajormar
Copy link
Contributor Author

submission

but what if you do form.watch? it will returns incorrect type that value is always defined and you get undefined errors. FormValues definition is not only used for submission. Typescript has no way of knowing that validator will catch undefined errors, so you either need to cast in your submit fn, or use something like Zod to parse values.

@asterikx
Copy link

asterikx commented Jun 20, 2023

For me, this change breaks all custom controlled components in my app where I restrict the field's value type using the FieldPathByValue type as constraint for TName:

type NumberInputFieldProps<
  TFieldValues extends FieldValues,
  TName extends FieldPathByValue<TFieldValues, number | undefined>,
> = NumberInputProps & UseControllerProps<TFieldValues, TName>;

function NumberInputField<
  TFieldValues extends FieldValues,
  TName extends FieldPathByValue<TFieldValues, number | undefined>,
>({ name, defaultValue, rules, control, ...inputProps }: NumberInputFieldProps<TFieldValues, TName>) {
  const {
    field: { value, onChange, onBlur: onFieldBlur, ref, ...fieldProps },
  } = useController({
    name,
    defaultValue,
    rules,
    control,
  });

  const onChangeValue: ArkNumberInputProps['onChange'] = ({ valueAsNumber }) => onChange(valueAsNumber);

  return (
    <NumberInput
      className={numberInput()}
      {...inputProps}
      {...fieldProps}
      value={value}
      onChange={onChangeValue}
    >
      <NumberInputScrubber />
      <ArkNumberInputField ref={ref} />
      <NumberInputControl>
        <NumberInputIncrementTrigger>
          <FiChevronUp />
        </NumberInputIncrementTrigger>
        <NumberInputDecrementTrigger>
          <FiChevronDown />
        </NumberInputDecrementTrigger>
      </NumberInputControl>
    </NumberInput>
  );
}

TS error:

Argument of type 'number' is not assignable to parameter of type 'PathValue<TFieldValues, TName> | ChangeEvent<Element>'. ts(2345)

@bajormar
Copy link
Contributor Author

this pattern seems to be broken by design. Yes, it enforces that from outside you can only pass correct name property, but inside both field.value and field.onChange argument is any, so you are free to do whatever you want

@asterikx
Copy link

@bajormar I don't think the pattern is broken. It is very useful to have a correctly typed API for the component. Yes, my component needs to handle any values accordingly, but users of my components can rely on correctly typed values.

@bajormar
Copy link
Contributor Author

@bajormar I don't think the pattern is broken. It is very useful to have a correctly typed API for the component. Yes, my component needs to handle any values accordingly, but users of my components can rely on correctly typed values.

What I mean by "broken" is that currently it is impossible from TypeScript perspective to gracefully handle inner Input controller typings.

To have full power of typesafety, the best approach is to use Controller with render props, because in that place you know exact form values structure and exact field name, so typescript is able to parse expected types.

<Controller
  control={form.control}
  name="reportingPeriod"
  render={(controllerProps) => (
    <FormSelectInput
      {...mapToInputProps(controllerProps)}
      variant="standard"
      wrapOptions={false}
      options={getPeriodOptions()}
    />
  )}
/>

This way controllerProps is narrowed correctly. For simpler mapping I also have mapToInputProps which basically flattens controllerProps. What is also nice about this approach, that now your inputs are decoupled from hook form - so you can easily have calculated readonly fields, easier to migrate to other form libraries if needed, etc..

@bjornpijnacker
Copy link

In our case this breaks compatibility with some MUI form components such as Select, for which we have our own wrappers.

The type that MUI uses for their onChange is now not compatible with the new onChange typing: Type '(event: ChangeEvent<Element> | PathValue<T, Path<T>>) => void' is not assignable to type '(event: SelectChangeEvent<unknown>, child: ReactNode) => void'. Types of parameters 'event' and 'event' are incompatible. Type 'SelectChangeEvent<unknown>' is not assignable to type 'ChangeEvent<Element> | PathValue<T, Path<T>>'.

Full example:

interface SelectProps<T extends FieldValues> extends UseControllerProps<T> {
  MUISelectProps?: MUISelectProps;
  options: SelectOption[];
}

interface SelectOption {
  id: string | number | null;
  name: string;
  description?: string;
}

export const Select = <T extends FieldValues>({
  MUISelectProps,
  name,
  control,
  options
}: SelectProps<T>) => (
  <Controller
    name={name}
    control={control}
    render={({ field }) => (
      <MUISelect
        name={field.name}
        value={field.value}
        onChange={field.onChange}
        {...MUISelectProps}
      >
        {/* map options in here, not important for demo */}
      </MUISelect>
    )}
  />
);

https://codesandbox.io/s/dreamy-villani-wftdyf?file=/src/Select.tsx

@iffa
Copy link

iffa commented Jun 21, 2023

Yes this is definitely a breaking change, and should be reverted and reintroduced in a major version. I much prefer the previous behavior, as it allowed setting undefined/null/whatever and the form validation handles these as it should.

@bajormar
Copy link
Contributor Author

Yes this is definitely a breaking change, and should be reverted and reintroduced in a major version. I much prefer the previous behavior, as it allowed setting undefined/null/whatever and the form validation handles these as it should.

So that is basically the same as to say "I prefer to have any in my code, as it allows to do anything I want". It was a types "bug" from library side, which is fixed by this PR. Yes, you need to make adjustment to your code (because TypeScript finally unveiled that it is unsafe), but that is for the better, as it allows to avoid nasty mistakes

@bjornpijnacker
Copy link

bjornpijnacker commented Jun 21, 2023

So that is basically the same as to say "I prefer to have any in my code, as it allows to do anything I want". It was a types "bug" from library side, which is fixed by this PR. Yes, you need to make adjustment to your code (because TypeScript finally unveiled that it is unsafe), but that is for the better, as it allows to avoid nasty mistakes

Having types is not bad. However, more care should be taken in how this may influence existing use cases and clearly this change must be a major version, as it is breaking to current workflows. Plus, the current type clearly doesn't work everywhere it should.

You mentioned earlier that it is simply a case of bad code, however this is not true, since many actual features break due to this, including but not limited to: MUI compatibility and setting undefined

@bajormar
Copy link
Contributor Author

@bajormar still not listening huh ?!

it is not for me to decide what is included and what is not :D I am just giving reasons why this change improves existing library and also providing a solution how with 5 symbols you can get identical behaviour as before, but you are the one that do not want to listen

@reediculous456
Copy link

you can simply add as any to your onChange handler (it was any before my PR), and you will have identical behaviour as before...

My issue with this proposed solution is it would require me to write an onChange for my component. Currently the onChange is added by using {…fields}

#10342 (comment)

@danieljmann96
Copy link

you can simply add as any to your onChange handler (it was any before my PR), and you will have identical behaviour as before...

My issue with this proposed solution is it would require me to write an onChange for my component. Currently the onChange is added by using {…fields}

#10342 (comment)

Same here. I just use
{...field}

in a form that has 20 or so inputs, with different types etc, don't see myself upgrading to this new version any time soon.

@eagle-head
Copy link

When using Controller field onChange fn, you were able to pass any value you want (because it was typed as any), even when it mismatches the type defined on the form.

So if form has defined type

{
  fieldName: number
}

You could pass

controllerProps.field.onChange('hello')

This is a huge typesafety hole, and I have patches my projects to have such behaviour. After this change a lot of error were exposed.

This implementation is still not 100% typesafe, because it still accepts event, for backwards compatibility reasons. But when event is passed, it suffer from safe issue - types can mismatch.

Ideally I would opt to either remove option to pass event at all (but this would hurt DX), or provide additional typesafe onChange which allows to pass just the exact type value

Hello @bajormar ,

I'm having an issue with TypeScript types when using react-hook-form with a custom TextInput component in a React Native project.

Here's the FormTextInput component:

import React from 'react';
import {Controller, UseControllerProps, FieldValues} from 'react-hook-form';
import {TextInput, TextInputProps} from '../TextInput/TextInput';

export function FormTextInput<T extends FieldValues>({
  control,
  name,
  rules,
  ...textInputProps
}: TextInputProps & UseControllerProps<T>) {
  return (
    <Controller
      control={control}
      name={name}
      rules={rules}
      render={({field, fieldState}) => (
        <TextInput
          value={field.value}
          onChangeText={field.onChange}
          errorMessage={fieldState.error?.message}
          {...textInputProps}
        />
      )}
    />
  );
}

And the onChangeText type from TextInput:

/**
 * Callback that is called when the text input's text changes.
 * Changed text is passed as an argument to the callback handler.
 */
onChangeText?: ((text: string) => void) | undefined;

The onChange type from ControllerRenderProps is causing the issue:

export type ControllerRenderProps<TFieldValues extends FieldValues = FieldValues, TName extends FieldPath<TFieldValues> = FieldPath<TFieldValues>> = {
    onChange: (event: ChangeEvent | FieldPathValue<TFieldValues, TName>) => void;
    onBlur: Noop;
    value: FieldPathValue<TFieldValues, TName>;
    name: TName;
    ref: RefCallBack;
};

Here's the TypeScript error I'm getting:

Type '(event: PathValue<T, Path<T>> | ChangeEvent<Element>) => void' is not assignable to type '(text: string) => void'. Types of parameters 'event' and 'text' are incompatible. Type 'string' is not assignable to type 'PathValue<T, Path<T>> | ChangeEvent<Element>'.ts(2322) TextInput.d.ts(670, 3): The expected type comes from property 'onChangeText' which is declared here on type 'IntrinsicAttributes & TextInputProps'

My package.json file:

{
"dependencies": {
    "@react-navigation/native": "^6.1.7",
    "@react-navigation/native-stack": "^6.9.13",
    "@shopify/restyle": "^2.4.2",
    "react": "18.2.0",
    "react-hook-form": "^7.45.0",
    "react-native": "0.71.10",
    "react-native-safe-area-context": "^4.6.2",
    "react-native-screens": "^3.22.0",
    "react-native-svg": "^13.9.0"
  },
  "devDependencies": {
    "@babel/core": "^7.20.0",
    "@babel/preset-env": "^7.20.0",
    "@babel/runtime": "^7.20.0",
    "@react-native-community/eslint-config": "^3.2.0",
    "@tsconfig/react-native": "^2.0.2",
    "@types/jest": "^29.2.1",
    "@types/react": "^18.0.24",
    "@types/react-test-renderer": "^18.0.0",
    "babel-jest": "^29.2.1",
    "eslint": "^8.19.0",
    "jest": "^29.2.1",
    "metro-react-native-babel-preset": "0.73.9",
    "prettier": "^2.4.1",
    "react-test-renderer": "18.2.0",
    "typescript": "4.8.4"
  },
}

Can you help me an idea how to solve this type problem?

@cleisonmp
Copy link

This breaks most components libraries out there due to requiring a custom onChange, so does not really matter whether it's good or not it should be on a major release.

@yeliex
Copy link

yeliex commented Jun 26, 2023

the event argument should be optional

@nhatncb
Copy link

nhatncb commented Jun 27, 2023

This PR is so simple, It doesn't cover all cases and all kinds of possible fields for form. You enforce stricter type for controller on change callback and force dev who uses generic types using as as a solution?

@bajormar
Copy link
Contributor Author

This PR is so simple, It doesn't cover all cases and all kinds of possible fields for form. You enforce stricter type for controller on change callback and force dev who uses generic types using as as a solution?

It is open source project, you can create a PR which fixes your case. I tried to find a way how to make it type safe in other cases, but could not do this. I am not even sure if that is possible in TypeScript

@jvandenaardweg
Copy link

jvandenaardweg commented Jun 27, 2023

Can we just revert this asap? If not, atleast propose some good official website docs on how we should do things now. Clearly this breaks alot of situations where people rely on.

@anton-g
Copy link
Contributor

anton-g commented Jun 27, 2023

I like strict types, but the change should be made in a major release since it is a breaking change. I encourage reverting and postponing this until the next major.

@JakeSaterlay
Copy link

JakeSaterlay commented Jun 27, 2023

Is it possible this change has also impacted components that utilise watch?

I have an abstraction of a MUI textfield component like such

 <TextField.Basic
       label="Contacts"
       sx={{ width: '100%' }}
       {...register('contacts')}
  />

I had a watch in a component which uses this textfield to get the character count, however as of last week it has suddenly stopped working and anything typed in to the text field is immediately removed.

Reverting back to "react-hook-form": "7.43.9", fixes the issue and works as intended

@jvandenaardweg
Copy link

@JakeSaterlay I see the changes from this PR are only related to Typescript types (+ some tests), so that does not affect runtime code which could lead to things not working like you experience. So it's probably something different

@kvasnicaj
Copy link

@JakeSaterlay I see the changes from this PR are only related to Typescript types (+ some tests), so that does not affect runtime code which could lead to things not working like you experience. So it's probably something different

I had same issues with MUI components as @JakeSaterlay Maybe it's not related to the PR, but it's definitely related to the release. Reverting to the previous version helped.

@JakeSaterlay
Copy link

I had same issues with MUI components as @JakeSaterlay Maybe it's not related to the PR, but it's definitely related to the release. Reverting to the previous version helped.

Would be great to re-create the problem in isolation and raise an issue @kvasnicaj I don't think I will get a chance to do this until the weekend though

@danieljmann96
Copy link

@JakeSaterlay I see the changes from this PR are only related to Typescript types (+ some tests), so that does not affect runtime code which could lead to things not working like you experience. So it's probably something different

Whilst this is true, aside from putting an "as any" in every form component, most devs using MUI or other libraries are going to have to change their runtime code to fit around these changes.
Which isn't necessarily a "bad" thing, but this just feels like the release was brought out under the radar without the warning that so many builds will start to fail.
The example in the official documentation here
https://www.react-hook-form.com/get-started/ recommends using {...field} to integrate with a MUI input and with react-select. This is incorrect typing on React-select now due to this change. The example should've therefore also been updated.

Are these examples tested with each "minor" release?

@Jarzka
Copy link

Jarzka commented Jun 28, 2023

We also have a problem with using onChange in Controller components after these changes.

We have built a generic form generator, which accepts the form data type as a generic:

function Form<FormData extends FieldValues>({
  ...
}

Inside Form, we generate fields like this:

case "checkbox":
  return (
    <Controller
      {...controllerProps}
      render={({ field: { value, onChange, onBlur } }) => {
        return (
          <Checkbox
            {...commonProperties}
            onChange={(checked) => handleOnChange(() => onChange(checked))}
            onBlur={onBlur}
            checked={value}
          />
        );
      }}
    />
  );

This has worked perfectly fine, but now I'm getting an error while calling onChange(checked):

TS2345: Argument of type 'boolean' is not assignable to parameter of type 'PathValue  > | ChangeEvent '.

I don't understand what's wrong. The type should be PathValue<FormData, Path<FormData>>, but It seems the onChange does not accept any value anymore.

@doberkofler
Copy link

@bajormar Thank you for improving this library!

I'm all in on type safety, but would really appreciate a little more information on how to deal with the new stricter type checks introduced with this PR. A few examples for typical use cases in the change log might go a long way.

I personally have a lot of type errors after upgrading to this revision and most of my use cases look somehow like this, with TypeScript complaining Argument of type 'string' is not assignable to parameter of type 'PathValue<TFieldValues, Path<TFieldValues>> | ChangeEvent<Element>' in the line field.onChange(e.target.value);:

type PropsBaseType<TFieldValues extends FieldValues> = {
	control: Control<TFieldValues>,
	name: FieldPath<TFieldValues>,
	disabled?: boolean,
	placeholder?: string,
	label: string,
	helperText?: string,
	errorText?: string,
	autoFocus?: boolean,
	required?: boolean,
};

type PropsTextType<TFieldValues extends FieldValues> = PropsBaseType<TFieldValues> & {
	minLength?: ValidationRule<number>,
	maxLength?: ValidationRule<number>,
	pattern?: ValidationRule<RegExp>,
	validate?: Validate<string, TFieldValues>,
};

export const InputText = <TFieldValues extends FieldValues>({
	control,
	name,
	disabled = false,
	label = '',
	placeholder = '',
	helperText = '',
	autoFocus = false,
	required = false,
	minLength,
	maxLength,
	pattern,
	validate,
}: PropsTextType<TFieldValues>): JSX.Element => {
	const {field, fieldState} = useController({
		control,
		name,
		rules: {
			required,
			minLength,
			maxLength,
			pattern,
			validate,
		},
	});
	const [value, setValue] = React.useState<string>(field.value);
	const handleChange = (e: React.ChangeEvent<HTMLInputElement>) => {
		field.onChange(e.target.value);
		setValue(e.target.value);
	};
	const fieldProps: TextFieldProps = {
		disabled,
		label,
		placeholder,
		error: Boolean(fieldState.error),
		helperText: fieldState.error ? 'Error' : helperText,
		autoFocus,
		value,
		name: field.name,
		onChange: handleChange,
		onBlur: field.onBlur,
		inputRef: field.ref,
		...elementDefaults,
	};

	return <TextField {...fieldProps}/>;
};

What would be type proper and type safe way to resolve this kind of type error?

@JJosephttg
Copy link

I wouldn't mind this fix, but the problem is that the type does not account for types that transform from one value to another. For example, your onChange might be a string that gets transformed into a number. With this change, I am unable to use transform and not have type issues. Keep that in mind if you release at a later version

@eg-bernardo
Copy link
Contributor

It's also an issue for cases in which there's validation, where the field cannot be null or undefined but the onChange can.

@danieljmann96
Copy link

I actually don't think keeping the current "any" behaviour is necesarrily a bad thing here. Just let each dev do whatever typing/parsing they want for this use case.

@vchirikov
Copy link

vchirikov commented Jul 30, 2023

I have a similar issue like @Jarzka with checkboxes + shadcn-ui (zod + react-hook-form) :
image

@Verhaeg
Copy link

Verhaeg commented Aug 14, 2023

This kind of change should have been a breaking change.. not Minor..
it really breaks everything and is not easily found as issue..

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

Successfully merging this pull request may close these issues.

None yet