[Form lib] Correctly add field to form on component mount

[sebelga]

This PR fixes #74677. A regression was introduced from the work of memorizing the field and form objects and handlers in (#71237).

Security solution forms

Fixing the form lib to correctly add and remove fields to the form hook has revealed some bugs in the security solution plugin. Mainly in the <StepAboutRuleComponent />.

I discovered that there were nested form fields which are not allowed in the form lib. There was a riskScore field and a riskScore.value field. In the form library, fields correspond to the leaves of an object.

// There are 3 fields for the `riskScore` object. But the object **is not** a form field
const formData = {
  riskScore: {
    value: 'hello', // field = 'riskScore.value'
    isMappingChecked: true, // field = 'riskScore.isMappingChecked'
    mapping: [] // field = 'riskScore.mapping' (This should probably be a <UseArray /> field
  }
}

I fixed the issue by removing the riskScore.value field.
I found the same issue with the security level and also fixed it.

Lesson learned

The lesson learned from the regression introduced in the lib is the importance of the order of the useEffect on the file.

// This was how it was with the bug

// [1]
const onValueChange = useCallback(async () => {
  ...
  await form.validateFields([path]); // Validate the current field (passing its "path")
}, [value, <other-deps>]);

// [2]
const field = useMemo(() => {
  return {
    value,
    ...
  }
}, [value, <other-deps>]);

useEffect(() => {
  if (!isMounted.current){
    return;
  }
  onValueChange(); // [4]
}, [onValueChange])

useEffect() => {
  form.addField(field); // [5]
  
  return () => {
    form.removeField(path); // [3]
  }
}, [field, path]);

In the above snippets, we can see that when the field value changes it

1. Creates a new callback onValueChange
2. Creates a new field object
3. Remove the field from the form (!)
4. Call onValueChange() which in turn asks to validate the field
5. Add the field back to the form

So on step 4, it asks to validate the field but the field was removed on step 3! So nothing to validate === bug.

The solution is simple, but it took me a while to figure it out! 😢

// This was how it was before

// [1]
const onValueChange = useCallback(async () => {
  ...
  await form.validateFields([path]); // the current field that has changed
}, [value, <other-deps>])

// [2]
const field = useMemo(() => {
  return {
    value,
    ...
  }
}, [value, <other-deps>]


useEffect() => {
  form.addField(field); // [4]
  
  return () => {
    form.removeField(path); // [3]
  }
}, [field, path]);

useEffect(() => {
  if (!isMounted.current){
    return;
  }
  onValueChange(); // [5] // The field has already been added so no more problem!
}, [onValueChange])

[sebelga]

Note: by wrapping the button click event inside the await act, we don't need the waitFor call below. This allows us to get a proper error from the test instead of a jest timeout.

[rylnd]

That should be the only use of waitFor in this file.

[sebelga]

Yes it seems so but I didn't touch the other tests that also wrap the expect inside a waitFor. This test was the one failing and the timeout error did not let me see why it was failing.

[rylnd]

Oh, absolutely! I just meant that the import is now unused.
Edit: And I apologize, because I am wrong! 🤦

[elasticmachine]

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

[sebelga]

@elasticmachine merge upstream

[sebelga]

@elasticmachine merge upstream

[jloleysens]

Great work @sebelga ! I tested locally with the my Grok form processor component that uses UseArray and the reordering works as expected with these changes (unfortunately don't have a PR up for this, yet).

Did not test mappings or security form changes.

[jloleysens]

Suggested change

	// If *not* all the fiels have been validated, the validity of the form is unknown, thus still "undefined"
	// If *not* all the fields have been validated, the validity of the form is unknown, thus still "undefined"

[jloleysens]

After reading this I had a look at the type annotations for the FormHook interface. Do you think you could add some docs to each of those fields to explain intended use? I see __addField is called inside of useField hook and I am assuming that is the call site you are referring to here?

[sebelga]

Yes I could do that. Indeed I was referring to __addField. Although all those handlers starting with a double underscore are not meant to be public and thus I wouldn't want to have a doc appear on top of their suggestion in the IDE.

[sebelga]

Thanks for the review @jloleysens ! I keep a note about adding some doc on top of the interface properties. I will do that along with some other improvement of TS on the lib that I have in mind.

[sebelga]

@elasticmachine merge upstream

[patrykkopycinski]

Thank you @sebelga 💪

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: b0c271f

Build metrics

async chunks size

id	value	diff	baseline
indexManagement	1.6MB	+439.0B	1.6MB
ingestPipelines	705.5KB	+22.0B	705.5KB
securitySolution	9.5MB	+467.0B	9.5MB
total		+928.0B

page load bundle size

id	value	diff	baseline
esUiShared	990.6KB	+1.4KB	989.2KB

History

• 💔 Build #70513 failed 44b5572
• 💔 Build #70498 failed c529e80
• 💔 Build #70479 failed 20e723d
• 💔 Build #70458 failed ef8aa16
• 💔 Build #70447 failed d04098b

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

[sebelga]

Thanks for the review @patrykkopycinski!