Skip to content
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

[Feature]: support expected behavior of novalidate and formnovalidate #207

Closed
nrako opened this issue Nov 11, 2022 · 18 comments · Fixed by #372
Closed

[Feature]: support expected behavior of novalidate and formnovalidate #207

nrako opened this issue Nov 11, 2022 · 18 comments · Fixed by #372

Comments

@nrako
Copy link
Contributor

nrako commented Nov 11, 2022

What is the new or updated feature that you are suggesting?

Triggered by #139.

The attributes novalidate and formnovalidate are standardized HTML attributes which can respectively be set on a <form> or a submitter (<input> , <button>) to indicate that the form is not to be validated during submission. Their React JSX counterparts are noValidate and formNoValidate.

In remix-validated-form, noValidate is used to disable native HTML validations to not conflict with the JS validation. It is currently recommended to use <ValidatedForm noValidate={useHydrated()} to only use the native HTML validation when JS is disabled, when <ValidatedForm is not hydrated. As for formNoValidate, it is currently completely ignored.

Therefore, there's currently no way to submit a <ValidatedForm without validation when it is hydrated.

As discussed in #139, noValidate and noFormValidate would benefit by remaining closer to their web standard behavior as both – but especially the latter one – can be needed to implement certain use cases.

As pointed by @airjp73 in #139 the following guidelines should however be followed:

  • noValidate and formNoValidate should have the same behavior (could also remove noValidate from the form component and make it an implementation detail)
  • Need to support having native validation when not hydrated and disabling that when hydrated
  • If it's a breaking change, we need to evaluate how much churn it introduces vs how compelling the new functionality is.

I believe that desiring both ValidatedForm's validation and native HTML at the same time is highly unlikely. Each browser have a different implementation of how error messages are displayed, thus besides duplicating validation messages, the native UI would likely overlap with remix-validated-form's errors and create a poor and confusing UX. Thus, I'd suggest making the disabling of native validation with <form novalidate when <ValidatedForm is hydrated an implementation detail.

This would free up noValidate to fill the standard behavior of disabling ALL validation (JS and native HTML). And allow formNoValidate to be supported with the same standard behavior.

This however would represent a breaking change. Moreover, this could inadvertently end up breaking client-side validations if developers don't update their code where <ValidatedForm noValidate={useHydrated()} was used; as it was recommended. To help mitigate this, an idea could be to do a pre-minor release to implement a console.warn('⚠️ DEPRECATED usage of "noValidate" potentially detected, for more info please check http://...') which would be raised whenever a change of props.noValidate is detected during the hydration.

Why should this feature be included?

novalidate and formnovalidate are web standard attributes that are expected to produce an established form behavior.

I was set aback when I migrated <form or Remix's <Form to <ValidatedForm because formNoValidate was not respected/supported.

I believe that supporting both noValidate and noFormValidate with the standard behavior would make things more straightforward and simpler for developers. Otherwise, we have to rely on a workaround to intercept a click on a submitter to perform the submission without validation (e.g with useFetcher).

Check https://codesandbox.io/s/rvf-y7jl8e?file=/app/routes/index.tsx for some code demoing an attempt to use formNoValidate in a <ValidatedForm.

@airjp73
Copy link
Owner

airjp73 commented Nov 12, 2022

I'm feeling better about this direction the more I think about it. Thanks for opening up the discussion 👍

A couple thoughts / questions to figure out.


What if, instead changing the behavior of noValidate, we remove the prop entirely? Is there any reason someone would want to set noValidate directly on the ValidatedForm? If they did, would that disable all validation or only submit validation?

We could still fully support formNoValidate, and anyone using noValidate could upgrade and delete code at their leisure (the best kind of breaking change). The downside here is it might make people think formNoValidate wouldn't work.


What should we do about the onSubmit prop? Currently, the first argument is the form data after it's validated, transformed, and correctly typed. If a user submits a form with formNoValidate, how do we communicate that to the onSubmit callback?

It seems bad not to call the submit callback, but we also can't really provide anything better than the raw submit event in that case.

Trying to think of ways we can make that work without requiring normal usage to require if (event.validated) or something.

@airjp73
Copy link
Owner

