[HOLD for payment 2024-02-26] [TS Migration] Improve form validation return type

[roryabraham]

Coming from #32992 (review)...

Problem

Currently, the object returned by validate is just a Record<string, string>, but we know that the return type should be a map of formInputID: translationKey, so we're missing an opportunity to be precise and expressive with our type definitions. Some more specific problems with this implementation are:

• It's easy to put whatever string you want in the return type (for example, I was mistakenly using translate(translationKey), when all I needed was translationKey).
• If you use a constant for a form inputID (which is a good practice), then you'll get a type error referencing it in the return type of validate and have to cast it to a normal string (with as string).

Solution

Implement the fixes @blazejkustra laid out here.

[blazejkustra]

I would love to work on this one 😄

[roryabraham]

This is kind of blocking my work on https://github.com/Expensify/App/pull/34925/files, and despite lots of effort trying to make progress on it, I'm unfortunately kind of stuck.

I've tried something like this:

const FORM_IDS = {
    ADD_DEBIT_CARD: 'AddDebitCardForm',
} as const;

// TODO: how can we enforce that all the values of FORM_IDS are included as keys in this,
// without losing type information
const INPUT_IDS = {
    [FORM_IDS.ADD_DEBIT_CARD]: {
        SETUP_COMPLETE: 'setupComplete',
    },
} as const;

I've tried this:

const INPUT_IDS: Record<ValueOf<typeof FORM_IDS>, Record<string, string>> = {
    [FORM_IDS.ADD_DEBIT_CARD]: {
        SETUP_COMPLETE: 'setupComplete',
    },
} as const;

But that doesn't enforce that all values from FORM_IDS are included as keys in INPUT_IDS. It also makes us lose type information instead of SETUP_COMPLETE: 'setupComplete', we just have Record<string, string>.

Maybe there's a type-fest utility that can help here? Exact, perhaps? We essentially want a mapped type as a constant, but I wasn't able to figure out how to accomplish that.

Then, I got stuck on the next part as well. I was attempting to create a type from FORM_IDS and INPUT_IDS such that:

• every form has a base set of properties
• every form defines types for all the inputIDs it defined in INPUT_IDS

I tried this:

type FormData = {
    [FORM_IDS.ADD_DEBIT_CARD]: {
        // TS1170: A computed property name in a type literal must refer to an expression whose type is a literal type or a 'unique symbol' type
        [INPUT_IDS[FORM_IDS.ADD_DEBIT_CARD].SETUP_COMPLETE]: boolean,
    },
};

This doesn't work, and again we'd have a similar struggle ensuring that all forms and inputIDs are represented.

[roryabraham]

I also ran into some trouble adding RadioButtons as a ValidInput, specifically around the onPress prop. Currently onPress is defined to receive a single GestureResponderEvent argument, but in reality it can receive different treatment depending on the input that it's used with.

I think the root cause for this problem, essentially, is that BaseInputProps is incorrect, in that there's nothing to ensure that all of the inputs accept those props as they're defined.

[roryabraham]

Another thing I found a bit confusing was that FormValueType is defined loosely as string | boolean | Date, but I'd think that the value type would depend on which form is being referenced.

For example, if we're talking about the FormValueType for AddDebitCardForm, it would basically be: ValueOf<AddDebitCardForm>

[roryabraham]

Kind of dumping a lot of ideas here, but that's partially because I'm having difficulty wrapping my head around a lot of this, and just trying to make sense of it.

[blazejkustra]

I think the root cause for this problem, essentially, is that BaseInputProps is incorrect, in that there's nothing to ensure that all of the inputs accept those props as they're defined.

Yes BaseInputProps is the root issue, I'll fix it in a follow-up.

Another thing I found a bit confusing was that FormValueType is defined loosely as string | boolean | Date, but I'd think that the value type would depend on which form is being referenced.

I'll try to fix it 👍

[blazejkustra]

But that doesn't enforce that all values from FORM_IDS are included as keys in INPUT_IDS. It also makes us lose type information instead of SETUP_COMPLETE: 'setupComplete', we just have Record<string, string>.

Maybe there's a type-fest utility that can help here? Exact, perhaps? We essentially want a mapped type as a constant, but I wasn't able to figure out how to accomplish that.

When I create INPUT_IDS mapping I'll make sure it requires to provide all FORM_IDS as keys, it may be complicated but worth trying for sure 👍

[roryabraham]

Another NAB improvement that would be nice to see is if we could eliminate the need to define every form key and type twice – once for the form and again for the form draft. This is another thing I couldn't figure out how to do without losing type information. I tried something like this:

import {Replace} from 'type-fest';

const formIDs = {
    ADD_DEBIT_CARD_FORM: 'addDebitCardForm',
    WORKSPACE_SETTINGS_FORM: 'workspaceSettingsForm',
    WORKSPACE_RATE_AND_UNIT_FORM: 'workspaceRateAndUnitForm',
} as const;

type FormIDs = typeof formIDs;

type DraftFormIDs = {
    [Key in keyof FormIDs | `${keyof FormIDs}_DRAFT`]: Key extends keyof FormIDs ? FormIDs[Key] : `${FormIDs[Replace<Key, '_DRAFT', ''>]}Draft`;
};

const formIDsWithDrafts: DraftFormIDs = Object.entries(formIDs).reduce((acc, [key, value]) => {
    acc[key] = value;
    acc[`${key}_DRAFT`] = `${value}Draft`;
}, {});

But that doesn't quite work. I did find this complicated TS code that provides a better-typed version of Object.entries, but it still didn't make it clear how to accomplish what I was trying to.

[roryabraham]

Reviewed @blazejkustra's WIP PR

[blazejkustra]

PR is ready for C+ review, I tested in on all platforms and finished reviewer checklist. cc @roryabraham @fabioh8010

[abdulrahuman5196]

@roryabraham Could you kindly assign me as C+ here?

[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.42-5 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• [TS Migration] Improve Form  #35584

If no regressions arise, payment will be issued on 2024-02-26. 🎊

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

• @blazejkustra does not require payment (Contractor)

[Julesssss]

Assigning @abdulrahuman5196 based on this comment

[melvin-bot]

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

[roryabraham]

flagging as new feature to handle C+ payment

[MitchExpensify]

Reminder set to pay tomorrow

[abdulrahuman5196]

No BZ checklist since we improved the TS representation of Form component

cc: @MitchExpensify

[melvin-bot]

Payment Summary

Upwork Job

• Contributor: @blazejkustra is from an agency-contributor and not due payment
• ROLE: @abdulrahuman5196 paid $(AMOUNT) via Upwork (LINK)

BugZero Checklist (@MitchExpensify)

• I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
• I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants//hired)
• I have paid out the Upwork contracts or cancelled the ones that are incorrect
• I have verified the payment summary above is correct

[MitchExpensify]

Offer here @abdulrahuman5196 https://www.upwork.com/nx/wm/offer/101083068

[abdulrahuman5196]

@MitchExpensify accepted the offer

[MitchExpensify]

paid, contract ended!