search

Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.36k stars 2.78k forks source link

Remove ReportActionsUtils.getParentReportAction(), ReportUtils.getParentReport(), TransactionUtils.getLinkedTransaction(), ReportUtils.getPolicy(), and TransactionUtils.getTransaction() #27262

Closed tgolen closed 2 months ago

tgolen commented 1 year ago

Problem

These methods are anti-patterns because they are most always used for loading data into a component without using withOnyx(). This breaks the data flow of a react application. (data is coming from somewhere that is not props or state and cannot be debugged in react dev tools).

Solution

Switch all references to properly use withOnyx() for components and connect() for libs.

Components

techievivek commented 1 year ago

Adding @aimane-chnaif to the GH as he helped review the PR: https://expensify.slack.com/archives/C02NK2DQWUX/p1694749699352029

tgolen commented 11 months ago

I'm slowly working my way through these, but I wasn't able to get to any of them this week.

tgolen commented 11 months ago

I wrote a new PR for the next one on the list today.

tgolen commented 10 months ago

I haven't had time to do any additional work on this.

bernhardoj commented 10 months ago

Coming from https://expensify.slack.com/archives/C01GTK53T8Q/p1688630003862989, here are some util functions that currently depend on Onyx.connect which I think can be replaced with withOnyx.

CardUtils isExpensifyCard, isCorporateCard, getCardDescription
CurrencyUtils getCurrenyDecimals. getCurrencySymbol, isValidCurrencyCode
GroupChatUtils getGroupChatName
LocalePhoneNumber formatPhoneNumber
LoginUtils appendCountryCode
PersonalDetailsUtils getAccountIDsByLogins, getLoginsByAccountIDs
ReportActionsUtils getLastVisibleAction, getReportAction, getReportPreviewAction, getAllReportActions, findPreviousAction
TransactionUtils getAllReportTransactions
PersonalDetails getDisplayName, getDisplayNameForTypingIndicator
ReimbursementAccount/store.js hasCreditBankAccount
tgolen commented 10 months ago

Thanks @bernhardoj. What do you think of this approach?

  1. Create a PR that marks all those methods as deprecated (same as I did for getParentReportAction() and the others)
  2. Start tackling these one-by-one to replace each one
bernhardoj commented 10 months ago

@tgolen oh, my bad, I didn't mean to completely drop the whole function.

What I mean is more like replacing the source of the onyx data from Onyx.connect to withOnyx, for example, getGroupChatName.

getGroupChatName depends on ONYXKEYS.PERSONAL_DETAILS_LIST. Currently, we get the onyx data with Onyx.connect which could break the reactive behavior.

https://github.com/Expensify/App/blob/d7366c6cb1c1989858006f3c9a0cb80d8f5ee6f7/src/libs/GroupChatUtils.ts#L7-L19

To fix it, we should pass the personal details onyx from the component that uses it.

function getGroupChatName(report, personalDetails) {}

getGroupChatName(report, personalDetails)
withOnyx({
    personalDetails: {key: ONYXKEYS.PERSONAL_DETAILS_LIST}
})

For the plan, I think we can immediately start tackling each function. But there are some function that has a lot of usage which I think can be split into a few parts.

tgolen commented 10 months ago

OK, yeah, I know what you mean now. @marcaaron and I have gone back and forth on this topic, and generally we've settled in a place where if there is no problem, then we have left it alone. However, I think it's a safer environment in general if data is usually fetched from withOnyx and then passed to action files. There are some performance optimizations that can be gained by having less things in withOnyx though, so there will always be some kind of balance we have to determine. For now, let's try to wrap up the current methods that I've marked as deprecated, and then we can tackle these one-by-one and figure out what to do with them (if anything).

bernhardoj commented 10 months ago

Sounds good 👍

melvin-bot[bot] commented 9 months ago

This issue has not been updated in over 15 days. @tgolen eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

tgolen commented 8 months ago

I've got PRs for the few remaining components 🎉

melvin-bot[bot] commented 7 months ago

This issue has not been updated in over 15 days. @tgolen eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

melvin-bot[bot] commented 7 months ago

Job added to Upwork: https://www.upwork.com/jobs/~0156f7187832f56740

melvin-bot[bot] commented 7 months ago

Triggered auto assignment to Contributor Plus for review of internal employee PR - @jjcoffee (Internal)

tgolen commented 7 months ago

@jjcoffee could you please do a C+ review on https://github.com/Expensify/App/pull/37340?

jjcoffee commented 7 months ago

@tgolen Sure, will get to that today!

tgolen commented 6 months ago

Weekly Update

Next Steps

ETA

tgolen commented 6 months ago

Weekly Update

Next Steps

ETA

tgolen commented 6 months ago

Weekly Update

Next Steps

ETA

tgolen commented 5 months ago

Weekly Update

Next Steps

ETA

tgolen commented 4 months ago

Weekly Update

Next Steps

ETA

tgolen commented 4 months ago

Weekly Update

tgolen commented 4 months ago

Weekly Update

melvin-bot[bot] commented 3 months ago

This issue has not been updated in over 15 days. @tgolen eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

tgolen commented 2 months ago

ReportUtils is the only remaining item, but I took a look at it and I think it's mostly fine for now so I am going to close this out.