(BREAKING) Migrate from Formik to RHF #349

samuelmale · 2024-07-11T14:50:18Z

Requirements

This PR has a title that briefly describes the work done including the ticket number. If there is a ticket, make sure your PR title includes a conventional commit label. See existing PR titles for inspiration.
My work conforms to the OpenMRS 3.0 Styleguide and design documentation.
My work includes tests or is validated by existing tests.

Summary

This PR introduces a major migration from Formik to React Hook Form, along with the implementation of a new form processing architecture. The changes focus on enhancing the form handling capabilities, improving performance, and providing a more flexible and maintainable codebase. The introduction of form processors abstracts the form engine from the underlying domain objects, allowing for more modular and reusable code. Additionally, the field submission handlers have been refactored to form-field adapters for better clarity and naming consistency. Form validation has also been updated to address specific challenges.

Major Changes Made

Migration from Formik to React Hook Form:
- Replaced Formik with React Hook Form across the project to leverage its performance benefits and simpler API.
- Updated all form components to use React Hook Form's hooks and methods for managing form state and validation.
Introduction of Form Processors:
- Implemented a new form processing architecture to abstract the form engine from the underlying domain objects (e.g., Encounter, Patient).
- Form processors provide an abstraction layer where the form engine is agnostic about the underlying domain object. This allows for the implementation details of the domain to be defined in specific processors, such as the EncounterFormProcessor. These processors handle tasks such as resolving dependencies, initializing form values, and processing submissions, ensuring a clean separation of concerns and enhancing code modularity.
EncounterFormProcessor Implementation:
- Defined the EncounterFormProcessor to manage the encounter-specific logic, including preparing the form schema, processing submissions, and resolving dependencies.
- Implemented methods to handle the initialization of form values, submission of encounter data, and saving of related entities such as patient identifiers, patient programs, and attachments.
- Utilized custom hooks (useCustomHooks) to manage the loading state and context updates for encounters.
Refactoring of Field Submission Handlers to Form-Field Adapters:
- Renamed field submission handlers to form-field adapters to better reflect their purpose and functionality.
- Added a tearDown function to the underlying interface of form-field adapters to handle any necessary cleanup.
Form Validation Management:
- Initially planned to use Zod's integration with React Hook Form for validation but encountered challenges in managing errors vs. warnings with different severity levels and handling expression-based validators.
- Decided to manage form validation manually to better control the validation logic and handle complex validation scenarios for the time being.

Screenshots

TBD

Related Issue

https://openmrs.atlassian.net/browse/O3-2906

Other

Form Engine Migration Checklist

Pending tasks

Unmasked issues/bugs in the Migration course

Previous values aren’t correctly initialised (tested with date comp, enc-datetime)
Errors and warnings aren't rendered correctly for content-switcher rendering
File picker doesn’t support multiple files selection (https://openmrs.atlassian.net/browse/O3-3682)
Encounter date time isn’t editable, and it also doesn’t seem to update the encounter’s date (https://openmrs.atlassian.net/browse/O3-3646)
The Obs comment feature wasn’t functional
Number input doesn’t render error messages when validation fails

samuelmale · 2024-07-11T14:50:59Z

package.json

    "react-markdown": "^7.1.2",
    "react-waypoint": "^10.3.0",
    "react-webcam": "^7.2.0",
-    "yup": "^1.4.0"
+    "yup": "^1.4.0",


Remove yup and formik plus other unused libraries

gracepotma · 2024-07-16T14:22:44Z

@ibacher @brandones and @denniskigen - Samuel would love your early impressions on this work to migrate the engine to RHF :)

brandones · 2024-07-23T03:08:30Z

Wow, this is a big change, and deals with a lot of stuff I haven't really worked on. Sorry I'm not really in a good position to review this.

slubwama · 2024-07-30T07:31:16Z

@samuelmale resolve conflicts

ibacher

Throwing up a few comments for you.

ibacher · 2024-08-01T16:15:14Z

src/provider/form-factory-provider.tsx

+  const rootForm = useRef<FormContextProps>();
+  const subForms = useRef<Record<string, FormContextProps>>({});


Why are we using Refs here?

The reason I used refs is that they won’t trigger a re-render on subsequent state updates. Do you want me to use the standard useState?

No, I guess since we're not sending them anywhere it's probably ok.

src/provider/form-factory-provider.tsx

ibacher · 2024-08-01T16:19:23Z

src/provider/form-factory-provider.tsx

+        setCurrentPage,
+        handleConfirmQuestionDeletion,
+      }}>
+      {formProcessors.current && children}


