[Form lib] Allow dynamic data to be passed to validator functions#109238
[Form lib] Allow dynamic data to be passed to validator functions#109238sebelga merged 5 commits intoelastic:masterfrom
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: |
| }; | ||
| formData: I; | ||
| errors: readonly ValidationError[]; | ||
| customData: { |
There was a problem hiding this comment.
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.
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). |
machadoum
left a comment
There was a problem hiding this comment.
Security solution code LGTM
yuliacech
left a comment
There was a problem hiding this comment.
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
validationDataprop.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.