-
-
Notifications
You must be signed in to change notification settings - Fork 365
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
feat(core): add getAllErrors to form api #1155
base: main
Are you sure you want to change the base?
Conversation
View your CI Pipeline Execution ↗ for commit 51a9b8d.
☁️ 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 87.86% 88.00% +0.14%
==========================================
Files 30 30
Lines 1318 1334 +16
Branches 354 356 +2
==========================================
+ Hits 1158 1174 +16
Misses 144 144
Partials 16 16 ☔ 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 :)
return { | ||
form: { | ||
errors: this.state.errors, | ||
errorMap: reduceErrorMap(this.state.errorMap as ValidationErrorMap), |
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.
@Balastrong where is FormValidationErrorMap used as Record<ValidationErrorMapKeys, FormValidationError>
?
Im guessing when the form validation returns an object of validations. I don't feel particularly keen on leaving it as ValidationError | FormValidationError, Is there a utility type where this is narrowed?
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.
Right, that type is gonna change a bit in #1104
In this case we should probably wait for the other PR to land first
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.
yeah, and increase the type complexity quite some too 😂
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. |
Add's a utility function to form.api to return form errors combined with a reduced fieldMeta.errorMap.
example
Tasks