formik icon indicating copy to clipboard operation
formik copied to clipboard

[v2] removeItem puts empty array in in error state and causes isValid to be incorrect

Open TLadd opened this issue 5 years ago • 14 comments

🐛 Bug report

Current Behavior

When I call FieldArray's removeItem, it sets the error state to an empty array, and it stays that way until validate is run again. Having a key with a truthy value results in isValid being calculated as true, when the form is actually still valid.

This codesandbox illustrates the issue: https://codesandbox.io/s/formik-example-uez7v. When you click the X next to one of the items, the errors state is Object {arr: Array[0]} and isValid is false.

Somewhat of a duplicate of https://github.com/jaredpalmer/formik/issues/784, but the behavior has changed to be a bit worse. Before, this was just a blip that ended up being cleaned up on a future render, presumably because validate was being triggered immediately and overwriting the error state. Now, no such rerunning of validation is occurring.

Expected behavior

I would expect that the error state for array fields isn't automatically set to [], which would then also solve the isValid problem.

Reproducible example

https://codesandbox.io/s/formik-example-uez7v.

Your environment

Software Version(s)
Formik v2.0.1-rc.5
React 16.8.6
TypeScript N/A
Browser Chrome
npm/Yarn N/A
Operating System OS X

TLadd avatar Jun 19 '19 15:06 TLadd

I can confirm this wrong behavior

sibelius avatar Aug 12 '19 21:08 sibelius

As mention in this https://github.com/jaredpalmer/formik/issues/784#issuecomment-503135849. You need to validate manually after removing item.

bichoymessiha avatar Sep 09 '19 17:09 bichoymessiha

This happens to me also, regardless of validateOnChange being set or not. I tried validating manually after removing item, but the error state returns the next time I add and remove an item.

lightitnow avatar Oct 23 '19 15:10 lightitnow

simple helper to fix this until the proper fix lands

type SetFieldValue = (field: string, value: any, shouldValidate?: boolean) => void;
export const arrayHelpersRemove = <T extends any>(
  name: string,
  values: object,
  index: number,
  setFieldValue: SetFieldValue,
) => {
  const arr = values[name];

  const newArr = [...arr.slice(0, index), ...arr.slice(index + 1)];

  setFieldValue(name, newArr);
};

arrayHelpersRemove('list', formik.values, i, formik.setFieldValue);

it could be turned in a hook

sibelius avatar Oct 23 '19 15:10 sibelius

@sibelius Thanks! That worked, but arrayHelpers.remove does more than that right? Btw, could PR #1782 be a fix for this?

lightitnow avatar Oct 23 '19 15:10 lightitnow

Hey! I worked on #1782. The issue described here is exactly what prompted me to investigate and submit that PR. I found similar issues with some of the other FieldArray methods, as well. I also discovered validateOnChange was not working, so I tried to fix that too, though my solution required updating the setFormikState typedef.

inv8der avatar Nov 14 '19 00:11 inv8der

Apart from cleaning empty array on errors there is one more issue.

If only some objects in the array had errors, ie.

{"errors": {"friends": [null, null, "invalid friend"]}}

then removing the field with error leaves:

{"errors": {"friends": [null, null]}}

@inv8der to solve this you might need to change line: https://github.com/jaredpalmer/formik/pull/1782/files#diff-7d037a4310f1022dee96554929bd14b3R275

copy.filter(err => err !== '' && err !== null).length > 0 ? copy : undefined;

Thanks for the PR!

KrzysztofMadejski avatar Nov 29 '19 18:11 KrzysztofMadejski

I am also having the same problem. Waiting for the update.

imdadul avatar Dec 17 '19 16:12 imdadul

The same problem is applicable to 'touched' state of array field. Field state gets removed from 'touched' object as soon as the last item is removed. That's not right.

gmltA avatar Feb 18 '20 12:02 gmltA

+1 .. I am also facing the same problem.

asvny avatar Jun 22 '20 07:06 asvny

+1

tyteen4a03 avatar Jul 30 '20 16:07 tyteen4a03

+1

h4t0n avatar Nov 20 '20 09:11 h4t0n

I've worked out what's going on:

  • The FieldArray helpers do more than just update the current value. They shift existing errors and touched state around because the indexes have changed (e.g. if I remove the first item, I want the touched states for sub-fields to all shift by 1)
  • For most of the array methods, this works by applying the same method to the value, the errors and the touched state
  • If the errors or touched state are initially undefined, trying to apply the same method does some strange things - it falls back to an empty array, but then uses splice on that, so it ends up expanding the empty array to an array of undefined
  • It then does a check against the empty array, but that just checks array length, rather than for the array items all being undefined/null

As a workaround, you can set validateOnChange to true for a FieldArray - then it will automatically tidy up the broken generated errors by triggering a revalidate (which is what @sibelius's fix was doing).

I had a further issue here in that that only works when the values have actually changed, but you can trigger this by calling move with the source and destination index the same, so I had to ignore null moves.

The proper fix would be ignore updating the errors or touched state if they're undefined (while retaining the validateOnChange behaviour). If the touched state is undefined, there can't be anything to shift around.

i.e. something like

      const prevErrors = getIn(prevState.errors, name)
      let fieldError = alterErrors && !!prevErrors
        ? updateErrors(prevErrors)
        : undefined;
      const prevTouched = getIn(prevState.touched, name)
      let fieldTouched = alterTouched && !!prevTouched
        ? updateTouched(prevTouched)
        : undefined;

lexanth avatar Jun 01 '21 11:06 lexanth

Looking at https://github.com/jaredpalmer/formik/discussions/3526 and seeing that there are recent patch releases, what would be needed to address these bugs and get them released?

sergei-maertens avatar Nov 15 '23 13:11 sergei-maertens