[$250] Remove exported ReportUtils.getReport()

[tgolen]

This is coming from #27262. You can read the issue description there to get the context behind the problem being solved and the mess being cleaned up.

Problem

ReportActionUtils.getReport() is called from many view components and other action files which is an anti-pattern.

Why this is important to fix

It maintains a more pure and exact flow of data through the react application. If the view is using report action data, then it needs to subscribe to the data in Onyx so that it's guaranteed that the data will never be stale or out-of-date.

Solution

• Use withOnyx() to load the data in view components
• Use Onyx.connect() to load the data in other action files
• Remove the usage of ReportUtils.getReport() and prevent it from being exported by adding a test to https://github.com/Expensify/App/blob/main/tests/actions/EnforceActionExportRestrictions.ts

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~014f8c2e6e8d2c1390
• Upwork Job ID: 1780353316902400000
• Last Price Increase: 2024-04-16
• Automatic offers:
• s77rt | Reviewer | 0
• gijoe0295 | Contributor | 0

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~014f8c2e6e8d2c1390

[melvin-bot]

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

[melvin-bot]

Triggered auto assignment to @MitchExpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[gijoe0295]

Proposal

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

Remove exported ReportUtils.getReport()

What is the root cause of that problem?

This is a polish issue.

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

There are currently 28 places using getReport(), we should either use withOnyx (12 places) or Onyx.connect instead. Note that we should keep it inside ReportUtils file, just no longer export it. Then add a test to EnforceActionExportRestrictions to make sure we won't use it in the future.

let allReports: OnyxCollection<Report>;
Onyx.connect({
    key: ONYXKEYS.COLLECTION.REPORT,
    waitForCollectionCallback: true,
    callback: (value) => (allReports = value),
});

withOnyx(
  report: {
      key: ({chatReportID}) => `${ONYXKEYS.COLLECTION.REPORT}${chatReportID}`,
  },
)

What alternative solutions did you explore? (Optional)

NA

[nkdengineer]

Proposal

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

Remove deprecated ReportUtils.getReport() method

What is the root cause of that problem?

This is refactor issue

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

1. For component we should get report via withOnyx of if the component already subscribe the report collection, we will get the report from this

report: {
    key: ({reportID}) =>  `${ONYXKEYS.COLLECTION.REPORT}${reportID}`
}

2. For the libs action, we should use Onyx.connect to get allReports and then use this to get the report

let allReports: OnyxCollection<Report>;
Onyx.connect({
    key: ONYXKEYS.COLLECTION.REPORT,
    waitForCollectionCallback: true,
    callback: (value) => (allReports = value),
});

OPTION: We can create a local function in action file to get the report with reportID and use this in other place of this file that we want.

3. Add a test here to verify that we do not export getReport function in ReportUtils

it('does not export getReport', () => {
    // @ts-expect-error the test is asserting that it's undefined, so the TS error is normal
    expect(ReportUtils.getReport).toBeUndefined();
});

App/tests/actions/EnforceActionExportRestrictions.ts

Line 8 in d8361ee

