-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Form lib] Allow dynamic data to be passed to validator functions #109238
[Form lib] Allow dynamic data to be passed to validator functions #109238
Conversation
599df15
to
1e0d339
Compare
Pinging @elastic/kibana-stack-management (Team:Stack Management) |
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]Module Count
Page load bundle
History
To update your PR or re-run it, just comment with: |
@@ -193,6 +193,11 @@ export interface ValidationFuncArg<I extends FormData, V = unknown> { | |||
}; | |||
formData: I; | |||
errors: readonly ValidationError[]; | |||
customData: { |
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.
Is customData
a required property of ValidationFuncArg
? To me it seems like it should be an optional property, as not all forms need a validation with customData
.
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.
Yes the customData
object will always be there in the validator. That does not mean that the consumer needs to pass custom data 😊 If he doesn't, its value
will simply be undefined
.
Hi @sebelga, great job adding an async custom data to the form validation! |
Thanks for the review @yuliacech !! 👍
That's correct. I didn't want to add more to this PR, but once it is merged we'll be able to update the index pattern flyout code (the is currently a hack done #109500) and this it will unblock #109233 where it will be used
I am not sure I understand how that would work and if the API would be any better than with an Observable (where the consumer simply needs to call
Can't you do the validation today with an asynchronous validator? Do you also need the async request somewhere else in the UI (which is what this PR is about). |
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.
Security solution code LGTM
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.
Hi @sebelga, as discussed in zoom, happy to approve this PR 👍
Thanks a lot for explaining the difference between an already existing async validator and this new async validation data passed to the validator function. We indeed don't need that in ILM.
Friendly reminder: Looks like this PR hasn’t been backported yet. |
In elastic#109238 I've added the possibility to provide dynamic data to field validators. When working on this current PR I realised that I didn't take into account the fact that we need to be able to validate the field without first triggering a change on the field. My initial proposal to expose an Observable (https://github.com/elastic/kibana/pull/109238/files#diff-ef2d4424a31a8d8def4c1c2d9d756cfef9cc58b30837c1b6639e3419e572cdd9R35) did not take that into account. I reverted that change and decided to allow a Promise to be provided and let the consumer in control of resolving the promise for the validation to resolve.
This PR adds support for dynamic data to be sent to the validators function in the form lib.
Problem
Scenario 1
We often (here, here, and many other places) need dynamic data to be able to validate our form fields. The form lib did not allow to provide (until now) dynamic data to the validators, forcing us to create the validator inside our component, which defeats the purpose of having a form schema.
Scenario 2
@mattkime in his index pattern create modal PR faced a challenging situation where the validation required dynamic data only available asynchronously after the field had changed (and its validation had ran). In order to solve this problem he had to create an indirection, updating a parent component from inside a validator. This is far from ideal and will probably give us some headaches in the future to understand the data flow.
As I bumped into the exact same problem when working on the index pattern field editor error handling I decided to update the form lib to handle asynchronous data to be passed to validators.
Solution
Scenario 1
We can pass dynamic data to the validator with a new
validationData
prop.Scenario 2
We can provide an observable to a new
validationData$
prop. The validator has a provider() that will only resolve when a new value is sent to the observable.