-
-
Notifications
You must be signed in to change notification settings - Fork 326
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
Fix/array field validation #700
Fix/array field validation #700
Conversation
… add missing moveFieldValues test
☁️ Nx Cloud ReportCI 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 targetSent with 💌 from NxCloud. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of questions:
-
We're now explicitly adding
change
validation on all methods, is there a reason why we're not adding it only tosetFieldValue
since all other methods are calling it anyway? -
We're now running the validation on all changes which as a side effect sets
touch: true
on the field.
Does that mean thatform.pushFieldValue("name", "", { touch: false })
would still set my field as touched due to the validation?
I will fix 2, as for 1 I am open to move validation to the |
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.
Great, thanks! |
How do we want to handle the following case: We have a form with an |
I see you removed setting My point was more about pushFieldValue (and the other methods): should we run the validation if you pass |
I think it is better that way, because now validation only runs if the fields have been touched either through the
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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'])
})
Oh, I see, that does make sense. I think there might be an issue with the validation for the |
Okay, so there was a bigger issue where nested fields within the arrays where not validated properly and So I added a new method |
Codecov ReportAttention: Patch coverage is
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for your work, I'm merging it! 🚀
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.