Hook form lib #41658

sebelga · 2019-07-22T12:35:28Z

Form library with hooks!

Motivation

In the Elasticsearch UI team we build many forms. Many many forms! 😊 For each of them, we manually define the form state & update and validate its value. Basically re-inventing the wheel for each new form. It takes our precious dev time to re-think the approach each time, but even more problematic: it means that each of our form is built slightly differently. Maintaining those forms means that each time we need to remember how the state is updated and how validation works for a specific form. This is far from efficient...

We need a system in place that takes care of the repetitive task of managing a form state and validating its value, so we can dedicate more time doing what we love the most: build amazing UX for our community! 😊

Main building blocks

`useForm()` hook & `<UseField />`

The main building blocks of the library are the useForm() hook and the <UseField /> component. The former is used to intantiate a form, you call it once and then forward the form object returned to the <UseField /> component. As you probably have guessed, <UseField /> is the Component you will use to declare a field on the form.

Let's go through some example:

// myForm.ts
import { useForm, Form, UseField, FormConfig } from "<path-to-elasticsearch-ui-shared>/static/forms/hook_form_lib";

export const MyForm = () => {
  const submit: FormConfig['submit'] = (formData, isValid) => {
    if (isValid) {
      // send Http Request or anything needed with the form data...
    }
  };

  const { form } = useForm({ onSubmit });

  return (
    <Form form={form}>
        <UseField path="title" />
        <button onClick={form.onSubmit}>Send form</button>
    </Form>
  );
};

That is all we need to create a form with a "title" property and listen for the input changes.
By default, <UseField /> will render an <input type="text" /> to hold the value. Of course, this is just to illustrate how the two building blocks work, in other examples we will see how to customize the UI.

Design decisions

Raw form state

In order to allow the most flexibility and also improve performance, the form data is saved as a flat object. Only when submitting the form, the output object is built and returned to the consumer. This is the reason why a form field is given a path and not a name to identify it.

// following the above example

return (
  <Form form={form}>
      <UseField path="book.title" />
      <UseField path="book.author" />
      <UseField path="city[0]" />
      <UseField path="city[1]"  />
      <button onClick={form.onSubmit}>Send form</button>
  </Form>
);

// internal form state
const rawFormData = {
  'book.title': 'Leviathan',
  'book.author': 'Paul Auster',
  'city[0]': 'New York',
  'city[1]': 'London'
}

// output
const output = {
  book: {
    title: 'Leviathan ',
    author: 'Paul Auster'
  },
  city: ['New York', 'London']
}

For array values, we would probably have dynamic rows, but this was just to illustrate how paths work. As you can see, updating the output form data shape is straightforward. Also, say goodbye to serialize and unserialize payloads for Elasticsearch. 😊

Performance

A lot of effort has been put in optimizing for performance. In our current forms, we usually have a state object with the form values and errors. On each keystroke, we update the state and thus re-render the whole form. On small form this is OK, but on large forms with many inputs, this is not efficient.

This is the reason why each form Field is wrapped inside its own component, managing its internal state of value + errors. This also implies that you can't directly access the form data and react to changes on a field value. Of course, there is a solution, have a look at "Access the form data" below.

Customize the UI

There are two ways to provide a custom UI:

Approach 1: Render props pattern

return (
  <Form form={form}>
    <UseField path="title">
      {(field) => {
        // field is the field object generated, is has several properties and methods you can use
        // to work with the field, but the mains ones are:
        // - onChange() --> method
        // - value --> property
        // - errors --> property

        const isInvalid = field.errors.length > 0 && form.isSubmitted;
        const errorMessage = field.errors.length ? (field.errors[0].message as string) : null;

        return (
          <EuiFormRow
            label="Some label"
            helpText="Some help text"
            error={errorMessage}
            isInvalid={isInvalid}
            fullWidth
          >
            <EuiFieldText
              isInvalid={isInvalid}
              value={field.value as string}
              onChange={field.onChange}
              isLoading={field.isValidating}
              fullWidth
            />
          </EuiFormRow>
        )
      }}
    </UseField>
    <button onClick={form.submit}>Send form</button>
  </Form>
);

As you can see, you have complete control on how a field is rendered in the DOM. The hook form library does not bring any UI with it, it just returns form and field objects to help building forms.
For our concrete Elasticsearch UI forms, this is a lot of boilerplate to simply render a text field with some error message underneath it. In a following PR I have prepared some goodies, let's call them "syntactic sugar Component" that will remove all the repetitive JSX so we can much more easily spot what is unique to a field (name, label, helpText, validation).

Approach 2: Passing a Component

Another way to customize the UI is to pass a Component as a prop.

// title_field.tsx

export const TitleField = ({ field, any, additional, prop }) => {
  const isInvalid = field.errors.length > 0 && form.isSubmitted;
  const errorMessage = field.errors.length ? (field.errors[0].message as string) : null;

  return (
    <EuiFormRow
      label="Some label"
      helpText="Some help text"
      error={errorMessage}
      isInvalid={isInvalid}
      fullWidth
    >
      <EuiFieldText
        isInvalid={isInvalid}
        value={field.value as string}
        onChange={field.onChange}
        isLoading={field.isValidating}
        fullWidth
      />
    </EuiFormRow>
  );
}

import { TitleField } from './title_field';

