airjp73 / rvf

Easy form validation and state management for React and Remix
https://rvf-js.io
MIT License
827 stars 66 forks source link

[Bug]: All server errors get cleared after focus changes or any field value changes #390

Open david-arteaga opened 2 days ago

david-arteaga commented 2 days ago

Which packages are impacted?

What version of these packages are you using?

Please provide a link to a minimal reproduction of the issue.

https://stackblitz.com/edit/remix-run-remix-g89vpl?file=app%2Froutes%2F_index.tsx

Steps to Reproduce the Bug or Issue

Go to https://stackblitz.com/edit/remix-run-remix-g89vpl?file=app%2Froutes%2F_index.tsx

Instructions:

  1. Enter valid email
  2. Enter valid password (at least 6 chars)
  3. Press enter while focus is in password field

Form will load, get errors from backend, but then email field will be focused and server errors will be cleared for all fields. This happens whenever the following 3 are true:

  1. @rvf/react focuses the first field with errors
  2. There was another field focused before
  3. These are not the same field

This can be confirmed by viewing the error history below.

If you press "Create account" without focus being in any field, then errors show up correctly. But again, as soon as you focus another field, focus no field, or change the value in 1 field, all backend are cleared.

Expected behavior

Ideally all server errors would be kept visible until that field is modified.

I think it's support for server validation errors should be as good as client side validation errors.


Related to this: It would be great (and I think kinda necessary if this library is meant to be independent of Remix) if it's also possible to show server validation errors on specific fields when not using remix actions. When only using @rvf/react, I couldn't find any docs mentioning how to support server validation errors (after the form is submitted). I tried using serverValidationErrors (which btw gives a type error when useForm is imported from @rvf/remix, but works when it's imported from @rvf/react), but it has the same issue described above: all the errors are cleared when focus changes or a field is modified.

react-hook-form has the setError API and has the behavior I described above about errors not clearing until a field is changed.

Screenshots or Videos

No response

Platform

Additional context

No response

airjp73 commented 2 days ago

Hi!

The current behavior is intentional, but this is definitely somewhere we can make improvements. Definitely open to ideas and discussion here!

serverValidationErrors and validationError are primarily geared towards progressive-enhancement style errors. The expectation here is that the validator on the server is the same as the validator on the client. So the client removes those the next time it validates because the errors are no longer present.

With Remix, I've personally found these sorts of server-side validations are pretty easy to simply return from the action and display manually, but I can see the appeal of being able to write these directly into the server-side validator as an async refinement.

I already have a few ideas for how to implement either a setCustomError API or some kind of proper async validation API on the client, so maybe it's worth adding a server-side API for that, too. I'm not sure if this would combo naturally with async refinements though.

david-arteaga commented 2 days ago

So regarding the Remix action validationError behavior of clearing all errors on focus change/value change, I do think that should be changed. I don't think it's reasonable to expect client and server async validation will always be the same. There are things you might want to/can only validate only on the server at insert/update time (like db constraints) , and not on every keystroke from the client. I think that's a pretty general use case that right now is not well supported.

I do see the appeal of assuming the validator will always be exactly the same and that any server errors can be effectively ignored on any change since it greatly simplifies what the library has to deal with, but I don't think it appropriately reflects real world use.

Displaying errors directly returned from Remix actions seems like a pretty involved workaround to be honest, since you'd have to basically always have 2 possible sources of errors (server errors and errors managed by RVF) and keep track of field changes to stop displaying server errors for a field when that particular field's value changes (which kind of is the reason we use form libraries instead of rolling our own thing).


And yeah I mentioned serverValidationErrors because it was the only thing I found that could in any way let me include server errors when not using Remix. Is there any plan to add support for displaying server validation errors when using only React and not Remix? Like I mentioned I do think this is pretty important if support outside of Remix is a goal for RVF.

airjp73 commented 1 day ago

And yeah I mentioned serverValidationErrors because it was the only thing I found that could in any way let me include server errors when not using Remix. Is there any plan to add support for displaying server validation errors when using only React and not Remix?

The remix useForm hook actually uses serverValidationErrors under the hood. The only difference between the two is that, in remix, we're able to grab the errors automatically, which isn't possible in the general case for all React frameworks.


So regarding the Remix action validationError behavior of clearing all errors on focus change/value change, I do think that should be changed. I don't think it's reasonable to expect client and server async validation will always be the same.

Having the ability to re-use your client validation on the server is a core value proposition of RVF. There are definitely use-cases where it makes sense to create a separate server validator (I do this a lot with different subactions in a single action function). But validator re-use is the default case.

This is also important for progressive enhancement. Imagine a case where a user with a poor internet connection submits a form before JS has loaded. RVF can still handle that because the validator on the server will run and the action will return the validation errors that would normally be surfaced on the client-side.

This is why validation behaves the same with server errors as it does with client-side errors. If it suddenly behaved differently in this case, that would be a bug.

There are things you might want to/can only validate only on the server at insert/update time (like db constraints) , and not on every keystroke from the client.

This is true. In those cases though, you should be re-running your client-side validations then perform your server-side validations. I do this by validating with the validator, then doing other checks. But you could build a separate validator that contained all the client-side validations + the server validations if you wanted.

If you don't run your client-side validations on the server, a tech-savy user can easily get around them by calling formElement.submit() directly from the console (or with a manual fetch).


So the behavior we have can't really change, but we can add new functionality.

One idea I had was something like this. If we went this route, it would be complimented by a customValidationErrors prop in the React useForm. I'm not sure when/if we could automatically clear these errors, though.

// Run validations as before
const result = await validator.validate(await request.formData())
if (result.error) return validationError(result.error, result.submittedData);

// Return custom errors explicitly
const isEmailTaken = await checkIfEmailTaken(result.data.email);
if (isEmailTaken) return customError({ email: "That email is taken" });
david-arteaga commented 1 day ago

This is why validation behaves the same with server errors as it does with client-side errors.

I don't think that's actually the case though, because on the client all errors don't get cleared when focus changes or a single field value changes (I know this is actually what is implemented since the validator runs on every keystroke, but the resulting behavior is not this, and users don't experience client validation error messages disappearing on focus, which does happen with server errors). This leads to having behaviors like this:

