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

Allow for debouncing live validation (liveValidate) #3616

Open
1 task done
michal-kurz opened this issue Apr 19, 2023 · 5 comments
Open
1 task done

Allow for debouncing live validation (liveValidate) #3616

michal-kurz opened this issue Apr 19, 2023 · 5 comments
Labels
core feature Is a feature request help wanted

Comments

@michal-kurz
Copy link
Contributor

michal-kurz commented Apr 19, 2023

Prerequisites

What theme are you using?

other

Is your feature request related to a problem? Please describe.

The liveValidate prop performs quite well for one-off state changes in our project when using @rjsf/validator-ajv6 (although not with @rjsf/validator-ajv8 for some reason), but it lags when typing on slow machines.

Describe the solution you'd like

This would seemingly be solved by giving implementator a choice to debounce the validation (so it would only initiate live validation once the user has not typed anything for X milliseconds).

Describe alternatives you've considered

I tried achieving the same effect by turning off liveValidation and calling a debounced instance of Form.validateForm() inside onChange, but it lead to very confusing results for me:

  1. it appears that liveValidation actually functions slightly differently from Form.validateForm() in what/how it validates - as if liveValidation only validated inputs that were actually being rendered, while Form.validateForm() always validating all of formData against all of schema - or something in that ballpark. I was getting very different results between these two, and Form.validateForm() turned out to be terribly incompatible with our legacy code.
  2. Form.validateForm() triggers onError, while liveValidation doesn't. This is a big issue for us, because we use custom logic for un-collapsing collapsed form sections (custom widgets) and scrolling to invalid fields onError. When using Form.validateForm() onChange to simulate live validation, this makes our form scroll all over the place all the time. De-coupling Form.validateForm() and onError (maybe allowing for Form.validateForm({ triggerErrorHandler: false })) might help our use-case greatly.

It also might be possible to manually achieve this by wrapping @rjsf/validator-ajv6 into a custom validator and using debounce somewhere in the way - but I have no idea how to approach this. Do you think that would be possible?

Thank you for your consideration 🙏

@michal-kurz michal-kurz added feature Is a feature request needs triage Initial label given, to be assigned correct labels and assigned labels Apr 19, 2023
@heath-freenome heath-freenome changed the title Allow for debounding live validation (liveValidate) Allow for debouncing live validation (liveValidate) Apr 21, 2023
@heath-freenome
Copy link
Member

@michal-kurz If you look at the implementation of how liveValidate works, you can see that it calls the validate() function on the form with just the changed data (as you surmised). I agree that trying to debounce things yourself would lose the state updating happens in the live validate code in the onChange() handler in Form. This sounds like a nice feature for someone (like you?) perhaps to implement. We maintainers probably have a few hours a week to work on issues like this and we LOVE to get help from you all using the library. I think that making the liveValidate flag also take an additional object value containing the debounceThreshold would be the simplest solution. (i.e. liveValidate?: boolean | { debounceThreshold: number };)

@heath-freenome heath-freenome added help wanted core and removed needs triage Initial label given, to be assigned correct labels and assigned labels Apr 21, 2023
@michal-kurz
Copy link
Contributor Author

@heath-freenome Thank you very much for you feedback and pointing me to the right direction. I have a lot of work and personal stuff to do right not, but I will get back to it in about two weeks and try to implement this. I made myself a ticket so I don't forget :)

@michal-kurz
Copy link
Contributor Author

I just want to affirm that I'm still planning to do this! I will get to it during next workweek :)

@michal-kurz
Copy link
Contributor Author

michal-kurz commented May 23, 2023

@heath-freenome I apologize for the delay, I was able to get to it much later than I expected.

I made a proof of concept for the invalidation debouncing, and it can be found here: https://github.com/michal-kurz/react-jsonschema-form/pull/1/files#

It's in no way polished, and in its current state breaks some other functionality, but I wanted to consult it with you to assess whether it's worth continuing on this route. Would you please look at it? It this roughly acceptable as a concept, or did you imagine something else?

Here are my takeaways:

  1. In the current main branch and Playground package, when liveValidation is enabled, this.validate() triggers three times on every keystroke
    1. this.onChange() -> this.getStateFromProps() - here, the validation result doesn't even get used, and gets later overriden - it just seems to be called as a byproduct of other invokers of this.getStateFromProps() using the validation result elsewhere
    2. inside this.onChange() body, when this.validate() gets called directly with result of this.getStateFromProps()
    3. in componentWillReceiveProps() afterwards - I suppose this could be solved from the outside by memoizing the instance, but doing that reliably comes with a whole another set of challenges when working with large and complex forms.
  2. Since my main point is improving the performance of the form, I definitely want to run validation zero times on every keystroke, when in debounced mode (and only eventually once debounce kicks in) - only debouncing the main trigger while leaving the other two in would be missing the point.
  3. I assume that the rest of the form's logic (outside of live validation) should remain unchanged, and and only live validation should be debounced. This way, error states (errors, errorSchema, validationErrors, validationErrorSchema) are no longer always consistent with the rest of the form, when liveValidate is enabled, but only eventually consistent - even when no debounceThreshold is provided. This seems OK, since a. this is how form works outside of liveValidate and b. this kindof seems to be the point of this change - but I wanted to check with you to make sure.

@michal-kurz
Copy link
Contributor Author

michal-kurz commented Jun 16, 2023

How do we go on with this, @heath-freenome ?

I initially had plans with this - I was hoping for massive performance gains, looking to potentially delegate debounced (async) liveValidation onto a separate worker to free up the main thread. But I've since uncovered that the performance bottlenecks with our use-cases lie elsewhere. Do you think it's worth-while to keep pursuing this? Maybe you have some knowledge suggesting that this would actually massively help with some very common use-cases - otherwise I'd say this is probably not worth it.

There's one thing I haven't yet tested and included in my consideration though: We are currently running on @rjsf/validator-ajv6, because ajv-8 gives us terrible performance. Do you know why this could be? If this difference is caused by liveValidate calls, this may be such "very common use-case" I described above :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core feature Is a feature request help wanted
Projects
None yet
Development

No branches or pull requests

2 participants