Fix/array field validation

[gutentag2012]

This PR closes #662

Any Array Field helper method now also runs the onChange validation of the Form and the field.
Furthermore, there is now a new method to validate a single field on a form.

[nx-cloud]

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 43dff36. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution

---

✅ Successfully ran 1 target

• nx affected --targets=test:format,test:sherif,test:knip,test:eslint,test:lib,test:types,test:build,build --parallel=3

---

Sent with 💌 from NxCloud.

[Balastrong]

A couple of questions:

1. We're now explicitly adding change validation on all methods, is there a reason why we're not adding it only to setFieldValue since all other methods are calling it anyway?

2. We're now running the validation on all changes which as a side effect sets touch: true on the field.
   Does that mean that form.pushFieldValue("name", "", { touch: false }) would still set my field as touched due to the validation?

[gutentag2012]

A couple of questions:

1. We're now explicitly adding change validation on all methods, is there a reason why we're not adding it only to setFieldValue since all other methods are calling it anyway?
2. We're now running the validation on all changes which as a side effect sets touch: true on the field.
   Does that mean that form.pushFieldValue("name", "", { touch: false }) would still set my field as touched due to the validation?

1. I did that since I was unsure if validation is a wanted side-effect for the setFieldValue method. But I guess it would make sense to move the change validation there since that effectively is a handleChange method for the form
2. Oh yes you are right, that should not be the case. For the validateField method I copied the existing validation logic which sets the isTouched to true whenever validation runs. So in that case using form.pushFieldValue("name", "", { touch: false }) would validate and set the isTouched=true either way

I will fix 2, as for 1 I am open to move validation to the setFieldValue if that is wanted

[Balastrong]

1. I did that since I was unsure if validation is a wanted side-effect for the setFieldValue method. But I guess it would make sense to move the change validation there since that effectively is a handleChange method for the form

Ok! Just wanted to make sure this was intentional. Let's also wait for more reviews before doing any change here as it might be ok this way.

2. Oh yes you are right, that should not be the case. For the validateField method I copied the existing validation logic which sets the isTouched to true whenever validation runs. So in that case using form.pushFieldValue("name", "", { touch: false }) would validate and set the isTouched=true either way

I will fix 2, as for 1 I am open to move validation to the setFieldValue if that is wanted

Great, thanks!

[gutentag2012]

How do we want to handle the following case:

We have a form with an onChange validation and do a form.pushFieldValue("name", "", { touch: false }). That should still run the validation on the form, since the form has no touched state for validation correct?

[Balastrong]

How do we want to handle the following case:

We have a form with an onChange validation and do a form.pushFieldValue("name", "", { touch: false }). That should still run the validation on the form, since the form has no touched state for validation correct?

I see you removed setting touched: true on validation, I think that was correct though. Since validateAllFields also sets touched, the single validation should do it too.

My point was more about pushFieldValue (and the other methods): should we run the validation if you pass { touched: false }?
If the answer is yes, your code is ok (by adding back the part you removed) but if the answer is no, we should also apply this condition to field.setValue since it is always running the validation now.

[gutentag2012]

I see you removed setting touched: true on validation, I think that was correct though. Since validateAllFields also sets touched, the single validation should do it too.

I think it is better that way, because now validation only runs if the fields have been touched either through the { touch: true } option or because they have been touched before.
Having the validation method implicitly setting the touched state feels a bit off to me

My point was more about pushFieldValue (and the other methods): should we run the validation if you pass { touched: false }?

To answer your question, no I don't think validation should run if the field should not be touched (at least the field validation, the form validation should still run)

By keeping the code as it is now I do the same thing as the field.SetValue function, since that first sets the isTouched state depending on the options and then runs validation. If it was touched during that process, the validation will abort early.
This is the same as the array field helpers are doing it now.

[fulopkovacs]

I think it is better that way, because now validation only runs if the fields have been touched either through the { touch: true } option or because they have been touched before.
Having the validation method implicitly setting the touched state feels a bit off to me

@gutentag2012 We talked about the {touch: boolean} API among the maintainers because it's kind of confusing. The thing is, it's an API that is supposed to be used internally for testing. Right now this is not obvious at all, but in the future, we're planning to default it to true everywhere and rename it to something else.

I think we should go back to following the example of validateAllFields() for now and set isTouched to true for the validated field. This will also spare us from running the validate() method of the form where validate a field, because a field's validate() method will run the form's validate() function anyways.

So this this.validate('change') will become redundant:

form/packages/form-core/src/FormApi.ts

Lines 708 to 709 in de8d8aa

	this.validate('change')
	this.validateField(field, 'change')

because the field's validate() method is called here:

form/packages/form-core/src/FormApi.ts

Line 381 in de8d8aa

return fieldInstance.validate(cause)

and it runs validate() on the form if isTouched is true for the field:

form/packages/form-core/src/FieldApi.ts

Lines 716 to 720 in de8d8aa

	if (!this.state.meta.isTouched) return []
	try {
	this.form.validate(cause)
	} catch (_) {}

Thanks for your patience and hard work btw! 🙌 If this issue gets resolved, we can merge the PR!

Notes on testing

The tests will need to be updated after this change, see how 👇

Note that if you don't run the form's validate() function explicitly, you'll need to create a field instance and mount it for the new tests, otherwise they'll fail. For example, the "should run onChange validation when pushing an array field's value" test would look like this:

  it("should run onChange validation when pushing an array field's value", () => {
    const form = new FormApi({
      defaultValues: {
        names: ['test'],
      },
      validators: {
        onChange: ({ value }) =>
          value.names.length > 3 ? undefined : 'At least 3 names are required',
      },
    })

    const field = new FieldApi({
      name: 'names',
      form,
    })

    form.mount()
    field.mount()
    form.pushFieldValue('names', 'other')

    expect(form.state.values).toStrictEqual({ names: ['test', 'other'] })
    expect(form.state.errors).toStrictEqual(['At least 3 names are required'])
  })

[gutentag2012]

Oh, I see, that does make sense.
I will make the validateField method then also set the touched state and remove the explicit this.validate("change") for the form.

I think there might be an issue with the validation for the insertFieldValue and the swaptFieldValues and moveFieldValues. I will verify that and fix that if that is the case.

[gutentag2012]

Okay, so there was a bigger issue where nested fields within the arrays where not validated properly and removeFieldValue validating all fields within the form despite that not being necessary.

So I added a new method validateArrayFieldsStartingFrom to validate nested fields within an array starting from a specified index, that is now used in the removeFieldValue and insertFieldValue methods, since they required all shifted fields to revalidate the validation.

[codecov]

Codecov Report

Attention: Patch coverage is 97.50000% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 90.36%. Comparing base (8e2335e) to head (43dff36).
Report is 27 commits behind head on main.

Files	Patch %	Lines
packages/form-core/src/FormApi.ts	97.36%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #700      +/-   ##
==========================================
+ Coverage   89.95%   90.36%   +0.41%     
==========================================
  Files          32       32              
  Lines         896      955      +59     
  Branches      195      206      +11     
==========================================
+ Hits          806      863      +57     
- Misses         84       86       +2     
  Partials        6        6              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[fulopkovacs]

Thanks a lot for your work, I'm merging it! 🚀