describe('ReportUtils', () => {

What alternative solutions did you explore? (Optional)

NA

[paultsimura]

@tgolen have we already found a way to synchronize all the Onyx reads?

Using Onyx.connect sometimes leads to unexpected bugs:
https://expensify.slack.com/archives/C01GTK53T8Q/p1711017867335759
https://expensify.slack.com/archives/C01GTK53T8Q/p1709843924926719

[tgolen]

If most of the code used withOnyx() to access the report then I believe the problem goes away. Right? This would be the preferred method. I think those threads illustrate the exact problem of why getReport() is not a good pattern.

[paultsimura]

If most of the code used withOnyx() to access the report then I believe the problem goes away

I'm not quite sure how this should help. The first thread was about Onyx.connect inside OptionListUtils not being executed before OptionListUtils.getReportOption is called, therefore allReports were empty.

For the record, OptionListUtils.getReportOption was called from within a component that uses withOnyx to fetch the report.

[tgolen]

For the record, OptionListUtils.getReportOption was called from within a component that uses withOnyx to fetch the report.

Yes, this is what I mean. If the report was already fetched in the component, then the report should be passed directly to OptionListUtils.getReportOption(). It shouldn't be necessary for OptionListUtils.getReportOption() to try to access allReports to get the report and it wouldn't matter if allReports was empty or not.

Keep in mind, I am only speaking to the theory behind it. It's been a long time since I've dug into the OptionsListUtils code, so I don't currently know how possible it is to actually do this.

[paultsimura]

If the report was already fetched in the component, then the report should be passed directly to OptionListUtils.getReportOption()

Passing objects around as parameters just because we cannot reliably fetch them from any place feels like a heavier anti-pattern than the one we are trying to solve by removing ReportUtils.getReport, IMHO.

Also, what do you think about this suggestion by @hurali97?

[tgolen]

what do you think about #35234 (comment) suggestion by @hurali97?

I think it reflects what I would expect for large collections. This is why I think waitForCollectionCallback is preferred and I think a better solution than building a singleton would be to do either or both of:

• Use waitForCollectionCallback everywhere
• Improve getCollectionDataAndSendAsObject to be more performant

I'm not flatly against a singleton, but I think that is a last resort and I have always been concerned that it's the "easy solution" which covers up deeper problems. I'd rather fix the deeper problems, and only when those are completely exhausted, consider using a singleton.

[s77rt]

@tgolen

Use Onyx.connect() to load the data in other action files

Why would that be needed? Or what's the difference between calling ReportUtils.getReport (which already uses Onyx.connect) vs. calling Onyx.connect in each action file. The latter will only increase memory usage

[s77rt]

@gijoe0295 @nkdengineer Thank you for your proposals. Given this is a straightforward feature/refactor request we should go with the first proposal (@gijoe0295's proposal)

🎀 👀 🎀 C+ reviewed
Link to proposal

[melvin-bot]

Current assignee @tgolen is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[tgolen]

Before accepting any proposals... @gijoe0295 did you take a look at the references to see if it's easy (or even possible) to get the report using withOnyx() in the components that need it? This was one of the bigger concerns I had about it. Some references might be easy, but others, it might not be very straightforward how to get the report using withOnyx().

[tgolen]

The latter will only increase memory usage

Regarding memory usage, I don't think there is that big of an impact. They should all be references to the same objects, so memory is negligible. If I'm wrong about that, please profile it and show the memory problem exists and is significant enough to be concerned about.

what's the difference between calling ReportUtils.getReport (which already uses Onyx.connect) vs. calling Onyx.connect in each action file.

You're right that there is no technical difference between the two. The difference for me is a code quality and maintenance issue. As soon as ReportUtils.getReport() is exported, even if you and I agree it should only be used in other action files, there are going to be 10 random engineers that see the exported function and use it in front-end view components. If you know of a different way of solving for this problem, I am open to suggestions. I don't mind if library files share these utility lookup methods, but I feel VERY STRONGLY that view components should not be using them.

[gijoe0295]

@tgolen I investigated and verified that we can use withOnyx for all existing components. Some places might use conditions or computed values to get the report but those conditions/values can be evaluated inside withOnyx's key callback.

We can optionally migrate to useOnyx that was just implemented in #34339 and recommended to use in this thread although it is new and not time-proven. Wdyt?

[tgolen]

Great, thank you for confirming that. Using useOnyx() is fine as well 👍

[melvin-bot]

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

Offer link
Upwork job

[melvin-bot]

📣 @gijoe0295 🎉 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 📖

[melvin-bot]

This issue has not been updated in over 15 days. @tgolen, @s77rt, @MitchExpensify, @gijoe0295 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!

[s77rt]

Just bumped the PR