https://github.com/user-attachments/assets/ee1525b6-92cf-405f-aa17-393b42bf08f1

In this video server validation errors aren't displayed after the first submission (done by pressing Enter) because focus changes. The server errors are displayed on the second submission because no field was focused, and RVF only focused the email field, which apparently doesn't trigger clearing the server errors.

Admittedly arguing validation doesn't behave the same with server vs client errors is a bit of a stretch because the implementation is similar. But the final behavior is not at all the same, and I'd say the server validation behavior is not correct (see video above). This behavior assumes no devs will ever want to run any additional validations on submit on the server.

What I'm proposing is kind of a compromise where we admit that RVF does accept the fact that some devs could have additional validations on the server that only run when the form is submitted, and that those errors shouldn't go away when a user focuses another field or changes another un-related field. The implementation of this could look like keeping track of server errors and removing them for fields only after they are modified. That doesn't necessarily mean the error will not happen on the server, but I think it's the most reasonable thing (btw it's the same thing that react-hook-form does with it's setError API).

None of this would break server validations or non-JS users. I'm not sure why you mentioned this. The case I'm making that server validations could have additional validations to the ones the client does (not fewer validations). Right now RVF doesn't support this.

I'm not sure when/if we could automatically clear these errors, though.

Yeah this is true. You'd be guessing. But like I mentioned I do think it's reasonable to assume that when the field value changes you can clear the server validation error. That enabled what I think is what most devs would expect from a form library. Or more complex behaviors could be implemented by default or be configurable (like not showing the server validation error when the field value is different from what was submitted to the server, not just clearing it when the field value is changed).

airjp73 commented 1 day ago

I get the feeling we're talking past each other a bit. 🫤

What I'm proposing is kind of a compromise where we admit that RVF does accept the fact that some devs could have additional validations on the server that only run when the form is submitted, and that those errors shouldn't go away when a user focuses another field or changes another un-related field.

I'm right here with you. RVF could have better support for this. And I'm open to discussion on API ideas.

It's just that changing the way RVF currently treats server errors is off the table. Returning custom server errors needs to be explicit, IMO.

david-arteaga commented 1 day ago

Ok yeah I get that sentiment. Does that mean you’re ok with the way RVF behaves for the video example I uploaded in my previous comment?

—-

I feel like having a whole separate API just for server validation errors that “persist” is a bit too complex, but it’s a valid concern to want to keep it explicit and separated.

And you’re thinking that this API would be the one React users have to use when they’re not using Remix and want to show server validation errors on specific fields?

airjp73 commented 1 day ago

Does that mean you’re ok with the way RVF behaves for the video example I uploaded in my previous comment?

Those errors would use this hypothetical new API, so the behavior in the video becomes a non-issue.

I feel like having a whole separate API just for server validation errors that “persist” is a bit too complex, but it’s a valid concern to want to keep it explicit and separated.

I definitely feel this. Fewer APIs is usually preferable, so a new API needs a compelling reason to exist.

As a reference point, I think if we can come up with something better than returning the custom errors from json manually, then it's worthwhile because that's the approach I usually recommend and use.

Worth thinking about if we can fit form-level errors into this too (only a nice-to-have). That's been requested before and I usually give the same recommendation.

And you’re thinking that this API would be the one React users have to use when they’re not using Remix and want to show server validation errors on specific fields?

It would be for both.

david-arteaga commented 1 day ago

And what are you thinking for how those server validation errors from the new API get dismissed once the form values start to be modified again on the client? Would the developer have to explicitly set some config or manipulate an API for every field so it clears/stops displaying server validation errors?

airjp73 commented 1 day ago

That's definitely an option. Any given form likely only needs one or two custom server errors so the burden there is small. It's certainly less work than managing your own state.

One potential API design is to tie this into a hypothetical setCustomError API for custom errors on the client side. That API would require the error to be cleared manually. In this version of the API, returning a custom error from the backend would behave just like calling setCustomError with that error.

I haven't implemented a setCustomError api yet because it feels like a low-level tool. One common use-case of that might be for async validations on the client side (e.g. checking if the email is taken as soon as the user stops typing). But I think that kind of validation needs more support than just a setCustomError.

david-arteaga commented 1 day ago

what do you think about a setCustomError that optionally lets you directly specify when the custom error should get cleared? with manual being the default, meaning you have to directly call something like clearCustomError or setCustomError(field, undefined) but also having options like onChange or onBlur for simple use cases where you want RVF to manage getting rid of the custom error.

airjp73 commented 23 hours ago

That might be okay. The only thing I might worry about there would be API creep.

A rough idea of the server-side api would need to cover at least this. I don't love how verbose it is, but we can optimize from here.

return customError({
  // We're able to supply formId automatically with `validationError`,
  // so maybe there's a way to do that here so you don't have to remember yourself.
  formId: validationResult.formId,

  errors: {
    // for manual validation
    email: "That email is taken",

    // or with custom options
    password: {
      password: "Password error",
      clearOn: "change"
    }
  },
  submittedData: omitPassword(validationResult.submittedData)
});