-
-
Notifications
You must be signed in to change notification settings - Fork 339
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: set field errors from the form validators #656
feat: set field errors from the form validators #656
Conversation
☁️ Nx Cloud ReportCI is running/has finished running commands for commit a43feb6. 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 2 targetsSent with 💌 from NxCloud. |
d11c8f8
to
0f5bc30
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #656 +/- ##
==========================================
- Coverage 91.55% 89.90% -1.65%
==========================================
Files 21 26 +5
Lines 900 981 +81
Branches 206 229 +23
==========================================
+ Hits 824 882 +58
- Misses 71 93 +22
- Partials 5 6 +1 ☔ View full report in Codecov by Sentry. |
Hi. When are you planning to release it? Thanks. |
I'm planning to pick this up during the weekend. 😅 Unfortunately there are some urgent things that I have to take care of in the |
@fulopkovacs Perfect. Thank you. This is indeed necessary. I migrated from react-hook-form, which had a setError function that allowed setting errors when the backend responded with errors. It was surprising that this library doesn't yet support something similar. I think it's a very common use case to have backend validation and then set field-level validation errors. |
9451f08
to
f47a529
Compare
Sad status update: I ran into some issues as I started adding more complex tests. This is a bummer because I wanted to get this one ready during the weekend. 😢 |
@fulopkovacs It's ok :) |
@fulopkovacs Hey :) You're probably busy, so I'm sorry to bother you, but do you know when you will be able to get back to this PR? Without this solution, I will likely need to find a workaround for properly handling backend errors, or switch back to react-hook-form |
Hey @denisorehovsky, just as a heads up - we don't follow any kind of release schedule or cadence. As we're all volunteer based, we're only able to take on tasks as we have time. Maybe you could consider sponsoring the project financially or making a PR if you're needing priority support? 😄 Moreover, though, I'm not sure I understand how this is a blocker for anything. This PR is a nicety, not a required feature to merge form and field errors. You can always do that manually by using a helper component. |
Hey I'm actively working on this PR! The process is slow, because there are many scenarios to handle. All the After all the Then, the PR will go to review, but unfortunately, I don't expect that to be very quick (it's gonna be a beefy PR), and there's always the chance that the approach I chose doesn't fit the project well enough. 😬 In short, I'm sorry, but this will likely take some time. (Honestly, it's more complex than I expected it to be, I thought I'd be ready by the end of last week.) Finding a workaround is a more realistic solution than waiting for this feature to be merged. For example, you can handle the form's validation on the server with the form's In situations like this, I'm really sad that it's not my full-time job, it'd be awesome to respond to the users' needs more quickly! 😢 Thanks for your understanding though. Edit: The tests are finally passing, but there are still some smaller tasks I need to do before the review. Check the PR's updated description for more details! |
b0c1d47
to
dbeb7d6
Compare
Hi all, this feature would be amazing and is very critical, I'll be happy to help but I cannot see what's blocking its merge. |
Hey 👋! It's waiting for a rebase (by me) before others in the team can review it. Unfortunately too many things have been changed, so it doesn't make sense to change the order of these things. I was away from the keyboard for 2 weeks, and I won't be able to work on this during the workweek, but I'll look into it on the weekend! (Can't promise anything though. A lot of PRs have been merged since I've finished this one. I hope it'll go smooth! 🤞) |
c42edc9
to
9615f90
Compare
@@ -1105,11 +1106,10 @@ describe('field api', () => { | |||
field.mount() | |||
|
|||
field.setValue('one') | |||
// Allow for a micro-tick to allow the promise to resolve | |||
await sleep(1) | |||
await vi.runAllTimersAsync() |
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.
After the changes in the validators, the number of micro-ticks needed also increased in some places. Instead of calculating the exact number of micro-ticks we need at each place, I decided to use fake timers and vi.runAllTimersAsync
.
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.
Just a couple of comments in the docs, looks great!
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!
Current status of this PR: we're gonna have one more review by @crutchcorn. (I introduced some serious changes in the validation, so the more people see it before the merge the better.) |
Just thinking out loud here: This introduces a new behavior that many might consider a "bug". This behavior would occur when the following pattern emerges in a codebase: export default function App() {
const form = useForm({
defaultValues: {
age: 0,
},
validators: {
onChange: ({ value }) => {
return {
fields: {
age: value.age < 12 ? 'Too young!' : undefined,
},
}
},
},
})
return (
<div>
<h1>Field Errors From The Form's validators Example</h1>
<form
onSubmit={(e) => {
e.preventDefault()
e.stopPropagation()
void form.handleSubmit()
}}
>
<form.Field
name="age"
validators={{
onChange: ({ value }) => value % 2 === 0 ? 'Must be odd!' : undefined,
}}
children={(field) => (
<div>
<label htmlFor={field.name}>Age:</label>
<input
id={field.name}
name={field.name}
value={field.state.value}
type="number"
onChange={(e) => field.handleChange(e.target.valueAsNumber)}
/>
{field.state.meta.errors.length > 0 ? (
<em role="alert">{field.state.meta.errors.join(', ')}</em>
) : null}
</div>
)}
/>
<form.Subscribe
selector={(state) => [state.canSubmit, state.isSubmitting]}
children={([canSubmit, isSubmitting]) => (
<button type="submit" disabled={!canSubmit}>
{isSubmitting ? '...' : 'Submit'}
</button>
)}
/>
</form>
</div>
)
} Where the developer might want to get two different errors at the same time, but is only reported one of the errors due to how we have this edgecase handled in our code. Now, to be clear, I don't think this is actually a huge problem - but:
Continuing my review otherwise ignoring this |
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.
The code LGTM and has sane defaults/behaviors - you've done an amazing job with this PR. Prior to merging, let's update the docs to reflect what I was talking about with the edgecase.
I can tackle that tho
…m/fulopkovacs/form into field-errors-from-form-validators
This PR adds the ability to set field errors from the form validators.
The API would look like this (this is a value returned by a validator):
We'll add support for Zod, Yup, and Valibot in a separate PR.
TODO
form-core
main