-
-
Couldn't load subscription status.
- Fork 535
feat (form-core): Merging Form-level server errors with Field-level errors #1432
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
base: main
Are you sure you want to change the base?
feat (form-core): Merging Form-level server errors with Field-level errors #1432
Conversation
|
View your CI Pipeline Execution ↗ for commit e86715e
☁️ Nx Cloud last updated this comment at |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1432 +/- ##
==========================================
- Coverage 90.35% 89.20% -1.15%
==========================================
Files 38 38
Lines 1752 1834 +82
Branches 444 474 +30
==========================================
+ Hits 1583 1636 +53
- Misses 149 171 +22
- Partials 20 27 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…-validation-merged-with-field-errors
…-validation-merged-with-field-errors
…github.com/theVedanta/form into server-validation-merged-with-field-errors
|
Hello, do you need a hand to move this branch forward? I would love to see it happen :) |
Yeah sure dude, go for it! |
|
Not sure if this feature is still requested or whether it has been fixed. Don't wanna tag maintainers but this has been dormant for a while, I'd like some insight on how to proceed! @LeCarbonator @harry-whorlow |
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.
Sorry for the long delay! I'm not too confident in my SSR knowledge, so I'll make sure to give this a try myself. I'll also go through some issues and the reproductions to confirm things.
Hopefully, this initial review should help with some refactoring. It's looking promising!
Since this has been stale for way too long, this will be my focus for now. Feel free to tag me if there's more info you need or a second review. I can also help out with the PR if you'd like.
| validationSource: 'form', | ||
| })) as UnwrapFormAsyncValidateOrFn<TOnServer> | undefined | ||
|
|
||
| console.log('ON SERVER ERROR', onServerError) |
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.
Forgotten console.log
| const errorsArray = onServerErrorVal | ||
| ? Array.isArray(onServerErrorVal) | ||
| ? onServerErrorVal.map((err) => | ||
| typeof err === 'object' ? Object.values(err)[0] : err, | ||
| ) | ||
| : [ | ||
| typeof onServerErrorVal === 'object' | ||
| ? Object.values(onServerErrorVal)[0] | ||
| : onServerErrorVal, | ||
| ] | ||
| : [] |
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.
This double ternary is a bit hard to follow. Please refactor to if statements instead.
| const errorsArray = onServerErrorVal | ||
| ? Array.isArray(onServerErrorVal) | ||
| ? onServerErrorVal.map((err) => | ||
| typeof err === 'object' ? Object.values(err)[0] : err, |
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'll need to look into this one. Objects don't inherently have an order, so I'm not sure how consistent taking the first element would be. Perhaps we do this in other places?
The object format you're referring to is the server error fields coming in, right?
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.
There is actually an order of sorts nowadays: https://2ality.com/2015/10/property-traversal-order-es6.html
- Integer indices (array-like indices) come first, in ascending numeric order
- String keys (non-integer strings) come next, in creation order
- Symbol keys come last, in creation
|
The merge conflicts appear to be in some of the important diffs, so I'll leave that up to you. I'll see if convenient unit tests could be made for these as we don't want to regress on this. |
Your help with the PR would be awesome! I'm pretty new to learning about the tanstack ecosystem. |
|
Instead of the previous
serverValidatefunction just returning a string, we now return an object:This then maps to the field-level errors with the
setMetaoptions.The only reason this is called a preliminary approach is because we are setting the
onServerkey of the errorMap differently.Previously, the error map for the form looked like so:
Now however, the onServer map is slightly different:
This causes some degree of inconsistency, which is why I would love some feedback on whether we can use some other property (or maybe create our own!) to manage the errors between fields and forms. This would not be a comprehensive property but just a map to sync them back and forth.
NextJS support has been pushed. Remix and Start coming soon.
Support for Array field types will also be done shortly.
Would love your feedback and comments on this approach before I proceed!
Solves the following issue: #1260