[Form lib] Get ready for v1.0.0

[sebelga]

While working on adding test coverage for the form library I discovered a few issues with a high impact on the consuming code. I found a memory leak problem that allowed me to fix a data flow problem.

This implied that I had to go and fix the consuming code (mainly the mappings editor) and do some refactor in places where we used some hacks in the mappings editor "to make it work".

These discoveries are very welcome and put the form library in much better shape for consumers. This also means that the "Add tests to the form lib" task that I had planned is now much bigger and this meta issue will serve to communicate how I plan to proceed to get the form lib to a v1.0.0 stable version (solid architecture, high-quality test coverage, good README.md and obviously all bug fixes).

v1.0.0

• PR 1 ([form lib] Fix issues + add test coverage #64647)

  • Make a copy (no changes) of the Testbed in es_ui_shared to be bale to use it in OSS
  • Add basic test coverage
  • Fix issues (important one is the memory leak + data flow fix)
  • Check for regression on consuming code
    • Index templates
    • Mappings editor
    • Ingest pipelines
    • SIEM
  • Refactor hacky code in mappings editor to "make it work" before the fix of the data flow in the form lib

• PR 2 ([test_utils/Testbed] Move to src/test_utils folder (OSS) #66898)

  • Remove the Testbed utils from x-pack
  • Point all consuming code to the es_ui_shared copy made in PR 1

• PR 3

  • Complete test coverage of the form lib

• PR 4

  • Refactor useForm and useField hook to memoized the objects returned and be able to properly declare the form object in the consumer useEffect dependency array.
  • Add tests to make sure of that behaviour

• PR 5 (v1.0.0)

  • Add README.md

v1.1.0

• PR 6 (v1.1.0)
  • Add a useFormData(<ArrayOfPathsToWatch>) hook to the library. This will allow consumers to get updates of the form data without the need of the <FormDataProvider>.
    Depending on the use case, one will be more suited than the other. If the whole component is wrapped by a <FormDataProvider>, then the useFormData(['someField']) hook would make more sense. If there is a lot of JSX in the component and only a small part of it needs to react to form updates, then <FormDataProvider> makes more sense.
• PR 7 ([Form lib] Export internal state instead of raw state #80842)
  • Remove the "Raw" flat internal state for fields and always returned the expected interface for the form. This tiny optimization creates more friction than benefits for the consumer that needs to declare 2 interfaces for his form (In and Out). Let's always return the expected form object.

[elasticmachine]

Pinging @elastic/es-ui (Team:Elasticsearch UI)

[cjcenizal]

@sebelga Thanks for sharing your plan for moving forward. This looks pretty clear to me. I have a few questions:

• Instead of creating a copy of the testbed, can we create an initial PR that moves the testbed into OSS and updates all consumers? I'm uncomfortable with having duplicate modules in the codebase because it creates possibilities for confusion and divergence. Seems cleaner to just cut over in one go.
• I'm interested to know what basic test coverage vs. complete test coverage entails?
• As part of this effort, can you explore addressing Remove temporary src/plugins/es_ui_shared eslint overrides #49554 to see if there are opportunities for further refactoring?
• Do we have any examples of code today where <FormDataProvider> is so cumbersome that we'd switch to the useFormData hook? Trying to wrap my head around it.

[sebelga]

Instead of creating a copy of the testbed, can we create an initial PR that moves the testbed into OSS and updates all consumers? I'm uncomfortable with having duplicate modules in the codebase because it creates possibilities for confusion and divergence. Seems cleaner to just cut over in one go.

I've thought about it and I don't share your concern. During a very short period of time, there will be 2 copies of the testbed utils in master. This allows me to move forward with the most important task, which is to merge asap #64647 without delaying it further. There won't be time for confusion as the next PR will be to remove the x-pack copy and point to the es_ui_shared folder.

I'm interested to know what basic test coverage vs. complete test coverage entails?

It means that I'd like to merge PR 1 asap and not wait to have complete test coverage of the lib to do it. The lib has already some good coverage and I prefer to put this work on pause and focus on getting the PR ready to be merged. I will complete in PR 3.

As part of this effort, can you explore addressing #49554 to see if there are opportunities for further refactoring?

Great point, although that would be a totally different work. Removing the override means touching/refactoring different pieces of code (the lib but also the request hook I guess) so this should be done in a separate PR.
This is a very delicate move and we should be confident of the coverage of both the consuming code + the lib code (form lib but also request hook which currently has its tests skipped) before letting eslint automatically add dependencies to arrays.

Do we have any examples of code today where is so cumbersome that we'd switch to the useFormData hook? Trying to wrap my head around it.

There are many places where it would be useful and more convenient for code readability. One of them is here:

https://github.com/elastic/kibana/blob/master/x-pack/plugins/ingest_pipelines/public/application/components/pipeline_form/pipeline_test_flyout/pipeline_test_flyout_provider.tsx#L21

Instead of the whole subscription block + the useState, we will have

export const PipelineTestFlyoutProvider: React.FunctionComponent<Props> = ({ closeFlyout }) => {
  const form = useFormContext();
  const formData = useFormData();
  // Or...
  // const formData = form.useData();

  return (
    <PipelineTestFlyout
      pipeline={formData}
      closeFlyout={closeFlyout}
      isPipelineValid={form.isValid}
    />
  );
};

[jloleysens]

@sebelga I had one suggestion :)

What do you think about updating form updates (OnFormUpdateArg) to contain only validate and data? That way consumers will always need to call validate and not have isValid which can be boolean|undefined.

Not sure what the implications might be for performance but I think it would simplify consumer code to:

async function myCoolListener ({ validate, data }) => { if (await validate()) {...} else {...} }

I might be missing a case that isValid specifically makes easier though?

[sebelga]

@jloleysens Why force the consumer to call a function if we can provide him with the necessary boolean? Also, the current API already allows you to do the above if the consumer prefers it.

I wouldn't recommend this though as the whole point of having 3 states (undefined, false, true) is what brings performance to the lib. We don't need to validate all the fields each time a user hits a keystroke on 1 field.

The normal flow we expect a user to have

1. Fill the form, 1 field at the time. The form lib validates only that field.
2. Click "Save" button. At that point, if the validity is still undefined, we call validate()

Also, your suggestion requires that each consumer declare a separate state to keep track of form validity (to be able to toggle a button disabled state).

const [isMyFormValid, setIsMyFormValid] = useState<boolean | undefined>(undefined);

It adds complexity to the consumer that I don't think is necessary as we already know that information.