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

Add correct types for Field #3927

Closed
Zikoat opened this issue Dec 14, 2023 · 6 comments
Closed

Add correct types for Field #3927

Zikoat opened this issue Dec 14, 2023 · 6 comments

Comments

@Zikoat
Copy link

Zikoat commented Dec 14, 2023

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:

#2086 (closed, but has gotten a lot of comments after it was closed)
#3885
#3913

Related pull request that have tried to solve the issue:
#3896
#3774 (reverted by #3775)

@Zikoat
Copy link
Author

Zikoat commented Dec 14, 2023

@jaredpalmer
I might take a crack at this, and build upon the work in #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
Copy link
Author

Zikoat commented Dec 14, 2023

Ah, I see now that much of this has already been done in the v3 PR: #3152 #3099 #3231
I dont think i will use time on this before Jared gives johnrom commit permissions.

@Zikoat
Copy link
Author

Zikoat commented Dec 14, 2023

Closing as duplicate of #3099

@Zikoat Zikoat closed this as completed Dec 14, 2023
@johnsonav1992
Copy link

johnsonav1992 commented Jan 2, 2024

@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
Copy link
Author

Zikoat commented Jan 2, 2024

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

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

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

No branches or pull requests

2 participants