export const MyForm = () => {
  ...
  
  return (
    <Form form={form}>
      <UseField
        path="title"
        component={TitleField}
        componentProps={{ any: 1, additional: 2, prop: 3 }}
      />
  
      <button onClick={form.submit}>Send form</button>
    </Form>
  );
};

This has the benefit of separation of concern. On one side we build the form, declaring its fields, on the other, we build the UI for each field.

Field configuration

<UseField /> accepts an optional config prop to provide field configuration. This is where we will define the default value, formatters, (de)Serializer, validation...

Note: All the configuration settings are optional.

import { TitleField } from './title_field';

...

return (
  <Form form={form}>
    <UseField
      path="title"
      component={TitleField}
      defaultValue="C*T"
      config={{
        defaultValue: 'NEW YORK',
        // Transform the value before saving it in the form state.
        // Can have an unlimited number of chained formatters.
        formatters: [(value: string) => value.toUpperCase()],
        // Called right before returning the output object (onSubmit).
        // Most useful with Arrays where we might want to map and return only part
        // of the array item objects.
        serializer: (value: string) => value.replace(/A/g, '*'),
        // Called when intantiating the field default value.
        // Again, it is mostly useful for Arrays.
        deserializer: (value: string) => value.replace(/\*/g, 'A'),
        // Validation functions, explained in the section below
        validations: [],
      }}
    />

    <button onClick={form.submit}>Send form</button>
  </Form>
);