Since we set formProcessors() above, won't this condition always be true?

Well it appears redundant for now because I'm currently hardcoding the form processors. I left myself a TODO which suggests that the processors will be promised.

// TODO: Manage and load processors from the registry const formProcessors = useRef<Record<string, FormProcessorConstructor>>({ EncounterFormProcessor: EncounterFormProcessor, });

A slightly different way to handle it would be to put the form processors in a separate component with a separate context and just wrap the component in Suspense until they're loaded, e.g.,

<Suspense fallback={null}> <FormProcessorProvider> {children} </FormProcessorProvider> </Suspense>

ibacher · 2024-08-01T16:23:20Z

src/components/processor-factory/form-processor-factory.component.tsx

+    visit,
+  });
+  const { formFields: rawFormFields, conceptReferences } = useFormFields(formJson);
+  const { concepts: formFieldsConcepts, isLoading: isLoadingConcepts } = useConcepts(conceptReferences);


isLoading here seems to be unused?

I'd also add that this was one of the sort of requests I'd hoped we could avoid with the o3forms endpoint, but if that doesn't work for some reason, let me know.

I agree that loading all the concepts is an expensive operation, and we should rely on backend optimization. I've tried loading a form using the O3 forms endpoint, but in my experience, it doesn't seem to resolve missing labels (by inferring them from the concept).

For us to fully depend on the O3 forms endpoint, we need to add support for resolving subforms.

it doesn't seem to resolve missing labels

Ah, yes, it doesn't magically create label elements. Basically, this was so, in the NGX engine, we could override the label with translations or, absent those, the concept name, since the concept name isn't always the appropriate string. That said, the endpoint should give you an element called "conceptReferences" which has, for every concept in the form, the concept's UUID and display name.

I guess that's a good point about subforms, but you could always just request them as separate forms for now.

Since the o3forms endpoints now supports subforms and correctly queries forms by name, I'm gonna create a ticket to track the work of fully leveraging this backend optimisation.

ibacher · 2024-08-01T16:31:38Z

src/components/processor-factory/form-processor-factory.component.tsx

+  const {
+    isLoadingInitialValues,
+    initialValues,
+    error: initialValuesError,


Should this value be used for something?

ibacher · 2024-08-01T16:34:57Z

src/components/processor-factory/form-processor-factory.component.tsx

+    if (formFieldAdapters) {
+      setProcessorContext((prev) => ({
+        ...prev,
+        ...{ formFieldAdapters },
+      }));
+    }
+    if (formFieldValidators) {
+      setProcessorContext((prev) => ({
+        ...prev,
+        ...{ formFieldValidators },
+      }));
+    }
+    if (formFieldsWithMeta?.length) {
+      setProcessorContext((prev) => ({
+        ...prev,
+        ...{ formFields: formFieldsWithMeta },
+      }));
+    } else if (rawFormFields?.length) {
+      setProcessorContext((prev) => ({
+        ...prev,
+        ...{ formFields: rawFormFields },
+      }));
+    }


Maybe something like this:

Suggested change

if (formFieldAdapters) {

setProcessorContext((prev) => ({

...prev,

...{ formFieldAdapters },

}));

}

if (formFieldValidators) {

setProcessorContext((prev) => ({

...prev,

...{ formFieldValidators },

}));

}

if (formFieldsWithMeta?.length) {

setProcessorContext((prev) => ({

...prev,

...{ formFields: formFieldsWithMeta },

}));

} else if (rawFormFields?.length) {

setProcessorContext((prev) => ({

...prev,

...{ formFields: rawFormFields },

}));

}

setProcessorContext((prev) => merge({}, prev, formFieldAdapters ?? {}, formFieldValidators ? {}, formFieldsWithMeta?.length ? {formFields: formFieldsWithMeta} : {}, rawFormFields?.length ? { formFields: rawFormFields } : {}));

merge from lodash to support deep merging, but otherwise Object.assign() would be the same as this with many fewer setState() calls.

This is a good solution, but it ends up losing my object references in memory due to the deep clone. This is problematic because I would have to manually reconcile the fields in the formJson tree versus the flattened fields. This can lead to increased memory usage and potential inconsistencies, as I will end up with two separate instances for each form field.

I've ended doing something like:

setProcessorContext((prev) => ({ ...prev, ...(formFieldAdapters && { formFieldAdapters }), ...(formFieldValidators && { formFieldValidators }), ...(formFieldsWithMeta?.length ? { formFields: formFieldsWithMeta } : rawFormFields?.length ? { formFields: rawFormFields } : {}), }));

Ah! Fair enough. Still... involves fewer calls to the setProcessContext(), which is basically the whole point.

ibacher · 2024-08-01T16:39:11Z

src/provider/form-factory-provider.tsx

+  const subForms = useRef<Record<string, FormContextProps>>({});
+  const layoutType = useLayoutType();
+  const { isSubmitting, setIsSubmitting, onSubmit, onError, handleClose } = formSubmissionProps;
+  const registerForm = (formId: string, context: FormContextProps, isSubForm: boolean) => {


This should probably be wrapped in useCallback()

I also wonder about the isSubForm parameter. Shouldn't the assumption be something like the first form registered is the root form and every other form is a subform, so something like:

if (!rootForm.current) { rootForm.current = context; } else { subForm.current[formId] = context; }

ibacher · 2024-08-01T16:44:11Z

src/provider/form-factory-provider.tsx

+        getSubForms,
+        getRootForm,


Is there a use case for providing these methods to children? The only use I see of them is in the "submitting" hook and it's better if we expose as minimal an API as possible to each child.

Nice argument! Updating 🧑‍🍳

…to migration changes

github-actions · 2024-08-07T11:52:26Z

Size Change: -417 kB (-26.75%) 🎉

Total Size: 1.14 MB

Filename	Size	Change
`dist/184.js`	0 B	-11.2 kB (removed)	🏆
`dist/474.js`	0 B	-116 kB (removed)	🏆
`dist/888.js`	0 B	-259 kB (removed)	🏆
`dist/main.js`	340 kB	-17.5 kB (-4.89%)	✅

ℹ️ View Unchanged

Filename	Size	Change
`dist/151.js`	300 kB	0 B
`dist/194.js`	0 B	-9.51 kB (removed)	🏆
`dist/225.js`	2.57 kB	0 B
`dist/277.js`	1.84 kB	0 B
`dist/3.js`	480 B	0 B
`dist/300.js`	709 B	0 B
`dist/335.js`	967 B	0 B
`dist/353.js`	3.02 kB	0 B
`dist/41.js`	3.36 kB	0 B
`dist/422.js`	6.79 kB	0 B
`dist/540.js`	2.63 kB	0 B
`dist/55.js`	756 B	0 B
`dist/572.js`	251 kB	0 B
`dist/635.js`	14.3 kB	0 B
`dist/674.js`	86.4 kB	+1 B (0%)
`dist/70.js`	0 B	-482 B (removed)	🏆
`dist/733.js`	107 kB	0 B
`dist/776.js`	0 B	-3.2 kB (removed)	🏆
`dist/901.js`	11.8 kB	0 B
`dist/99.js`	690 B	0 B
`dist/993.js`	3.09 kB	0 B
`dist/openmrs-form-engine-lib.js`	3.67 kB	-51 B (-1.37%)

_{compressed-size-action}

samuelmale commented Jul 11, 2024

View reviewed changes

samuelmale closed this Jul 11, 2024

samuelmale reopened this Jul 11, 2024

samuelmale marked this pull request as draft July 11, 2024 14:52

gracepotma requested review from denniskigen, brandones and ibacher July 16, 2024 14:16

brandones removed their request for review July 23, 2024 03:08

ibacher reviewed Aug 1, 2024

View reviewed changes

samuelmale force-pushed the feat/migrateToRHF branch from f1e7c8a to 567aa8a Compare August 6, 2024 18:39

samuelmale marked this pull request as ready for review August 6, 2024 18:40

samuelmale added 16 commits August 7, 2024 12:21

Use RHF library (base commit)

c043dea

Delete 'engine' folder

7b18b23

(feat) spike on encounter submission

ec6e484

(feat) implement form-field value(s) binding

ce7da41

Flesh out input components and field value adapters (spike 1)

47a4c5b

Flesh out input components and field value adapters (spike 2)

777fa01

Handle skip logic

a564cfc

Process post submission actions and misc fixes

03c1a5a

Cleanup dependencies

34a2e90

Update date-picker

4c57891

Wrap root component with an i18n context provider

2692a8b

Remove deprecated types

5295cd0

More fixes

f99d197

Reference form components using aliases

40bf91b

Implement additional bug fixes; temporarily ignore failing tests due …

7295391

…to migration changes

Cleanup

d3e5222

samuelmale force-pushed the feat/migrateToRHF branch from eb4d069 to d3e5222 Compare August 7, 2024 11:48

ibacher approved these changes Aug 7, 2024

View reviewed changes

ibacher merged commit 1e744c8 into main Aug 7, 2024
4 checks passed

ibacher deleted the feat/migrateToRHF branch August 7, 2024 11:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(BREAKING) Migrate from Formik to RHF #349

(BREAKING) Migrate from Formik to RHF #349

samuelmale commented Jul 11, 2024 •

edited by denniskigen

Loading

samuelmale Jul 11, 2024 •

edited

Loading

gracepotma commented Jul 16, 2024

brandones commented Jul 23, 2024

slubwama commented Jul 30, 2024

ibacher left a comment

ibacher Aug 1, 2024

samuelmale Aug 1, 2024

ibacher Aug 1, 2024

ibacher Aug 1, 2024

samuelmale Aug 1, 2024

ibacher Aug 1, 2024

ibacher Aug 1, 2024

ibacher Aug 1, 2024

samuelmale Aug 1, 2024

ibacher Aug 1, 2024

samuelmale Aug 6, 2024

ibacher Aug 1, 2024

ibacher Aug 1, 2024

ibacher Aug 1, 2024

samuelmale Aug 2, 2024

ibacher Aug 2, 2024

ibacher Aug 1, 2024

ibacher Aug 1, 2024

samuelmale Aug 1, 2024

ibacher Aug 1, 2024

samuelmale Aug 1, 2024

github-actions bot commented Aug 7, 2024

		const rootForm = useRef<FormContextProps>();
		const subForms = useRef<Record<string, FormContextProps>>({});

(BREAKING) Migrate from Formik to RHF #349

(BREAKING) Migrate from Formik to RHF #349

Conversation

samuelmale commented Jul 11, 2024 • edited by denniskigen Loading

Requirements

Summary

Major Changes Made

Screenshots

Related Issue

Other

Form Engine Migration Checklist

samuelmale Jul 11, 2024 • edited Loading

Choose a reason for hiding this comment

gracepotma commented Jul 16, 2024

brandones commented Jul 23, 2024

slubwama commented Jul 30, 2024

ibacher 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

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

github-actions bot commented Aug 7, 2024

samuelmale commented Jul 11, 2024 •

edited by denniskigen

Loading

samuelmale Jul 11, 2024 •

edited

Loading