[HOLD for payment 2024-01-18] [HOLD for payment 2023-10-23] Refactor Form to a custom hook

[luacmartins]

Problem

Although our Form component has been working great so far, the migration to functional components has surfaced some issues with our design. Coming from this thread, our existing Form component either causes child functional components with hooks to crash the App or functional components without hooks don't work. Given that we're migrating the whole codebase to use hooks, we need to redesign Form to play nicely with child components with/without hooks.

Why is this important

We migrated to functional components and we need Form to work with child functional components that have hooks.

Solution

Redesign Form into a custom hook. This will be worked on by an external agency.

[melvin-bot]

Triggered auto assignment to @strepanier03 (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details.

[melvin-bot]

Triggered auto assignment to Design team member for new feature review - @shawnborton (NewFeature)

[luacmartins]

No design changes are necessary here. Removing @shawnborton as an assignee

[lukemorawski]

Hi it's Lucas from Callstack. Happy to have a go on that one :) I assume, given the app is currently being migrated to TS, this new custom form hook should be written in TS too?

[luacmartins]

Thanks @lukemorawski! Assigned you to the issue. Writing it in TS sounds good.

[lukemorawski]

Cool! Thanks @luacmartins. Starting the research :)

[luacmartins]

Hey @lukemorawski, it seems like another external contributor is already working on this issue. I'll unassign you from it, sorry about the confusion.

[kowczarz]

Hello, I'm from Software Mansion, and I would like to work on this task.

[lukemorawski]

@luacmartins Cool, though it was a fun thing to work with :) Good luck!

[cubuspl42]

Hey! During solving another issue, we found a warning/error which has its root in the Form component implementation. It appears on more than one screen. Could you check after this refactor if that warning/error is gone?

[strepanier03]

As BZ, I'll follow this is being done and handle any actions or payment as needed. Feel free to ping me if I missed taking a needed action.

[kowczarz]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Form component causes the app crashing if there is any child which uses hooks API.

What is the root cause of that problem?

React.cloneElement is a legacy API and it doesn't support hooks.

What changes do you think we should make in order to solve the problem?

We can introduce the useForm hook which would contain all the form-related logic. It will return Form wrapper, and a registerInput function, that would be passed to all kinds of inputs used in NewDot. It would work in a similar way as it’s currently handled by childrenWrapperWithProps but it won’t utilise deprecated React.cloneElement. I’ve already prepared a POC so you could see how the registerInput API would look like! As the next step I want to introduce context to make the code cleaner, but since it’s a POC, I would like to know what are your general thoughts about this solution first. 😄

What alternative solutions did you explore? (Optional)

Instead of registerInput we can introduce a <Input/> component which will receive a component to render and all other props like the registerInput so the behaviour will be very similar, but it will look slightly different.

Second alternative

We can introduce a third party library like formik, or react-hook-form - both have good support and big community (over 30k stars on GitHub and couple millions of weekly downloads).

[luacmartins]

Curious for your thoughts on the proposal above @marcaaron @tgolen

[tgolen]

Looks kinda neat! Here are some of my initial thoughts:

1. Would there be a registerX method for every type of form element? I don't see how the current POC would work with other types of form elements.
2. I don't much care for the ...registerInput() style and if we could make it look more like a normal component like you suggest, I think that would look nicer and be more approachable
3. Let's definitely avoid third party libs if we can :D

[marcaaron]

Sorry just skimmed here, but the proposal is to switch to react-hook-form ?

We considered this actually. There were two downsides:

1. We didn't use hooks.
2. Registering the references was kind of confusing and complicated.

1 is no longer a problem 🎉
Is the second argument still valid?

Our existing design looks better to me. But that's just my personal preference. I am more passionate that we have consistent UX practices here. As I briefly mentioned someplace else (Slack maybe) I wouldn't stand in anyone's way if they really want to do this, but not passionate about changing this right now.

[DylanDylann]

@luacmartins Do we have the issue to migrate this one ?https://github.com/Expensify/App/blob/main/src/components/AddressForm.js

[luacmartins]

It should be this issue - #30312

[DylanDylann]

@luacmartins I just checked the PR that related to the issue you mentioned above but it does not contain the https://github.com/Expensify/App/blob/main/src/components/AddressForm.js

[luacmartins]

Ah you're right, we refactored ReimbursementAccount/AddressForm in that one, not components/AddressForm. Created a new issue for it here. Thanks for catching that.

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.24-3 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• [No QA] Delete unused old Form component #34132

If no regressions arise, payment will be issued on 2024-01-18. 🎊

For reference, here are some details about the assignees on this issue:

• @kowczarz does not require payment (Contractor)

[melvin-bot]

BugZero Checklist: The PR adding this new feature has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@kowczarz] Please propose regression test steps to ensure the new feature will work correctly on production in further releases.
• [@strepanier03] Link the GH issue for creating/updating the regression test once above steps have been agreed upon.