airjp73 commented Nov 12, 2022

Another thought:

Changing the function signature of onSubmit in a breaking way probably isn't that bad since using that already isn't the most common case.

Just a sketch of an idea:

onSubmit={async (event) => {
  // identical to the normal event
  // but enhanced with more properties

  // data is a promise that rejects if `noValidate`
  // That way we can keep the strict types
  await event.data;

  // false when noValidate.
  // in case you want to handle both cases
  event.validated;

  // Since we're adding stuff, this would be convenient.
  // Also includes the data from the clicked button, which is normally an extra step
  event.formData;
}}

So migrating would look like this

- onSubmit={(data, event) => {
+ onSubmit={async (event) => {
+   const data = await event.data;

It guess it doesn't gain that much over just requiring if (!validated) return; or something. But it's also closer to the native API which is cool.

@nrako
Copy link
Contributor Author

nrako commented Nov 12, 2022

What if, instead changing the behavior of noValidate, we remove the prop entirely? Is there any reason someone would want to set noValidate directly on the ValidatedForm? If they did, would that disable all validation or only submit validation?

That's a good question. I personally never have encountered a use-case for noValidate. However I could imagine using it to debug something during development, for example, to momentarily get rid of all validations to quickly test an ActionFunction. Maybe also for a rushed hot-fix; if somehow the validation create problems for users in production – it could be argued that a hotfix could use Remix's <Form> instead of <ValidatedForm> but that might also means addressing chidren such as inputs who need the remix-validated-form context and are using hooks such as useField, etc. Passing a permissive "empty" validator is probably an option too.

I do think those are edge-cases though and this isn't possible with the current implementation of noValidate anyway. So it's not something I'd prioritize moreover considering your next point:

... still fully support formNoValidate, and anyone using noValidate could upgrade and delete code at their leisure...

...it might make people think formNoValidate wouldn't work.

I think dropping noValidate– at least for a while with a deprecated code path to warn devs using non-typed languages (JS) – could represent a good migration option. I'd be tempted to believe people wouldn't really think that formNoValidate doesn't work because of it. Everyone probably used <ValidatedForm noValidate the way it was recommended, and as I did, most devs would probably expect that formNoValidatewhich get set on "their" HTML, which is fully in their control, get handled adequately on validation and submission by <ValidatedForm>.

I didn't thought about onSubmitI think your idea for the signature is very good 👍🏽, close to the native API with a clear async/await requirement for getting validated data.

@nrako
Copy link
Contributor Author

nrako commented Nov 12, 2022

Just an idea for onSubmit, instead of a promise event.data that raise and event.validated which I think would need to be a promise too (?), maybe something closer to the Validator => ValidationResult API could work?

onSubmit={async (event) => {
  const { errors, data, submittedData } = await event.validate();
  // or
  const { error, data, submittedData } = await event.validateField('name')
}
- type ValidateFieldResult = { error?: string };
+ type ValidateFieldResult<DataFieldType> = { error?: string, data?: DataFieldType };

And I think event.formData wouldn't be needed – which somehow, to me, almost feels like a native property of SubmitEvent.

@nrako
Copy link
Contributor Author

nrako commented Nov 12, 2022

Looking more closely at ValidatedForm's handleSubmit implementation, I wonder if it should be more similar to Remix's Form by calling the props.onSubmit before it's own internal logic.

Just a thought, but maybe leaving the option for devs to use the Validator API without setting the errors and focus a field might be useful to either silently event.preventDefault() or operate changes on the form's before letting ValidatedForm take over.

Food for thoughts:

onSubmit={async (event) => {
  const { errors, data, submittedData } = await event.validate();
  if (errors) return // void/truthy to let remix-validated-form take over, to show the errors etc.

  if (data.is_valid_but_in_some_intricate_condition) { // which might include third party data
    // do something in my user's space
    e.preventDefault() // do not submit,
   return // void/truthy to let remix-validated-form do its magic
  }

  if (data.is_valid_but_in_some_other_intricate_condition) { // which might include third party data
    // do something in my user's space
    // return false to indicate that remix-validated-form internal validation should be skipped 
    // no setFieldErrors and no focus first invalid field etc. and no submit should occur
    return false 
  }
}}

@nrako
Copy link
Contributor Author

nrako commented Nov 12, 2022

On second thought, I'm not sure about my latest comments, native validation does occur before onSubmit is called, and won't get called if the native validation doesn't pass.

@airjp73
Copy link
Owner

airjp73 commented Nov 12, 2022

I think dropping noValidate– at least for a while with a deprecated code path to warn devs using non-typed languages (JS) – could represent a good migration option.

Yeah, I'm definitely leaning this way. We could add noValidate back at some point in the future, but completely remove it for now.

And I think event.formData wouldn't be needed – which somehow, to me, almost feels like a native property of SubmitEvent.

Unfortunately, I don't think the native submit event includes a formData property. Internally, we have to do an extra step to get the form data + the value on the clicked button. People might not use it, but it could still be nice to add.

I didn't thought about onSubmitI think your idea for the signature is very good 👍🏽, close to the native API with a clear async/await requirement for getting validated data.

Yeah, right now the only disadvantage I can think of is the churn aspect. I want to think on it for a couple more days to see if anything comes to me.

@nrako
Copy link
Contributor Author

nrako commented Nov 15, 2022

I completely strayed away on my previous comments on this onSubmit topic – I jumped on that too fast, my bad. 🙈

Coming back to your proposal, after giving this a couple of days of thinking, I wonder in which instance I'd need to have a custom event with data, validated and formData properties instead of a standard event: FormEvent<HTMLFormElement>.

I wonder wether validated and parsed/typed data: DataType should ever be expected in onSubmit's arguments since the submit event can be triggered without any form validation if noValidate or noFormValidate are used – or if submitted programmatically.

I understand your intent of adding those properties to make it easier to handle those different cases but I believe this might induce wrong expectations and inappropriate usages of onSubmit. Especially if we consider Remix's principles which are aimed on correcting this over-used past trend of doing client-side-only XHR form submission to resources API – an SPA anti-pattern for web standard and still rampant in React's world.

Thus, I wanted to find a use-case where data was used in ways which justifies it. I personally haven't came to such case so I thought I'd try to search for use-cases on GitHub:

https://github.com/search?p=1&q=ValidatedForm+onSubmit+remix-validated-form&type=Code

That's certainly not exhaustive and full-proof but after going through the 22 results that search returned to me; the only case which seemed to use data in a remarkable way is the following one:

https://github.com/vedantroy/discuss/blob/a0dd06e24b74ec6c83d2675bafbc296715f16b5e/app/routes/__header/c/create.tsx#L106-L117

In this instance, it appears to be for adding the "files" dropped in a react-file-drop component to a FormData before submitting the form's data programatically. That's certainly a defendable use-case. In this instance I think this should also be doable by appending the files to a new DataTransfer and set inputTypeFileInForm.files with it though. I also believe those "file drop component" always have an underlying input[type=file] target, thus in principle, no onSubmit handling should be required at all, but that's not really the point. Overall, I don't think this code would be more cumbersome or less solid to achieve without the validated/typed data – especially if an event.validated code-path would need to be taken care off.

However, I do think it should be possible to programmatically check if the form's content has been validated in onSubmit – and elsewhere. This is also what native API such as checkValidity() and ValidityState are for.

And with remix-validate-form's Validation helpers, that's actually quite straightforward to do at any time:

const { result, data } = await validator.validate(new FormData(formRefOrFormEventTarget))

TL;DR – I think <ValidatedForm>'s props.onSubmit signature should simply be identical to React's onSubmit, just as Remix's <Form>. I don't think it should be the role of onSubmit to be concerned with the data, validated or not.

@airjp73
Copy link
Owner

airjp73 commented Nov 16, 2022

I think being able to access the validated data in onSubmit is a feature I won't give up. It's kinda funny -- one of the first results in the search I found was my own xD (here). The use-case here is that I'm sending a chat message over a websocket, so the normal submit behavior doesn't cut it.

I also think submitting an AJAX request instead of using the Remix / native submit behavior is a perfectly valid case. I totally found an example in those search results of someone doing that when they didn't need to though, so it may be worth documenting that this isn't the primary way to do things.

Ultimately, even after a few days, I still think switching to a more native-ish api makes sense. Though I do want to augment the event with custom properties (we already do that for preventDefault so it's not a big lift).

One last thing to bikeshed for me would be, which one of these two should we go with?

const data = await event.data;

// or

const data = await event.getData();

@airjp73
Copy link
Owner

airjp73 commented Nov 16, 2022

Something I just thought of.

Before we set noValidate on all ValidatedForms, it would be good to look into how this interacts with setCustomValidity. Taking validation errors from this library and displaying them through the native validation api is something I've thought about experimenting with in the past.

@nrako
Copy link
Contributor Author

nrako commented Nov 16, 2022

one of the first results in the search I found was my own xD (here). The use-case here is that I'm sending a chat message over a websocket, so the normal submit behavior doesn't cut it.

I unfortunately don't have access to this repository, it must be private. But I can guess how such use-case could simply be solved with something akin to:

onSubmit={e => {
  e.preventDefault()
      
  // assuming consumer should always do it's own validation anyway, data can be send raw
  sendMessage({ message: refInput.current.value })
  // or still raw
  sendMessage(Object.fromEntries(new FormData(e.currentTarget))
     
  // or if for some reason some data parsing is required client-side (consumer should never trust 
  // client to send valid data but the client might still need to transform data for it to be accepted 
  // by the consumer)
  const { data } = await validator.validate(new FormData(e.currentTarget)) // with async on function
  // perhaps the form was submitted with "novalidate" and the data isn't valid 
  // and can't be parsed as expected
  if (!data) return console.log('oups')

  sendMessage({ message: data.message })
}

validated data in onSubmit is a feature I won't give up

What I was trying to point out in my previous comment is that there's also many examples where ValidatedForm onSubmit is not so well used... and a particular API can easily induce certain non-educated usage while web standards and native APIs already does a great job at defining interfaces which devs would gain to understand better rather than being possibly wrongly led by library's API abstraction which muddy the boundaries and I think usually don't age well.

Before we set noValidate on all ValidatedForms, it would be good to look into how this interacts with setCustomValidity.

setCustomValidity() could indeed be used to make the native checkValidity() and related API useful even when novalidate is used. Here's a quick codepen to test that: https://codepen.io/nrako/pen/eYKGWOv?editors=1111

I think leveraging custom validation would be a great approach to make it possible to check a form or input validity at any time, that would leverage native API and makes remix-validated-form more resilient. And this is all well supported accross browsers.

Taking validation errors from this library and displaying them through the native validation api is something I've thought about experimenting with in the past.

This however, I'm not sure. I believe the native UI doesn't provide the kind of UX and controls that devs using this library might be looking for. As an example I use z.refine to set a custom error message on a group of dot-nested-named inputs by setting the path on the "parent name". Then, I have an <InputGroup> component which display the error. I wouldn't be able to achieve this with only the native API (as of today, but that's a topic here whatwg/html#6870 (comment)). Although, there's probably also merits on having native UI with a validator (e.g zod) doing the heavy work of setCustomValidity(msg) in certain scenarios, and a solution allowing a mix of native UI + usage of const { error } = useField(name) could still be possible.

@airjp73
Copy link
Owner

airjp73 commented Nov 16, 2022

I unfortunately don't have access to this repository, it must be private

Whoops! Yeah, I was wondering why that showed up (I guess that makes sense now).

But I can guess how such use-case could simply be solved with something akin to:

Getting the correct, validated form data isn't something I think we should push onto users. It's more than simply doing new FormData(form). This is how we handle it. Plus you have to add a check for result.error even though you already know the form is valid, the vast majority of the time.

and a particular API can easily induce certain non-educated usage

I definitely agree with this. I think anything we go with from here will be less obvious than providing the data directly as a parameter. On top of having to call preventDefault, that should give enough of a signal to a user that this isn't the normal way of doing things.

onSubmit={async (event) => {
  event.preventDefault();
  const data = await event.data;

  // Do something else
}}

I'm open to bikeshedding the exact api. These helpers don't necessarily need to be accessed off the event -- I just thought it was a neat idea. One aspect of this api I'd like to stick around for sure is the ability to access the data without a conditional and throwing an error when the data is missing.

@nrako
Copy link
Contributor Author

nrako commented Nov 17, 2022

It's more than simply doing new FormData(form). This is how we handle it.

That's right currently for doing the submission of the form, behind the scene both <ValidatedForm> and Remix's <Form> add the submitter name/value to the formData:

https://github.com/airjp73/remix-validated-form/blob/53b09d4d2b63bc38a2283f8ff63d494fc816e83c/packages/remix-validated-form/src/ValidatedForm.tsx#L309-L315

And on Remix's useSubmitImpl:
https://github.com/remix-run/remix/blob/93d33107c7c0409658cfacc7931f4d6e3a5518e9/packages/remix-react/components.tsx#L1173-L1175

Remix's useSubmit() => submit does this too and both that hook and Remix's <Form> use useSubmitImpl().

💡BTW: <ValidatedForm ...handleSubmit shouldn't need to add the submitter's name/value anymore since submit(submitter does it now :
https://github.com/airjp73/remix-validated-form/blob/53b09d4d2b63bc38a2283f8ff63d494fc816e83c/packages/remix-validated-form/src/ValidatedForm.tsx#L342

To go back on my argument; this particular handling is required for sending the form like a native <form>, to mimic a real form but with the extra goodies that Remix's <Form> does:

ˮSince Remix <Form> works identically to <form> (with a couple extra goodies for optimistic UI etc.), we're going to brush up on plain ol' HTML forms, so you can learn both HTML and Remix at the same time.ˮ
– src: https://remix.run/docs/en/v1/guides/data-writes#plain-html-forms

Remix's <Form onSubmit signature is the same than React's <form onSubmit (which is native-ish with synthetic event). And if a user starts to use onSubmit, it's better that devs understand how native form's submit event work rather than having some API abstraction which break onSubmit interoperability with <form>, Remix's <Form etc.

plus you have to add a check for result.error even though you already know the form is valid, the vast majority of the time.

The form might not be valid if it's submitted with some novalidate or noformvalidate. And this is where one might want to be able to rely on form.checkValidity() and other Validation API on the FormElements. This might imply using setCustomValidation() as briefly discussed before.

One aspect of this api I'd like to stick around for sure is the ability to access the data without a conditional and throwing an error when the data is missing.

What is making you so sure? I fail to understand the motivation or concrete use-case which can justify that despite all the exchanges we had. The only use-case I could see is to retrieve parsed data but there's way to do it as discussed before; but there's also caveats that any dev using onSubmit need to understand, especially in combination to novalidate/noformvalidate – as I tried to explain in previous exchanges.


Note: There's an open issue whatwg/xhr#262 which discuss a proposal that would include something such as new FormData(form, submitter). There's also an interesting exchanges and plenty informations about all this topic in that thread whatwg/html#3195 (comment)

@airjp73
Copy link
Owner

airjp73 commented Aug 8, 2024

RVF v6 has been released 🎉

There is some explicit handling for noValidate and formNoValidate in v6.

You can find the documentation here and the migration guide here.

@IanWitham
Copy link

I can't find any documentation specific to this. Adding formNoValidate to a submit button seems to do nothing. I need a way for users to save their progress, although required fields may be missing. You say "explicit handling" has been added in v6, but I have no idea what that means, sorry.

@airjp73
Copy link
Owner

airjp73 commented Sep 22, 2024

Ah, that looks like an oversight. formNoValidate is getting passed into the internal submit handler but I'm not doing anything with it 🤦. That's a bug -- I can fix.

@IanWitham
Copy link

Thanks so much. Your efforts are appreciated!

@airjp73
Copy link
Owner

airjp73 commented Nov 12, 2024

I wasn't able to find a way I was happy with that handled formNoValidate automatically, but it's possible to handle now with a new helper function inside of onBeforeSubmit. Something like this should do the trick:

onBeforeSubmit: async (api) => {
  if (api.sumitterOptions.formNoValidate) await api.performSubmit(api.unvalidatedData);
}

This is an advanced / uncommon enough use-case that I'm satisfied with a slightly manual solution like this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants