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

[Form lib] Allow dynamic data to be passed to validator functions #109238

Merged
merged 5 commits into from
Aug 24, 2021

Conversation

sebelga
Copy link
Contributor

@sebelga sebelga commented Aug 19, 2021

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.

// Form schema
const schema = {
  name: {
    validations: [{
      validator: ({ customData: { value } }) => {
        // value === [1, 2 ,3] as passed below
      }
    }]
  }
};

// Component JSX
<UseField path="name" validationData={[1, 2, 3]} />

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.

// form.schema.ts
const schema = {
  indexName: {
    validations: [{
      validator: async ({ value, customData: { provider } }) => {
        // Whenever a new value is sent to the `validationData$` Observable
        // the Promise will resolve with that value
        const indices = await provider();

        if (!indices.include(value)) {
          return {
            message: `This index does not match any of your indices`
          }
        }
      }
    }]
  }
}

// myform.tsx
const MyForm = () => {
  const { form } = useForm();
  const [{ indexName }] = useFormData({ watch: 'indexName' });
  const [indices, setIndices] = useState([]);
  const [indices$, nextIndices] = useAsyncValidationData(); // Use the provided hook to create the Observable

  const fetchIndices = useCallback(async () => {
    const result = await httpClient.get(`/api/search/${indexName}`);
    setIndices(result);
  
    // Here we send the indices to the validator "provider()" which can now resolve
    nextIndices(result);
  }, [indexName]);

  // Whenever the indexName changes we fetch the indices
  useEffet(() => {
    fetchIndices();
  }, [fetchIndices]);

  return (
    <>
      <Form form={form}>
        /* Pass the Observable to your field */
        <UseField path="indexName" validationData$={indices$} />
      </Form>

      /* Display the list of indices that match the index name entered */
      <ul>
        {indices.map((index, i) => <li key={i}>{index}</li>)}
      </ul>
    <>
  );
}

@sebelga sebelga force-pushed the form-lib/dynamic-validation-data branch from 599df15 to 1e0d339 Compare August 20, 2021 16:59
@sebelga sebelga marked this pull request as ready for review August 20, 2021 17:02
@sebelga sebelga requested review from a team as code owners August 20, 2021 17:02
@sebelga sebelga requested review from yuliacech and machadoum August 20, 2021 17:02
@sebelga sebelga added Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.16.0 v8.0.0 labels Aug 20, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-stack-management (Team:Stack Management)

@sebelga
Copy link
Contributor Author

sebelga commented Aug 23, 2021

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
esUiShared 184 185 +1

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
esUiShared 197.4KB 199.7KB +2.3KB

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@sebelga sebelga added the release_note:skip Skip the PR/issue when compiling release notes label Aug 23, 2021
@@ -193,6 +193,11 @@ export interface ValidationFuncArg<I extends FormData, V = unknown> {
};
formData: I;
errors: readonly ValidationError[];
customData: {
Copy link
Contributor

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.

Copy link
Contributor Author

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.

@yuliacech
Copy link
Contributor

Hi @sebelga, great job adding an async custom data to the form validation!
If I understand this correctly, there is not yet an place in Kibana where this new validation can be tested locally, right?
I was able to follow the code, docs additions and tests, though, and it looks good to me. Before I approve, I'd like to understand why we want to use an Observable for custom async data. It looks to me like we only use the first output and transform it to a Promise internally anyways. Would it make sense to let the form consumers provide a Promise instead and resolve it when they have the data?
Btw, looking forward to use this new validation in ILM! That would be a great use case for ILM policy name validation.

@sebelga
Copy link
Contributor Author

sebelga commented Aug 24, 2021

Thanks for the review @yuliacech !! 👍

there is not yet an place in Kibana where this new validation can be tested locally, right?

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

Would it make sense to let the form consumers provide a Promise instead and resolve it when they have the data?

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 nextValue('newValue') to resolve the promise. Can you show me with an example?

looking forward to use this new validation in ILM! That would be a great use case for ILM policy name validation

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).

Copy link
Member

@machadoum machadoum left a 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

Copy link
Contributor

@yuliacech yuliacech left a 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.

@sebelga sebelga merged commit 3534450 into elastic:master Aug 24, 2021
@sebelga sebelga deleted the form-lib/dynamic-validation-data branch August 24, 2021 12:56
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 109238 or prevent reminders by adding the backport:skip label.

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Aug 26, 2021
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Aug 26, 2021
sebelga added a commit to sebelga/kibana that referenced this pull request Oct 21, 2021
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.16.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants