Add correct types for `Field`

Zikoat commented 6 months ago

Feature request

Current / Desired Behavior

Type the Field props properly. Currently the props areany, which means we dont get intellisense when we want to add props to the Field, and we also dont get any type errors when for example the name is missing.

Currently, these examples typecheck just fine, but are all slightly wrong. They should throw the expected errors as in the // @ts-expect-error directive

Name should be required:

// @ts-expect-error Property 'name' is missing in type '{}' but required in type 'FieldConfig<any>'.
<Field></Field>

Type of field in children props should be FieldInputProps:

<Field name="name">
  {({ field }) => {
    // @ts-expect-error Type 'FieldInputProps<any>' is not assignable to type 'string'.
    const expectTypeError: string = field;
    return expectTypeError
  }
  }
</Field>

Custom props should be typed:

const Component = (props: FieldProps & { myCustomProp: string }) => (
  <div>
    <p>{props.myCustomProp}</p>
    <input {...props.field} />
  </div>
);
<Field
  name="name"
  component={Component}
  // should not throw a type error
  myCustomProp={'Name'}
></Field>;

When a custom component is passed in, the props to Field should not include props from input element. This is to avoid type mismatches in case the custom component has a prop with the same name as an input prop.

<Field
  name="name"
  component={() => null}
  // @ts-expect-error
  type="email"
></Field>;

Suggested Solution

Remove & T from FieldAttributes. Add a new generic to FieldAttributes and FieldConfig to allow for custom props to be passed in. Remove & GenericFieldHTMLAttributes from FieldAttributes conditionally based on the presence of component, children or as prop.

The guiding principles for this implementation are the expected behavior/tests above. This is not quite finished yet, but I think the new types will look something like this:

export interface FieldConfig<V = any, P = {}>{
  /**
   * Field component to render. Can either be a string like 'select' or a component.
   */
  component?:
    | string
    | React.ComponentType<Omit<FieldProps<V>,"meta"> & P &  { className?: string }>
    | React.ComponentType<any>
    | React.ForwardRefExoticComponent<any>;
  // ...
}

export type FieldAttributes<T, P, IsCustom extends boolean> = 
  & (IsCustom extends true ? {} : GenericFieldHTMLAttributes) 
  & FieldConfig<T, P> 
  & Omit<P, keyof FieldProps<any>> 
  & { name: string; };

Here

T is the type of the value of the field, like before
P is the type of the props passed in to the custom component
IsCustom is a generic type which is true when the component prop is passed in, and false otherwise (the input element is used by default).

Who does this impact? Who is this for?

This will impact all users that use TypeScript. I have added these types to our local repository, and among 100 Field usages, there were about 10 different type errors that needed to be fixed. For me, they were pretty easy to fix, as i knew what changes had been done, but they might come as unwanted surprises for others if we change the types in minor release. I think we might have to create a migration guide for this, and maybe bump the major version so people are prepared to do some type-work for this change.

Describe alternatives you've considered

Migration to React Hook Form, but we have a lot of forms, and we are happy with Formik.
Creating our own fork of formik and use the fixed types in our own repo
Working around the type limitations by creating field wrappers using useField instead, which avoids usage of the Field component
Let it be like it is

Additional context

Will solve these issues:

https://github.com/jaredpalmer/formik/issues/2086 (closed, but has gotten a lot of comments after it was closed) https://github.com/jaredpalmer/formik/issues/3885 https://github.com/jaredpalmer/formik/issues/3913

Related pull request that have tried to solve the issue: https://github.com/jaredpalmer/formik/pull/3896 https://github.com/jaredpalmer/formik/pull/3774 (reverted by https://github.com/jaredpalmer/formik/pull/3775)

Zikoat commented 6 months ago

@jaredpalmer I might take a crack at this, and build upon the work in https://github.com/jaredpalmer/formik/pull/3896.

I have already done maybe 60% of the work, but i will have to get your approval that this is actually the behavior we want.

I am also wondering if you think we should bump the minor or major version for this? The types just assert what has been in the docs already, so there won't be very large migrations that have to be done, but in codebases that use Typescript loosely, we might want to recommend people to cast to as any for existing code that works at runtime but throws convoluted Typescript errors.

We also have to update the documentation, at least for Typescript. I dont have an overview of what has to be changed yet.

Zikoat commented 6 months ago

Ah, I see now that much of this has already been done in the v3 PR: https://github.com/jaredpalmer/formik/pull/3152 https://github.com/jaredpalmer/formik/issues/3099 https://github.com/jaredpalmer/formik/pull/3231 I dont think i will use time on this before Jared gives johnrom commit permissions.

Zikoat commented 6 months ago

Closing as duplicate of https://github.com/jaredpalmer/formik/issues/3099

johnsonav1992 commented 6 months ago

@Zikoat I see you closed this and said much of this had already been done. Are these previous PRs merged and have they been released to production?

cc @johnrom

Zikoat commented 6 months ago

@johnsonav1992 No, they aren't merged. The changes are published to npm https://www.npmjs.com/package/formik?activeTab=versions @3.0.0-next.8, and i think it might be pretty stable, but the release isnt officially supported.

We are moving to react-hook-form now.

johnsonav1992 commented 6 months ago

Got it. Yeah, I understand your choice. We are hoping for good things from TanStack Form soon!

jaredpalmer / formik