The first thing you see is that we have declared 2 default values. The prop defaultValue takes over the config one. This is because the config object might be declared statically in a separate file with a default value to be used in create mode. (for example for a toggle, if it's true or false by default). Then, on the field, when you are in edit mode, you want to override any default value from the config and provide the value that comes from the API request.

This is great, but as our form grows, declaring configuration object inline would quickly clutter our form component. A better way to declare the fields configuration is through a form schema.

Form Schema

A form schema is simply an object mapping to fields configuration. It means: cutting the inline configuration from the JSX above and putting it inside an object.

// myForm.schema.ts
import { FormSchema } from "<path-to-elasticsearch-ui-shared>/static/forms/hook_form_lib";

export const schema: FormSchema = {
  title: {
    defaultValue: 'NEW YORK',
    // Transform the value before saving it in the form state.
    // Can have an unlimited number of chained formatters.
    formatters: [(value: string) => value.toUpperCase()],
    // Called right before returning the output object (onSubmit).
    // Most useful with Arrays where we might want to map and return only part
    // of the array item objects.
    serializer: (value: string) => value.replace(/A/g, '*'),
    // Called when intantiating the field default value.
    // Again, it is mostly useful for Arrays.
    deserializer: (value: string) => value.replace(/\*/g, 'A'),
    // Validation functions, explained in the section below
    validations: [],
  }
}

// myForm.ts
import { useForm, UseField, FormConfig } from "<path-to-elasticsearch-ui-shared>/static/forms/hook_form_lib";
import { TitleField } from './title_field';
import { schema } from './myForm.schema';

export const MyForm = () => {
  const submit: FormConfig['submit'] = (formData, isValid) => {
    if (isValid) {
      // send Http Request or anything needed with the form data...
    }
  };

  const { form } = useForm({ onSubmit, schema }); // provide the schema

  return (
    <Form form={form}>
      <UseField path="title" component={TitleField} />
      <button onClick={form.submit}>Send form</button>
    </Form>
  );
};

And we're back to a nice, easy to read, <MyForm /> component! As you might have guessed, the UseField path maps to the schema object properties.

The schema does not need to be flatten though

const schema = {
  book: {
    title: {
      // field config (path = "book.title")
    },
    author: {
      // field config (path = "book.author")
    }
  }
}

Validation

We finally get to it! 😊 You validate a field by providing a set of Validation objects to the validation array of the field configuration. These Validation objects have one required property: a validator() function. This validator function will receive the value being validated, along with the formData in case we need to validate the value against other form fields.

The validator() functions can be synchronous or asynchronous (see below), and they will be executed sequentially until one of them fails. Failing means returning an Error object. If nothing is returned from the validator it means that it succeeded.

const schema = {
  title: {
    ...,
    validation: [{
      validator: ({ value }) => {
        if ((value as string).startsWith('.')) {
          return {
            code: 'ERR_FORMAT_ERROR',
            message: `The title can't start with a dot (.)`,
          };
        }
      },
    }]
  }
}

As we can see, field validator function are pure functions (when doing synchronous validation) that receive a value and return an Error object or void. As we know, a lot of the validation we do in our forms check for the same type of error (is the field empty?, does the string start with "x"?, is this a valid index pattern?, is this a valid time value?...). Those field validator functions are a good candidate for other pieces of reusable static code. In my separate "goodies" PR I will include some of them. The idea is that we grow our list of reusable field validators functions and only add business-specific validators inside the form schema whenever it does not make sense to make reusable.

// <path-to-reusable-form-field-validators>/starts_with_char.ts

export const startsWithCharField = (char: string) => ({ value }) => {
  if (value.startsWith(char)) => {
    return {
      code: 'ERR_FORMAT_ERROR',
      message: `The field value can't start with "${char}"`,
      char, // return any useful property to the error
    };
  }
}

import { startsWithCharField } from '<path-to-reusable-form-field-validators>/starts_with_char';

const schema = {
  title: {
    ...,
    validation: [{
      validator: startsWithCharField('.')
    }]
  }
}

Asynchronous validation

In some cases, we need to make an HTTP Request to validate a field value (for example to validate an index name). The good news is... asynchronous validation works exactly the same way as synchronous validation! You can mix both, the only thing to bear in mind is if one of the validation is asynchronous, calling field.validate() will need to be "awaited".

const schema = {
  title: {
    ...,
    validation: [{
      validator: async ({ value }) => {
        const response = await httpService.post('/some-end-point', { value })
        if (response.KO) {
          return {
            code: 'ERR_INVALID_INDEX_NAME',
            message: 'Sorry the name is not valid',
          }
        }
      }
    }]
  }
}

Default value object

We have seen earlier how we can provide a default value on the <UseField /> component through props:

<UseField path="title" defaultValue={myFetchedResource.title} />

If we only have a few fields, this is good enough. But if we have a complex form data object, this will quickly become tedious as we might have to drill down the myFetchedResource object to child components of our <MyForm /> main component.

To solve this, when initiating the form we can provide an optional defaultValue object. When a field is added on the form, if there is a value on this defaultValue object at the field path, it will be used as the default value.

export const MyForm = () => {
  ...

  const myFetchedResource = {
    title: 'Hello'
  }

  const { form } = useForm({ onSubmit, defaultValue: myFetchedResource });

  return (
    <Form form={form}>
      <UseField path="title" />
      <button onClick={form.submit}>Send form</button>
    </Form>
  );
};

Adding the "edit" capability to a form has never been so easy 😊

Dynamic form row with `<UseArray />`

In case your form requires dynamic fields, the <UseArray /> component will help you add and remove them.

export const MyForm = () => {

  ...

  return (
    <Form form={form}>
      <div>
        <UseArray path="users" >
          {({ items, addItem, removeItem }) => (
            <React.Fragment>
              <div>
                {items.map(item => (
                  <div key={row.id}>
                    <UseField path={item.path + '.firsName'} />
                    <UseField path={item.path + '.lastName'} />
                    <button type="button" onClick={() => removeItem(row.id)}>
                      Remove
                    </button>
                  </div>
                ))}
              </div>
              <button type="button" onClick={addItem}>
                Add row
              </button>
            </React.Fragment>
          )}
        </UseArray>
      </div>
      <button onClick={form.submit}>Send form</button>
    </Form>
  );
};

Access the form data with `<FormDataProvider />`

As mentioned above, for performance reason, you can't access the form data on the form object and get updates in your view when a field value change. That does not mean it is not possible to do. Whenever you need to react to a field value change, you can use the <FormDataProvider /> component.

return (
  <Form form={form}>
    <div>
      <UseField path="title" />

      <FormDataProvider pathsToWatch="title">
        {formData => <div>Name: {formData.title as string}</div>}
      </FormDataProvider>
    </div>
    <button>Send form</button>
  </Form>
);

The pathsToWatch is optional. It can be a path or an Array of paths. It allows you to declare which form field(s) you are interested in. If you don't specify the pathsToWatch, it will update on any field value change. For performance, you should always provide a value for it.

cc @cjcenizal

elasticmachine · 2019-07-22T12:36:10Z

Pinging @elastic/es-ui

elasticmachine · 2019-07-22T13:34:19Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

jloleysens · 2019-07-23T11:51:49Z

src/plugins/elasticsearch_ui_shared/static/forms/hook_form_lib/lib/subject.ts

+  subscribe(fn: Listener<T>): Subscription {
+    this.callbacks.add(fn);
+
+    fn(this.value);


Might be worth popping this inside a

setTimeout(() => { fn(this.value) }, 0);

so that the JS scheduler runs it async and any logic right after subscription (like setup logic) doesn't run after the sub listener.

Great point! Thanks for the catch 👍

elasticmachine · 2019-07-23T13:43:24Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-07-23T16:52:44Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

cjcenizal · 2019-07-24T00:04:13Z

👏 This is great work Seb! I haven't looked at the code yet, just the PR description. Thanks for taking the time to explain your reasoning so thoroughly and providing examples. Overall I think this makes a ton of sense and will provide enormous value.

By default, UseField will render an input type="text"

When do we expect consumers to use this default? Do we need it?

say goodbye to serialize and unserialize payloads for Elasticsearch

To give some background here, de/serialization logic is broadly used to convert ES API responses into a format suitable for editing in a form, and convert that form data back into a shape that is suitable for consumption the API. I think of it as the mechanism we use to uncouple the ES APIs from our UI. Some examples of what we currently use it to do are:

Reconcile interval, fixed_interval, and calendar_interval rollup job properties to a single editable date histogram interval value (jobs.js#L115)
Normalize similar fields in a rollup job to have a consistent format so that they can all be edited by the same field chooser component (jobs.js#L140, 147, 151)
Add/remove multiple levels of nesting which are required by the API but would otherwise be noise in the code (cluster_serialization.ts#L60

Can the form library handle these use cases? Can it handle any arbitrary change we may want to make to the shape of the ES object based on the user input?

As a corollary, if we decide to preserve de/serialization in its current form in the codebase, then do we need the serializer and deSerializer config options?

{({ field }) => {

What’s the reason behind the nesting of this parameter? In other words, why { field: { value, etc } } instead of { value, etc }

Passing a component

Did you consider an option like this?

<UseField
  path="title"
  form={form}
>
  {({ field: { errors, isSubmitted, value, onChange, isValidating } }) => (
    <TitleField
      errors={errors}
      isSubmitted={isSubmitted}
      value={value}
      onChange={onChange}
      isValidating={isValidating}
      any={1}
      additional={2}
      prop={3}
    />
  )}
</UseField>

{/* If manually passing through those props is necessary _every_ time,
then the consumer can solve this problem with composition. */}
function passFieldPropsToElement(element) {
  // If the consumer defines their props with different names that what is provided by
  // field then they could perform that mapping here.
  return ({ field }) => (
    React.cloneElement(element, { ...field });
  );
}

<UseField
  path="title"
  form={form}
>
  {passFieldPropsToElement(
    <TitleField
      any={1}
      additional={2}
      prop={3}
    />
  )}
</UseField>

This still separates concerns and has the added benefit of reducing UseField's interface and decoupling TitleField from the form library, because its props can be defined based on what it needs as opposed to what UseField provides. This means you could unit test TitleField without needing to create a field prop (which is something forced upon you by the form lib), share the components with code that don't use the form lib, or migrate away from the form lib without needing to change its props.

Form Schema

This schema seems great! Do we even need the config prop? Can we remove support for the config prop and require consumers to use the schema? Can we remove the other defaultValue prop, too?

Validation

I see a lot of complexity here to enable reusable validators and I’m not convinced of their value yet (or the value of the error codes)… can we simplify this for now and add back functionality later once the value of reusable validators is more obvious to all of us?

For example, we could begin by expecting consumers to define the validation array as an array of validator functions (not objects). Then, once the need for reusable validators becomes obvious to all of us, we could add support for validator objects as well, which will behave the way you’ve described them. This change will be backwards compatible because the implementation code can check to see whether a function or an object has been provided.

I just want to point out that with this change, the consumer can still create their own reusable validators, by moving the logic for customizing the message to the validator factory function. I think the consumer would gain even more control over how to customize the validator's behavior/messages this way:

validation: [
  startsWithCharField('.', ({ char }) => (
    `The title can't start with: ${char}`
  )), // validator factories can have any kind of customization logic you can think of
}]

On a separate note, is there any way to validate a field when the user is interacting with a different field? For example, in the Rollup Job Wizard, the rollup index and index pattern fields are both validated when the user interacts with one of them.

isValidationAsync

Is this necessary? Is it possible to check if the validator function returns a thenable, and use that to infer that it’s async?

Adding the "edit" capability to a form has never been so easy

Nice!!! :D

{({ rows, addRow, removeRow }) => (

This is awesome!

I suggest we use as generic a name as possible here, e.g. item, addItem, and removeItem. This seems like an extremely valuable functionality in many other apps in Kibana (e.g. Visualize app, TSVB) and I think the word “row” risks undercommunicating its value by implying its somehow related to rows as a visual concept, e.g. table rows or form rows.

jloleysens · 2019-07-24T07:54:50Z

src/plugins/elasticsearch_ui_shared/static/forms/hook_form_lib/hooks/use_field.ts

+            });
+          }
+        }),
+      Promise.resolve()


Very cool pattern! I've never thought of Promise.resolve() as the empty case.

I guess this could be slightly less verbose with the imperative alternative:

for (const validation of validations) { const validationResult = await runValidation(validation); // ...

(but you don't have to implement, just wanted to point out the cool use of Promise+reduce here. 🙂)

Good idea, I updated with the imperative alternative. 👍

jloleysens · 2019-07-24T08:24:20Z

Great work @sebelga! It was really fun to read this PR and learn more about general form building that happens across applications. I'm sure that this can create a lot of value 👍.

I'd like to add another thought in addition to @cjcenizal .

My gut feeling at this point is that there are libraries (like formik+yup for forms and rxjs for observables) that do very similar things to this lib and which could be leveraged to reduce our code size. I have not done an investigation here so I can't say what we would gain or lose. So I'd like to know whether there is specific reasoning behind not using them (and wrapping them?) vs. rolling our own?

sebelga · 2019-07-24T15:42:09Z

When do we expect consumers to use this default? Do we need it?

It is always a good practice to have a default. Some consumers might have styled their <input /> tag and don't need any specific component to be provided.

say goodbye to serialize and unserialize payloads for Elasticsearch

Nothing has changed, and obviously, business logic needed for serialization will still be needed. I was only referring that there will not be the need of having those anymore:

export const serializeAutoFollowPattern = ({ indexPatterns, remoteCluster }) => ({
  remote_cluster: remoteCluster,
  inex_patterns: indexPatterns
})

As we can set correctly the path: <UseField path="remote_cluster" form={form}>.

Again, it's the dev that decides how he wants to structure the objects. The form library does not handle any business logic.

As a corollary, if we decide to preserve de/serialization in its current form in the codebase, then do we need the serializer and deSerializer config options?

Yes, as I might prefer to have my serializer defined on the field and you might decide to have them elsewhere.
The config serializer has also another purpose. Some EUI component like the Selectable has some specific format to save its state. But obviously, we don't want that EUI state when outputting the form data, so the serializer is there to transform one format to the other.

{({ field }) => {

What’s the reason behind the nesting of this parameter? In other words, why { field: { value, etc } } instead of { value, etc }

That's a mistake in the example. It is indeed {(field) => ()}

Passing a component

Did you consider an option like this?

Yes I considered it... and this is my first example (approach 1) on how to customize the EUI 😊

Form Schema

This schema seems great! Do we even need the config prop? Can we remove support for the config prop and require consumers to use the schema? Can we remove the other defaultValue prop, too?

I prefer to maintain them and not force the consumer to use a schema if not needed.

isValidationAsync

Is this necessary? Is it possible to check if the validator function returns a thenable, and use that to infer that it’s async?

Can you show me how you would do this? I haven't managed unless we execute the function.

{({ rows, addRow, removeRow }) => (

This is awesome!

I suggest we use as generic a name as possible here, e.g. item, addItem, and removeItem. This seems like an extremely valuable functionality in many other apps in Kibana (e.g. Visualize app, TSVB) and I think the word “row” risks undercommunicating its value by implying its somehow related to rows as a visual concept, e.g. table rows or form rows.

OK I will change the names

…-lib

elasticmachine · 2019-07-25T05:36:35Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-07-25T07:13:29Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

alisonelizabeth

@sebelga this looks great! Can't wait to start using it :)

I believe I tested through all the different scenarios you provided in your comment. I found a couple typos that I commented on and had one question about duplicate code.

A couple other comments:

I noticed when formatters is provided, it transform the value in the UI as well. So even if I typed "north carolina" it was transformed to "NORTH CAROLINA". Is that intentional?
I wondering the same thing @cjcenizal mentioned as far as if it's necessary to have both the config prop and schema
I was having trouble testing the validation. Is there anything else I need to do besides providing the validation array with the validator func in the schema, like so:

    validation: [
      {
        validator: ({ value }) => {
          if ((value as string).startsWith('.')) {
            return {
              code: 'ERR_FORMAT_ERROR',
              message: `The title can't start with a dot (.)`,
            };
          }
        },
      },
    ],

I think it would be super helpful if you transferred your comments on this PR into a readme as part of this PR. WDYT?

alisonelizabeth · 2019-07-25T12:59:57Z

src/plugins/elasticsearch_ui_shared/static/forms/hook_form_lib/components/form_data_provider.ts

+
+  useEffect(() => {
+    subscription.current = form.__formData$.current.subscribe(data => {
+      // To avoid re-rendering the the children for updates on the form data


typo: delete extra the

alisonelizabeth · 2019-07-25T13:40:17Z

src/plugins/elasticsearch_ui_shared/static/forms/hook_form_lib/hooks/use_field.ts

+      return validationResult;
+    };
+
+    // Execute each validations for the field sequencially


typo sequencially --> sequentially

alisonelizabeth · 2019-07-25T14:05:52Z

src/plugins/elasticsearch_ui_shared/static/forms/hook_form_lib/hooks/use_field.ts

+    // we will clear the errors as soon as the field value changes.
+    clearErrors([VALIDATION_TYPES.FIELD, VALIDATION_TYPES.ASYNC]);
+
+    const runValidation = async ({


this looks like the same code as in validateSync except with async/await. is it possible to remove some of this duplicate code?

That was my initial goal but did not find a clean way to do it. One requires an async function to run and the other not. Suggestions welcome!

alisonelizabeth · 2019-07-25T14:10:37Z

src/plugins/elasticsearch_ui_shared/static/forms/hook_form_lib/hooks/use_field.ts

+   * method allows us to retrieve error messages for certain types of validation.
+   *
+   * For example, if we want to validation error messages to be displayed when the user clicks the "save" button
+   * _but_ in caase of an asynchronous validation (for ex. an HTTP request that would validate an index name) we


typo caase --> case

alisonelizabeth · 2019-07-25T14:13:08Z

src/plugins/elasticsearch_ui_shared/static/forms/hook_form_lib/hooks/use_form.ts

+
+  /**
+   * Remove all fields whose path starts with the pattern provided.
+   * Usefull for removing all the elements of an Array


type Usefull --> Useful

alisonelizabeth · 2019-07-25T14:16:12Z

src/plugins/elasticsearch_ui_shared/static/forms/hook_form_lib/lib/utils.ts

+  }
+
+  if (throwIfNotFound) {
+    throw new Error(`Can't acess path "${path}" on ${JSON.stringify(object)}`);


typo acess --> access

sebelga · 2019-07-29T10:47:02Z

Thanks for the review @alisonelizabeth !

I noticed when formatters is provided, it transform the value in the UI as well. So even if I typed "north carolina" it was transformed to "NORTH CAROLINA". Is that intentional?

This is the purpose of the formatter. 😊 If you have a "numeric" field, you'd want a formatter to convert the string to int for example.

I wondering the same thing @cjcenizal mentioned as far as if it's necessary to have both the config prop and schema.

Yes as for small forms you might just have 2/3 simple field and just the need to add a config={{ type: 'comboBox' }} with no validation. No need to have the whole schema boilerplate for simple usecases.

I was having trouble testing the validation. Is there anything else I need to do besides providing the validation array with the validator func in the schema.

If you were using the Field component to render, then you need to send the form to see the validation. Unless it's something else then I'd need to look at your code.

alisonelizabeth · 2019-07-30T01:47:05Z

@sebelga thanks for explaining! i was able to get the validation to work; i think i had mistakenly defined validation: [] instead of validations: [] (plural) when i first tested it. It's singular in the PR comment, so if you move some of this information over to a readme, that will need to be updated!

elasticmachine · 2019-08-29T18:37:38Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-08-30T09:50:54Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

jloleysens · 2019-08-30T12:24:35Z

@sebelga Nice work! I read through the changes you made re FormProvider and it looks good! Do you think this pattern is common enough:

  ...
  const { form } = useForm({ ... });

  return (
    <FormProvider form={form}>
      <form onSubmit={form.submit} noValidate>
        <UseField path="title" />
        <button>Send form</button>
      </form>
    </FormProvider>
  );

that we can add something like this:

   ...
   const { form } = useForm({ ... });

   return (
     <Form>
      <UseField path="title" />
      <button>Send form</button>
    </Form>
  );

to wire it up for us? Just a very ~~thing~~ thin wrapper around form element similar to this: https://github.com/jaredpalmer/formik/blob/master/src/Form.tsx

sebelga · 2019-08-30T13:12:08Z

@jloleysens

I guess you meant const { Form } = useForm({ ... }); (uppercase) right?

I thought of it, but the pattern <form onSubmit={form.submit} noValidate> is actually not supported in Kibana because of the <EuiComboBox />. This component has an event listener on keydown "ENTER" that interferes with the form onSubmit sent by the browser.

In Kibana it should be <EuiForm> ... </EuiForm> which basically renders a <div> tag. So instead of deciding for the consumer what tag should wrap the HTML form, I decided to not enforce that decision. Does it make sense? What do you think?

But I agree it would be much nicer :)

[EDIT]: Although, based on your idea, we could have

const { Form } = useForm({ ... });

<Form>
  <EuiForm>
    <UseField path="title" />
  </EuiForm>
</Form>

But suddently having 2 Form tag seems strange :/. Maybe keep the name <FormProvider> ?

jloleysens · 2019-08-30T14:00:52Z

@sebelga

Thanks for the clarification! Yes that makes sense now!

jloleysens

Did a final look through the code - discussed offline that we'd still add some tests. Happy that my concerns have been addressed, lgtm!

cjcenizal

💎 This looks great Seb!! I do think we could ensure a stabler dev path by removing stripWhitespace for now and introducing it once we've used this in production a couple times to prove the use-case, but I don't want to block this PR on that. I also had a few questions and suggestions for comments and nits and stuff, nothing important though. Great work!

cjcenizal · 2019-08-30T04:01:28Z

src/plugins/es_ui_shared/static/forms/hook_form_lib/components/form_data_provider.ts

+      // that we are **not** interested in, we can specify one or multiple path(s)
+      // to watch.
+      if (pathsToWatch) {
+        const valuesToWatchToArray = Array.isArray(pathsToWatch)


Nit: I think valuesToWatchArray works better here. valuesToWatchToArray sounds like a function for converting something to an array.

cjcenizal · 2019-08-30T18:21:27Z

src/plugins/es_ui_shared/static/forms/hook_form_lib/components/use_array.ts

+export interface ArrayItem {
+  id: number;
+  path: string;
+  isNew: boolean;


What's the role of isNew?

It allows the consumer to know if the item has been created (by an addItem() call) or if the comes from the defaultValue.

cjcenizal · 2019-08-30T18:32:56Z

src/plugins/es_ui_shared/static/forms/hook_form_lib/components/use_array.ts

+  isNew: boolean;
+}
+
+export const UseArray = ({ path, initialNumberOfItems, children }: Props) => {


I'd love to see some brief docs around UseArray, for example:

/** * Use UseArray to control fields that contain a selection of items, e.g. EuiComboBox and * EuiSelectable. */

cjcenizal · 2019-08-30T18:34:24Z

src/plugins/es_ui_shared/static/forms/hook_form_lib/components/use_field.tsx

+}: Props) => {
+  const form = useFormContext();
+
+  if (typeof defaultValue === 'undefined') {


Is there a benefit to this form over defaultValue === undefined?

No real benefit, just the way I used to write it. 😊

cjcenizal · 2019-08-30T18:58:00Z

src/plugins/es_ui_shared/static/forms/hook_form_lib/constants.ts

+export const VALIDATION_TYPES = {
+  FIELD: 'field', // Default validation error (on the field value)
+  ASYNC: 'async', // Throw from asynchronous validations
+  ARRAY_ITEM: 'arrayItem', // If the field value is an Array, this error would be thrown if an _item_ of the array is invalid


Is "throw" still accurate in this comment and the comment above since validation doesn't throw errors any more?

cjcenizal · 2019-08-30T18:58:36Z

src/plugins/es_ui_shared/static/forms/hook_form_lib/hooks/use_field.ts

+    formatters = [],
+    fieldsToValidateOnChange = [path],
+    isValidationAsync = false,
+    errorDisplayDelay = form.options.errorDisplayDelay,


Just providing one perspective as a consumer of this code -- it's ultimately your decision.

cjcenizal · 2019-08-30T19:08:26Z

src/plugins/es_ui_shared/static/forms/hook_form_lib/hooks/use_field.ts

+    };
+
+    const runSync = () => {
+      // Sequencially execute all the validations for the field


Typo: sequencially -> sequentially

cjcenizal · 2019-08-30T19:24:18Z

src/plugins/es_ui_shared/static/forms/hook_form_lib/hooks/use_form.ts

+  const validateFields: Form<T>['__validateFields'] = async fieldNames => {
+    // Fields that are dirty have already been validated when their value changed. Thus their errors array
+    // already contains any possible error.
+    // So when *no* fieldNames is provided, we only validate fields that haven't been changed by the user.


Hey Seb, I'm sorry I'm still having trouble wrapping my head around this. Could you explain step by step the state changes made to a field when a user enters input, and how those state changes affect whether the field is validated/not validated here? "Only validate fields that haven't been changed by the user" is just not making sense to me... isn't the point of validation to check if a user's changes are valid or not? Maybe we should Zoom?

When a field changes, this method is called with a fieldNames array. It will only validate the fields provided in the fieldNames array.

If we call this function without fieldNames (probably when a user clicks the sendForm button), we only validate the fields that are pristine (= that haven't been changed by the user).

We have 2 fields: "name" and "lastName"

A user changes the "name"

form.validateFields(['name'] is called and thus the name field has the validation

The user clicks the sendForm button

form.validateFields() is called (no fieldNames are provided). ---> we only need to validate the "lastName" fields as "name" has already been validated

Is it clearer?

Ahhh, I see. Your second example really clarified the behavior of the code for me. I think the desired outcome (optimizing to only validate fields which haven't been validated) makes sense to me. But I don't believe the code communicates this intention and I'm afraid it will be very easy to accidentally break this behavior.

I suggest adding a isValidated state to fields (forgive me if this already exists; I haven't looked at the code in awhile). I suggest setting this state to true once validation executes for a given field, and to false once the user enters input. I believe this will make the intended behavior explicit and protect against someone accidentally breaking it. What do you think?

I think this adds unnecessary complexity and I think the comment above explains well that a dirty field is a field that has been validated (due to the fact that this method is called whenever its value changes).

// use_field.ts (inside the useEffect() { ... }, [value]) await form.__validateFields(fieldsToValidateOnChange); // use_form.ts (when calling submit()) const isFormValid = await validateFields();

protect against someone accidentally breaking it

When tests will be in place, we shouldn't worry about it.

We can work on the phrasing of the comment if you want to still make it more clear, but I don't think an extra state is needed.

We'll move forward with whatever you think makes the most sense. I'll just share a couple parting thoughts:

If the validation is async, it's possible for a field to be dirty, but not yet validated. I don't know how this affects the system but thought I should point it out.

Tests are great for letting a dev know they broke something. But to figure out why something is broken (or why it was working in the first place) I usually find myself reading the code. And I did genuinely have a difficult time understanding why this code works the way it does. :) Maybe others will too?

If the validation is async, it's possible for a field to be dirty, but not yet validated.

If you look at line L114 you will see that the form won't be valid until all the fields have finished validating.

https://github.com/elastic/kibana/blob/master/src/plugins/es_ui_shared/static/forms/hook_form_lib/hooks/use_form.ts#L114

cjcenizal · 2019-08-30T19:27:55Z

src/plugins/es_ui_shared/static/forms/hook_form_lib/hooks/use_form.ts

+    if (options.stripEmptyFields) {
+      return Object.entries(fields).reduce(
+        (acc, [key, field]) => {
+          if (field.value !== '') {


In response to my question about multiple whitespace, you replied:

Yes multiple whitespace === an empty field.

But in this check ' ' === '' will evaluate to false. I just want to double-check that this is what you intended?

Good catch 👍

cjcenizal · 2019-08-30T19:43:23Z

src/plugins/es_ui_shared/static/forms/hook_form_lib/hooks/use_field.ts

+          // The validator returned a Promise.
+          // Abort and run the validations asynchronously
+          hasToRunAsync = true;
+          break;


I think this can be simplified a bit by removing hasToRunAsync (haven't tested this though).

if (!!validationResult.then) { // The validator returned a Promise. // Abort and run the validations asynchronously return runAsync(); }

We also don't need to worry about clearing validationErrors if each helper defines it in its own scope.

const runAsync = async () => { const validationErrors: ValidationError[] = []; /* ... */ } const runSync = () => { const validationErrors: ValidationError[] = []; /* ... */ }

cjcenizal · 2019-08-31T13:53:18Z

src/plugins/es_ui_shared/static/forms/hook_form_lib/hooks/use_field.ts

+          continue;
+        }
+
+        if (!!validationResult.then) {


If I'm following this logic correctly, this means we first call validator and if it returns a promise, then we call runAsync which will call validator a second time. If the validator creates a network request, does that mean the request will be sent twice?

Great point. I added support to cancel the validation. The consumer can optionally add a cancel method to the promise he returns (in case of an HTTP request the best would be to use axios that has a cancelation API.

This cancel() method will be called before validating again the field.

Here is a "manual" example for a simple Promise:

let counter = 0; .... validations: [ { validator: () => { let canceled = false; const current = counter; console.log('Validating', current); const promise = new Promise(resolve => { setTimeout(resolve, 2000); }).then(() => { if (canceled) { console.log('Has been canceled, should not do anything....', current); } else { console.log('Finished long validation!', current); } }); // Attach a cancel() method (promise as any).cancel = () => (canceled = true); counter++; return promise; }, }, ],

And this is what is logged when entering "abc" in the field

Validating 0 Validating 1 Validating 2 Validating 3 Validating 4 Validating 5 Has been canceled, should not do anything.... 0 Has been canceled, should not do anything.... 1 Has been canceled, should not do anything.... 2 Has been canceled, should not do anything.... 3 Has been canceled, should not do anything.... 4 Finished long validation! 5

cjcenizal · 2019-08-31T14:12:00Z

@sebelga

I realized that in some cases we do need the validation to be synchronous. (The Combobox createOption() is synchronous so when we validate the value to decide to add it or not, we need a sync response)

In this particular example, this might be a flaw in the combobox implementation. If this is changed in EUI so that createOption() is async, then would this solve the problem? Or do you see more evidence that indicates synchronous validation is a broad requirement?

…-lib

sebelga · 2019-09-06T13:47:20Z

@cjcenizal I prefer the library to support synchronous validation at the cost of a few lines in the code. I think it is better than patching everywhere async is not supported. And you heard about the Gorilla and the banana? 😄 You make an async at one place and suddenly the whole chain has to be async (you pulled the jungle! )

elasticmachine · 2019-09-06T14:46:32Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

sebelga · 2019-09-06T15:32:56Z

retest

elasticmachine · 2019-09-06T16:45:50Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

…-lib

elasticmachine · 2019-09-09T09:46:48Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-09-09T13:32:53Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-11-12T07:50:12Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: a77823f

sebelga added 3 commits July 19, 2019 10:23

Initial commit

efcb118

Refactor api private methods

d614c97

Revert refactor private field API

64e9fd1

sebelga requested review from jen-huang and alisonelizabeth July 22, 2019 12:36

jloleysens reviewed Jul 23, 2019

View reviewed changes

Add setTimeout() to make subscribe callback async

31ef210

Add "checkbox" type and make form config optional

b91c6ee

jloleysens reviewed Jul 24, 2019

View reviewed changes

sebelga added 3 commits July 24, 2019 18:52

Merge remote-tracking branch 'upstream/master' into feature/hook-form…

d87780b

…-lib

Remove default field formatters

505284c

Remove type fom field config and message from Validation config

2dea2c4

Put back type to field config

0e5d4be

alisonelizabeth reviewed Jul 25, 2019

View reviewed changes

sebelga added 2 commits August 30, 2019 10:00

Refactor field.onValueChange()

b2fa949

Refactor: rename useFormState() to useFormContext() for clarity

2f0ed6c

jloleysens approved these changes Aug 30, 2019

View reviewed changes

cjcenizal reviewed Aug 30, 2019

View reviewed changes

cjcenizal approved these changes Aug 30, 2019

View reviewed changes

cjcenizal reviewed Aug 31, 2019

View reviewed changes

sebelga added 3 commits September 6, 2019 11:01

Merge remote-tracking branch 'upstream/master' into feature/hook-form…

6a1e2f9

…-lib

Wrap FormProvider in a <Form /> component + improve error display delay

1eddd42

Make CR changes + add support to cancel inflight validation

9b1e584

sebelga added 2 commits September 9, 2019 10:13

Merge remote-tracking branch 'upstream/master' into feature/hook-form…

7077f03

…-lib

Refactor: rename Form & Field types to avoid naming colision

a77823f

sebelga merged commit dbdb9bb into elastic:master Sep 9, 2019

sebelga deleted the feature/hook-form-lib branch September 9, 2019 12:15

sebelga mentioned this pull request Sep 9, 2019

[7.x] Hook form lib (#41658) #45122

Merged

sebelga added a commit that referenced this pull request Sep 9, 2019

Hook form lib (#41658) (#45122)

617d4fb

alisonelizabeth mentioned this pull request Jan 17, 2020

System index templates can't be edited #55145

Closed

7 tasks

jkelastic mentioned this pull request Jan 17, 2020

System index templates can't be edited #55229

Merged

7 tasks

Hook form lib #41658

Hook form lib #41658

Conversation

sebelga commented Jul 22, 2019 • edited Loading

Form library with hooks!

Motivation

Main building blocks

useForm() hook & <UseField />

Design decisions

Raw form state

Performance

Customize the UI

Approach 1: Render props pattern

Approach 2: Passing a Component

Field configuration

Form Schema

Validation

Asynchronous validation

Default value object

Dynamic form row with <UseArray />

Access the form data with <FormDataProvider />

elasticmachine commented Jul 22, 2019

elasticmachine commented Jul 22, 2019

💚 Build Succeeded

jloleysens Jul 23, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Jul 23, 2019

💚 Build Succeeded

elasticmachine commented Jul 23, 2019

💚 Build Succeeded

cjcenizal commented Jul 24, 2019 • edited Loading

jloleysens Jul 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jloleysens commented Jul 24, 2019

sebelga commented Jul 24, 2019

elasticmachine commented Jul 25, 2019

💔 Build Failed

elasticmachine commented Jul 25, 2019

💔 Build Failed

alisonelizabeth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebelga commented Jul 29, 2019 • edited Loading

alisonelizabeth commented Jul 30, 2019

elasticmachine commented Aug 29, 2019

💚 Build Succeeded

elasticmachine commented Aug 30, 2019

💚 Build Succeeded

jloleysens commented Aug 30, 2019 • edited Loading

sebelga commented Aug 30, 2019 • edited Loading

jloleysens commented Aug 30, 2019

jloleysens left a comment

Choose a reason for hiding this comment

cjcenizal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebelga Sep 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebelga commented Jul 22, 2019 •

edited

Loading

`useForm()` hook & `<UseField />`

Dynamic form row with `<UseArray />`

Access the form data with `<FormDataProvider />`

jloleysens Jul 23, 2019 •

edited

Loading

cjcenizal commented Jul 24, 2019 •

edited

Loading

jloleysens Jul 24, 2019 •

edited

Loading

sebelga commented Jul 29, 2019 •

edited

Loading

jloleysens commented Aug 30, 2019 •

edited

Loading

sebelga commented Aug 30, 2019 •

edited

Loading

sebelga Sep 6, 2019 •

edited

Loading