jaredpalmer / formik

Build forms in React, without the tears 😭
https://formik.org
Apache License 2.0
33.77k stars 2.78k forks source link

v2.3.0 breaks TypeScript linting on `push()` inside of `<FieldArray>` #3797

Closed kylemilloy closed 1 year ago

kylemilloy commented 1 year ago

Bug report

Current Behavior

In the following code:

<Formik
  initialValues={{
    people: [{ name: '' }]
  }}
>
  <FieldArray name="people">
    {({ form, push }) => (
      {form.values.people.map(({ name, index }) => {
        <Field key={`people-${index}`} name={`people.${index}.name`}>
          {/* ... */}
        </Field>
      })}

      <Button onClick={() => push({ name: '' })}>
        Add user
      </Button>
    )}
  </FieldArray>
</Formik>

We get a build error that the first prop of push() in the button must be of type unknown[] and does not adhere to the expected prop ({ name: '' }) whereas before it used to be any.

Expected behavior

This is a default put in place by ArrayHelpers and I don't think it's the right type for this use case. If there were a spread operator on it, it might make more sense such as:

push(...args: unknown[])
push(...args: never[])

Which would be more in line with the standard arr.push(...args) method. This is the change in question: https://github.com/jaredpalmer/formik/commit/73de78d169f0bc25bd84dff0beaed3cc7a2cbb11#diff-6e6b5046e39251f2d076677d7c80216dd3fe9311dff284336c2589646fe3efe0R33

Suggested solution(s)

Revisit the type for some of the array helper methods.

Your environment

Software Version(s)
Formik 2.3.2
React 17.0.2
TypeScript 4.7.x
kylemilloy commented 1 year ago

Should be noted that wrapping push(obj) so it becomes push([obj]) results in wonky behaviour when then using remove() and breaks validation.

quantizor commented 1 year ago

Agreed, will update the types.

kylemilloy commented 1 year ago

Not fixed. I think the issue here is that the obj param that push takes should be a spread operator, then the type of any[] makes sense as it can take multiple inputs such as the "native" javascript push method:

/**
 * @param items items to add to the array
 * @return New length of array 
 */
push(...items: T[]): number

Edited: more deets.

This is still causing build errors after #3798

The current implementation allows for customization here:

export interface ArrayHelpers<T extends any[] = any[]> {
  push<X = T[number]>(obj: X): void;

  // more...
}

And this can be customized when we do:

<FieldArray name="people">
  {({ push }: ArrayHelpers<{ name: string }>) => (
    <button type="button" onClick={ () => push({ name: '' })}>
      Add person
    </button>
  )}
</FieldArray>

However FieldArray is passed an instance of FieldArrayRenderProps which takes a generic ArrayHelpers type and so we cannot customize the type as:

<FieldArray name="people">
  {({ push, form }: FieldArrayRenderProps<{ name: string }>) => ( // error here, FieldArrayRenderProps cannot take a type
    <button type="button" onClick={ () => push({ name: '' })}>
      Add person
    </button>
  )}
</FieldArray>
kylemilloy commented 1 year ago

Huh...or...maybe fixed? I just nuked my node_modules and did a clean install and now it's working as intended so maybe I'm just stupid?

I swear this was throwing an issue for me where it was saying:

push({ name: '' }) // err, { name: string } is not compat with any[]

🧐

Lemme revisit and confirm this is still whackadoo

kylemilloy commented 1 year ago

Yeah...must have been my version sticking? I dunno...weird. Either way...rescinded. Works perfect. Thanks for the PR @probablyup