[HOLD for payment 2024-07-10] [$250] [TS Migration] Investigate and remove usages of EmptyObject

[mountiny]

Follow up issue for TS migration project. Coming from this spreadsheet

Investigate and remove usages of EmptyObject across libs and components (it will be easier now when all files are TS)

Any more details for this one cc @blazejkustra @fabioh8010

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~019e9ec8ddc48ac6c7
• Upwork Job ID: 1780333615910244352
• Last Price Increase: 2024-04-23
• Automatic offers:
• rayane-djouah | Reviewer | 0
• jsdev2547 | Contributor | 0
• tienifr | Contributor | 0
• mkhutornyi | Contributor | 102856727

Issue Owner

Current Issue Owner: @joekaufmanexpensify

[blazejkustra]

Details:
This is a bigger task that requires a lot of changes in the logic, basically we need to search for all EmptyObject usages in the code and try to remove them, also we should remove some of isEmptyObject usages and {} default values to improve the logic further more.

⚠️ A deep understanding of Typescript and how onyx types work is required from anyone taking this one.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~019e9ec8ddc48ac6c7

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @rayane-djouah (External)

[melvin-bot]

Triggered auto assignment to @joekaufmanexpensify (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details. Please add this Feature request to a GH project, as outlined in the SO.

[ghost]

@mountiny I am available for the ts migration.

[ghost]

Proposal

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

Remove the usage of EmptyObject

What is the root cause of that problem?

TS Migration

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

src/components
20 components

src/libs

1. getReportPolicyID.ts
2. NextStepUtils.ts
3. OptionsListUtils.ts
4. PersonalDetailsUtils.ts
5. PolicyUtils.ts
6. ReportActionsUtils.ts
7. ReportUtils.ts
8. TransactionUtils.ts

src/libs/actions

1. IOU.ts
2. OnyxUpdates.ts
3. Policy.ts
4. Report.ts
5. Task.ts
6. User.ts
7. Welcome.ts

What alternative solutions did you explore? (Optional)

[joekaufmanexpensify]

I think CS might be doing all of these migration issues, but not 100% sure.

[mountiny]

No, this one is supposed to be external, looking for proposals

also we should remove some of isEmptyObject usages and {} default values to improve the logic further more.

@jsdev2547 can you give examples of how you would approach this int he codebase?

[ghost]

@mountiny

also we should remove some of isEmptyObject usages and {} default values to improve the logic further more.

Instead of isEmptyObject lodash/isEmpty can be used. lodash/isEmpty is used currently by BaseSelectionList.tsx, FormHelpMessage.tsx. lodash/isEmpty can also handle null values.

EmptyObject is defined in src/types/utils/EmptyObject.ts. Remove type EmptyObject and EmptyValue. The isEmptyObject function can be wrapped around lodash/isEmpty and then isEmptyObject is already used in various components, libs and actions.

import isEmpty from 'lodash/isEmpty';
function isEmptyObject<T>(obj: T) {
    return isEmpty(obj);
}
export {isEmptyObject};

Then go one by one in each component, libs and actions.

EmptyObject is used in function parameter types and method parameters. The EmptyObject can be removed eg

1. Function parameter types:
   i.e. AttachmentModalProps
   report?: OnyxEntry<OnyxTypes.Report> | EmptyObject;

2. Methods:
   function getReport(reportID: string | undefined): OnyxEntry | EmptyObject {

3. Sometimes the code has early return on null or undefined values,

function getReport(reportID: string | undefined): OnyxEntry<Report> | EmptyObject {
    if (!allReports) {
        return {};
    }
    return allReports?.[`${ONYXKEYS.COLLECTION.REPORT}${reportID}`] ?? {};
}

EmptyObject can be removed. The lodash/isEmpty can handle null values, so possible approach could be to return null and then if needed on the caller side of getReport call isEmpty to check if object is empty.

function getReport(reportID: string | undefined): OnyxEntry<Report> {
    if (!allReports) {
        return null;
    }
    return allReports?.[`${ONYXKEYS.COLLECTION.REPORT}${reportID}`];
}

For Onyx Api types, src/libs/api/types.ts
In Onyx Types, the parameter is defined as EmptyObject e.g

[READ_COMMANDS.OPEN_INITIAL_SETTINGS_PAGE]: EmptyObject;
[READ_COMMANDS.OPEN_INITIAL_SETTINGS_PAGE]: null;

Instead of EmptyObject use null and then if needed lodash/isEmpty can be used to check for empty objects. The onyx types can use null values (tested by using null) and does not throw any errors.

[rayane-djouah]

I will be OOO until june 12th, feel free to reassign another C+ to review the PR if you prefer.

[mkhutornyi]

Let me take over as assigned in PR which is in review

[joekaufmanexpensify]

PR still in review

[joekaufmanexpensify]

Same

[rayane-djouah]

unassigning myself as @mkhutornyi took over. I'm back if you need my help here @mkhutornyi.

[joekaufmanexpensify]

Sounds good. TY!

[melvin-bot]

📣 @mkhutornyi 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[joekaufmanexpensify]

PR on staging

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

• [TS Migration] Remove EmptyObject type #42851

If no regressions arise, payment will be issued on 2024-07-10. 🎊

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

• @mkhutornyi requires payment automatic offer (Contributor)
• @tienifr requires payment automatic offer (Contributor)

[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:

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

[joekaufmanexpensify]

No checklist needed here for TS migration

[joekaufmanexpensify]

Payment is a bit complex here, as there were two contributors and C+ at various points on this issue. Discussing here.

[joekaufmanexpensify]

We discussed in Slack, and decided to split payment here four-ways between both contributors and both C+, $125 to everyone. This would mean:

• $125 to jsdev2547 via Upwork for creating their proposal and most of a PR (before they were removed).
• $125 to @tienifr via Upwork for picking the issue up, creating a new PR and getting it over the line.
• $125 to @rayane-djouah via Upwork for C+ role of reviewing proposals and reviewing first PR.
• $125 to @mkhutornyi via Upwork for C+, picking up second PR review and getting it over the line.

LMK if any Q's on that!

[joekaufmanexpensify]

Issuing payment now!

[joekaufmanexpensify]

All payment issued and contracts closed.

[joekaufmanexpensify]

Upwork job closed.

[joekaufmanexpensify]

All set, thanks everyone!