-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[HOLD for payment 2024-02-26] [TS Migration] Improve form validation return type #35318
Comments
I would love to work on this one 😄 |
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 Maybe there's a Then, I got stuck on the next part as well. I was attempting to create a type from
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. |
I also ran into some trouble adding I think the root cause for this problem, essentially, is that |
Another thing I found a bit confusing was that For example, if we're talking about the |
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. |
Yes BaseInputProps is the root issue, I'll fix it in a follow-up.
I'll try to fix it 👍 |
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 👍 |
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 |
Reviewed @blazejkustra's WIP PR |
PR is ready for C+ review, I tested in on all platforms and finished reviewer checklist. cc @roryabraham @fabioh8010 |
@roryabraham Could you kindly assign me as C+ here? |
|
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: If no regressions arise, payment will be issued on 2024-02-26. 🎊 For reference, here are some details about the assignees on this issue:
|
Assigning @abdulrahuman5196 based on this comment |
Triggered auto assignment to @MitchExpensify ( |
flagging as new feature to handle C+ payment |
Reminder set to pay tomorrow |
No BZ checklist since we improved the TS representation of Form component cc: @MitchExpensify |
Payment Summary
BugZero Checklist (@MitchExpensify)
|
@MitchExpensify accepted the offer |
paid, contract ended! |
Coming from #32992 (review)...
Problem
Currently, the object returned by
validate
is just aRecord<string, string>
, but we know that the return type should be a map offormInputID: translationKey
, so we're missing an opportunity to be precise and expressive with our type definitions. Some more specific problems with this implementation are:translate(translationKey)
, when all I needed wastranslationKey
).as string
).Solution
Implement the fixes @blazejkustra laid out here.
The text was updated successfully, but these errors were encountered: