Form generators refactor - First steps and factored out FormInput

[gpaoloni]

Primary reviewer:

Description

This PR is a proposal to bring some order to our form generators module. It also includes some changes in the way they are generated.

• Removes createFormFromDefinition function. This is instead replaced by a custom hook useCreateFormFromDefinition that will receive mostly the same parameters and
  • Abstracts away the repeated logic in every place where a form was built in the previous way (that's why I think a custom hook is a good fit).
  • Will take care of memoizing the computed form resulting from the given form definition. This was previously a burden in the caller components, and in most cases after some time we ended up ruining the memoization anyways.
    To see the benefits of this change you can sit in commit b816e89, which is the current state of our forms plus a debug log. Interact with the tabbed forms while keeping an eye to the Chrome console and you'll see that the form is being re-built in every re-render of the parent component. Then sit in commit 328286f that already handles the creation of the form via this hook, and check that it is only created once, and then kept in sync via rerenders (but the form as such does not needs to be entirely rebuilt from the definition).
    Removed after @stephenhand gathered empirical evidence that this memo is not really necessary :P
• Introduces a file inputGenerator.tsx, with createInput function which is intended to replace the existing getInputType one. It is intended to be simpler as the logic for each component should be factored out from it, leaving this function as the link between a FormItemDefinition and it's corresponding input.
• Adds a new folder under components, forms/ where I think we could "factor out" our custom inputs (right now they all live in the 1k lines file src/components/common/forms/formGenerators.tsx.tsx 😅). Inside I've exemplified with top level files types.ts and styles.tsx, where the code shared among all the component types will live. There's also a FormInput/ module that contains FormInput.tsx and styles.tsx, which make the base input that corresponds to FormInputType.Input. This is the easier one since it's the base style, but you get the idea. I'm also bundling the unit tests for this module within itself. Happy to move this to the usual __tests__ folder if preferred by the team, I've just been thinking this way can be easier to work with.

Why I think this refactor is a good idea (this are mostly my opinions):

• The code for the form generators module is more organized and easier to maintain.
• The styles for the above will live in a more specialized place, which will help reducing the amount of code we have in the root src/styles/HrmStyles.tsx.
• The code that calls the form generators is cleaner too, it just needs to call single custom hook and will receive the generated form.
• Writing tests for the UI, the createItem and the hook as separate things will be easier to handle than trying to write tests for the big monster we currently have 😆.

Checklist

• Corresponding issue has been opened
• New tests added
• [n/a] Feature flags added
• [n/a] Strings are localized
• Tested for chat contacts
• Tested for call contacts

Related Issues

Fixes #....

Verification steps

Manual regression test to verify the forms generated (tabbed forms, case forms..) are still working as intended, no change in the behavior is expected.

• Forms are generated and works as expected, preserving the same UI. The required and error states should be the same (asterisk and error message appears upon validation, respectively).
• The updateCallback is invoked as expected, properly updating the Redux state (if pertinent).

[stephenhand]

This is an awesome refactor.

Much happier that this is refactored internally in Flex rather than going into the form defs module, didn't really want a clatter of React deps going in there.

Although it might make sense to put them in a 3rd module at some point, but only when something else needs to consume them.

I would move the 'forms' directory out of the 'common' directory as part of this refactor. I would probably move everything else out of there too - there doesn't really seem to be any distinction between stuff in components/common and any of the other components...

[stephenhand]

I think we should decompose this into a hook and a simple function that the hook calls, and make them both public.

I've had to change the memoisation behaviour a number of times in the past on individual forms because the 'standard' behaviour is broken in some scenarios - I had some issues around checking whether the form was in the correct state to show the 'you have unsaved edits' dialog that fixing involved changing the memo change - which it probably drastically reduced the memoisation effectiveness but didn't noticeably affect performance.

So we should leave ourselves the option of hand rolling the hooks but still being able to reuse the core logic, should be a straightforward change.

[stephenhand]

FYI the E2E test failures seem to be related to your changes, rather than just flake

[stephenhand]

Is more efficient, since the creation of the form takes place only once. This is achieved by doing some React black magic, but as long as that lives inside useCreateFormFromDefinition, and we confirm that it works fine, I think is a good trade-off.
As a disclaimer, I know that efficiency is not a top priority until we reach a bottleneck, but re-creating a form of a hundred elements each time the form re-renders make no sense to me. If we are fine with that we can get rid of all the momoizations we've been doing and just keep the code simpler.

I did a quick profiling analysis of our use of memo in the forms and this is what I found:

I did some profiler tests of changing the values in fields on the child form on Aselo Development for a new offline contact. This is where I expect the memo optimisation to kick in, the values are changing in Redux kicking off a component render, but the memo prevents the form generation step from rerunning.

Versions where the form had effective memoisation were faster than the current implementation - by approx 40ms each time moved away from a field you just updated, small but not trivial.

However, as you say, the current implementation in many cases isn't no memoisation, it's ineffective memoisation. Removing the memoisation completely made it almost as fast as the memoised version, maybe 5-10ms slower. Removing the overhead of memoising in the first place saves almost as much as properly memoising.

Couple of reasons why this is the case:

• The form is being rerendered on the React level anyway, memo or not. This is because we aren't memoising the render step, we are only memoising the bit where we generate the JSX.Element objects that we pass into the react renderer. The 'rerender' on the React level is still happening
• Optimisations in the React rendering level mean that the React render doesn't translate into a browser render. React is smart enough to know that what it's being asked to rerender hasn't really changed, so doesn't bother - so the 'rerender' stage is fast anyway

I experimented with some relatively straightforward optimisations we could do to speed up forms that seemed to have a reasonable impact, but I won't go into those here, it's a bit off topic.

[gpaoloni]

Does it makes sense for this module to live in /components/forms or would you prefer this to go one level up, outside of components folder? I think we could treat forms as it's own module right?

[stephenhand]

Looks great! 🚀