[HOLD for payment 2024-11-11] [250] Use general-purpose memoization lib in Localize

@abekkala

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

@hoangzinh

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

@kacper-mikolajczak

cc @kacper-mikolajczak

@abekkala

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.

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,

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
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.

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

App/src/libs/Localize/index.ts

Line 177 in 821edc5

    
           const translatedPhrase = getTranslatedPhrase(language, path, languageAbbreviation, ...parameters);

And remove the old memoize code present

App/src/libs/Localize/index.ts

Line 63 in 821edc5

    
           const translationCache = new Map<ValueOf<typeof CONST.LOCALES>, Map<TranslationPaths, string>>(

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

What alternative solutions did you explore? (Optional)

@roryabraham

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

Reviewing label has been removed, please complete the "BugZero Checklist".

@hoangzinh

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

Localize memoize #50698

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

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

@hoangzinh requires payment through NewDot Manual Requests
@shubham1206agra requires payment (Needs manual offer from BZ)
@kacper-mikolajczak does not require payment (Contractor)

@hoangzinh

@hoangzinh / @shubham1206agra @abekkala The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]

@shubham1206agra

PAYMENT SUMMARY FOR NOV 11

Fix: @shubham1206agra [$250, if no regressions] OFFER
PR Review: @hoangzinh [$250, if no regressions] Payment via NewDot
please post checklist

BugZero Checklist:

[Contributor] Classify the bug: It's not a bug issue, it's a refactoring issue

Bug classification

Source of bug:

1a. Result of the original design (eg. a case wasn't considered)
1b. Mistake during implementation
1c. Backend bug
[x ] 1z. Other:

Where bug was reported:

2a. Reported on production
2b. Reported on staging (deploy blocker)
2c. Reported on a PR
2z. Other:

Who reported the bug:

[Contributor] 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: N/A
[Contributor] If the regression was CRITICAL (e.g. interrupts a core flow) A discussion in #expensify-open-source 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: N/A
[Contributor] If it was decided to create a regression test for the bug, please propose the regression test steps using the template below to ensure the same bug will not reach production again.
No, we don't need a regression test for this issue. It's already covered somewhere

@hoangzinh

Payment Summary

Upwork Job

Reviewer: @hoangzinh owed $250 via NewDot
ROLE: @shubham1206agra paid $(AMOUNT) via Upwork (LINK)
Contributor: @kacper-mikolajczak is from an agency-contributor and not due payment

BugZero Checklist (@abekkala)

I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
[HOLD for payment 2024-11-11] [250] Use general-purpose memoization lib in Localize #50194 (comment)
I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants//hired)
I have paid out the Upwork contracts or cancelled the ones that are incorrect
I have verified the payment summary above is correct

@shubham1206agra

@shubham1206agra can you accept the offer in Upwork? I can then process payment today, thanks!

@abekkala

@abekkala Done

@shubham1206agra

@shubham1206agra payment sent and contract ended - thank you! 🎉

@shubham1206agra

PAYMENT SUMMARY FOR NOV 11

Fix: @shubham1206agra [$250] [PAID VIA UPWORK]
PR Review: @hoangzinh [$250] Payment via NewDot

@hoangzinh

$250 approved for @hoangzinh

Navigation Menu

Repository navigation

[HOLD for payment 2024-11-11] [250] Use general-purpose memoization lib in Localize #50194

Problem

Solution

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

46 remaining items

PAYMENT SUMMARY FOR NOV 11

BugZero Checklist:

Payment Summary

BugZero Checklist (@abekkala)

PAYMENT SUMMARY FOR NOV 11

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

[HOLD for payment 2024-11-11] [250] Use general-purpose memoization lib in Localize #50194

Description

Problem

Solution

Activity

melvin-bot commented on Oct 3, 2024

roryabraham commented on Oct 3, 2024

melvin-bot commented on Oct 3, 2024

shijir0927 commented on Oct 4, 2024

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

melvin-bot commented on Oct 4, 2024

shijir0927 commented on Oct 4, 2024

melvin-bot commented on Oct 4, 2024

shubham1206agra commented on Oct 4, 2024

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

hoangzinh commented on Oct 4, 2024

46 remaining items

melvin-bot commented on Nov 4, 2024

melvin-bot commented on Nov 4, 2024

melvin-bot commented on Nov 4, 2024

abekkala commented on Nov 5, 2024

PAYMENT SUMMARY FOR NOV 11

hoangzinh commented on Nov 6, 2024

BugZero Checklist:

melvin-bot commented on Nov 11, 2024

Payment Summary

BugZero Checklist (@abekkala)

abekkala commented on Nov 11, 2024

shubham1206agra commented on Nov 11, 2024

abekkala commented on Nov 11, 2024

abekkala commented on Nov 11, 2024

PAYMENT SUMMARY FOR NOV 11

garrettmknight commented on Nov 13, 2024

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions