-
-
Notifications
You must be signed in to change notification settings - Fork 479
feat(core): add getAllErrors to form api #1155
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
Conversation
View your CI Pipeline Execution ↗ for commit 34c00fa.
☁️ Nx Cloud last updated this comment at |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1155 +/- ##
==========================================
+ Coverage 88.32% 88.41% +0.08%
==========================================
Files 27 27
Lines 1199 1208 +9
Branches 315 319 +4
==========================================
+ Hits 1059 1068 +9
Misses 125 125
Partials 15 15 ☔ 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 for the great PR!
I think we're on the right direction but we can probably make it even better. Here I see you're returning the flat errors
array for the form but the validationMap
for the fields. We should probably return both for both.
What do you think about a return type similar to that:
{
form: { errors: ValidationError[]; errorMap: ValidationErrorMap }
fields: Record<
DeepKeys<TFormData>,
{
errors: ValidationError[]
errorMap: ValidationErrorMap
}
>
}
Feel free to make the types smarter if you think it's needed, the overall idea is to return both errors and errorMap for form and each field :)
This also somewhat ties into my PR where we need to determine which field errors came from form validations vs field validations. I think if we're getting all errors in a function like this, it'd be nice to know where the error came from as well. If this could be included within this functionally we could also use it internally to clear stale field errors sourced from the form. |
2f83be8
to
e39acb0
Compare
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.
Well done! I think we're almost there :)
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.
LGTM!
Do you want to add a few more unit tests to cover edge cases and also some type tests? Otherwise we can just merge and add them later, let me know :)
@Balastrong as always it's been a pleasure. Normally, I would add the tests but I've got three open pr's and it's getting hard to track and keep them up to date, plus I'm away at the moment. So if we can merge it and I'll come back to the tests in two weeks or so that would work best for me. |
Oki, no worries, I added a type test :) Thanks again, merging it now 🚀 |
Add's a utility function to form.api to return form errors combined with a reduced fieldMeta.errorMap.
example
Tasks