[250] Use general-purpose memoization lib in Localize

[roryabraham]

This is essentially a follow-up for https://expensify.slack.com/archives/C01GTK53T8Q/p1716371827915779. Just a bit of cleanup.

Problem

We have built a general-purpose memoziation lib and standardized on using it as a best practice for consistency. However, in src/libs/Localize we still have a different custom memoization/cache implementation.

Solution

Refactor src/libs/Localize to use the general-purpose memoization lib

Issue Owner

Current Issue Owner: @abekkala

[melvin-bot[bot]]

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

[roryabraham]

cc @kacper-mikolajczak

[melvin-bot[bot]]

Triggered auto assignment to @abekkala (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.

[shijir0927]

Proposal

---

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

utils/Localize still uses it's own memoization implementation using translationCache function which is not in alignment with the best practice since all/most? other places are using utils/memoize

What is the root cause of that problem?

Just a refactor that was left behind and needs to be done

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

Since utils/memoize is just wrapping a function with memoization layer and returning the memoized value,

1. In utils/Localize getTranslatedPhrase() function, we can get rid of the logic that is manually setting and checking the phrase in a map which makes the function translationCache() obselete. https://github.com/Expensify/App/blob/821edc51788b616e323cae0dc00f4b0aa7246766/src/libs/Localize/index.ts#L89C1-L99C6

2. Use the new memoize utility function wrapping the getTranslatedPhrase() value https://github.com/Expensify/App/blob/821edc51788b616e323cae0dc00f4b0aa7246766/src/libs/Localize/index.ts#L171C1-L177C103

I can make the the refactor here if you want to make sure it works and won't create any unwanted behavior. I'll also backfill a unit test in LocalizeTests.ts for checking the memoization actually works unless you think that's extra.

What alternative solutions did you explore? (Optional)

Since we already have a utility function that we specifically want to use, no alternative solution is explored.

[melvin-bot[bot]]

📣 @shijir0927! 📣 Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

1. Make sure you've read and understood the contributing guidelines.
2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[shijir0927]

Contributor details Your Expensify account email: [email protected] Upwork Profile Link: https://www.upwork.com/freelancers/~0190ddfecd8c405330

[melvin-bot[bot]]

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

[shubham1206agra]

Proposal

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

Use general-purpose memoization lib in Localize

What is the root cause of that problem?

Feature Request

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

We need to cache getTranslatedPhrase using memoize present under @libs/memoize module.

We will use the following code to create memorized function.

const memoizedGetTranslatedPhrase = memoize(getTranslatedPhrase, {
    monitoringName: 'getTranslatedPhrase',
    transformKey: ([language, phraseKey, fallbackLanguage, ...parameters]) => `${language}-${phraseKey}-${fallbackLanguage}-${parameters.length > 0 ? JSON.stringify(parameters.at(0)) : ''}`,
});

And use it in https://github.com/Expensify/App/blob/821edc51788b616e323cae0dc00f4b0aa7246766/src/libs/Localize/index.ts#L177

And remove the old memoize code present https://github.com/Expensify/App/blob/821edc51788b616e323cae0dc00f4b0aa7246766/src/libs/Localize/index.ts#L63

Test branch. - https://github.com/shubham1206agra/App/tree/test-localize-memoize

What alternative solutions did you explore? (Optional)

[hoangzinh]

@roryabraham just wants to confirm if @kacper-mikolajczak will work on this issue or it's opened for external contributors?

[mountiny]

@hoangzinh this is open to proposals. @kacper-mikolajczak will help to review too

[melvin-bot[bot]]

Current assignee @mountiny is eligible for the AutoAssignerNewDotQuality assigner, not assigning anyone new.

[kacper-mikolajczak]

Hi 👋 I am Kacper from Callstack and I would like to participate in the issue!

[hoangzinh]

Thanks for the proposals, everyone. Although @shijir0927 is first, @shubham1206agra provided a better solution. He mentioned the transformKey param, which helps us better determine our cache key. Therefore I think we can go with @shubham1206agra's proposal.

Link to proposal https://github.com/Expensify/App/issues/50194#issuecomment-2392702540

🎀👀🎀 C+ reviewed

[melvin-bot[bot]]

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

[kacper-mikolajczak]

@shubham1206agra Thanks for your proposal!

Here is a link to discussion where we talked about transformKey for this specific use-case - it may come in handy for the implementation 👍

Also, when working on this, if you could share your perception on recently added descriptions for memoize module.

Thanks! ❤️

P.S. In this particular case, the monitoringName could be omitted as the function definition has the same name.

[melvin-bot[bot]]

The solution for this issue has been :rocket: deployed to production :rocket: in version 9.0.45-4 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

• https://github.com/Expensify/App/pull/50231

If no regressions arise, payment will be issued on 2024-10-14. :confetti_ball:

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

• @hoangzinh requires payment (Needs manual offer from BZ)
• @shubham1206agra requires payment (Needs manual offer from BZ)
• @kacper-mikolajczak does not require payment (Contractor)

[melvin-bot[bot]]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [ ] [@hoangzinh / @shubham1206agra] The PR that introduced the bug has been identified. Link to the PR:
• [ ] [@hoangzinh / @shubham1206agra] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [ ] [@hoangzinh / @shubham1206agra] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [ ] [@hoangzinh / @shubham1206agra] Determine if we should create a regression test for this bug.
• [ ] [@hoangzinh / @shubham1206agra] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [ ] [@abekkala] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[hoangzinh]

The automation above is incorrect. PR is still work in progress

[abekkala]

@hoangzinh so the PR was not deployed to production yesterday?

[hoangzinh]

yes @abekkala. It's @kacper-mikolajczak's PR to add some description for Memoize module. It's not a fixing PR of this issue. @shubham1206agra is working on it.

[mountiny]

@shubham1206agra what is your ETA for the PR?

[shubham1206agra]

This is being discussed with @kacper-mikolajczak right now.

[abekkala]

I'm going ooo until Oct 20. Reassigning.

STATUS: waiting on PR for this fix. After deploy to prod and no regressions $250 payment can be sent to the PR fixer and the PR Reviewer.

cc: @OfstadC

[shubham1206agra]

The direction we are going is that I will introduce a new option that will skip cache check and run the function for a given parameter.

[abekkala]

I'm back from ooo - unassigning @OfstadC

[melvin-bot[bot]]

@hoangzinh, @abekkala, @mountiny, @shubham1206agra, @kacper-mikolajczak Whoops! This issue is 2 days overdue. Let's get this updated quick!

[melvin-bot[bot]]

@hoangzinh, @abekkala, @mountiny, @shubham1206agra, @kacper-mikolajczak Eep! 4 days overdue now. Issues have feelings too...

[mountiny]

PR merged

[melvin-bot[bot]]

@hoangzinh, @abekkala, @mountiny, @shubham1206agra, @kacper-mikolajczak 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

[shubham1206agra]

PR is merged