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.27k stars 2.71k forks source link

[HOLD for payment 2024-08-02] Moize - general purpose memoization tool #42200

Closed kacper-mikolajczak closed 3 weeks ago

kacper-mikolajczak commented 3 months ago

In order to properly evaluate if a new library can be added to package.json, please fill out this request form. It will be automatically assigned someone from our review team that will go through and vet the library.

In order to add any new production dependency, it must be approved by the App Deployer team. They will evaluate the library and decide if it's something we want to move forward with or if other alternatives should be explored.

Note: This is only for production dependencies. While we don't want people to add packages to dev-dependencies willy-nilly, we recognize that there isn't as great of a need there to secure them.

Name of library: Moize

Details

Issue OwnerCurrent Issue Owner: @puneetlath
melvin-bot[bot] commented 3 months ago

Triggered auto assignment to @puneetlath (AutoAssignerAppLibraryReview), see https://stackoverflowteams.com/c/expensify/questions/17737 for more details.

melvin-bot[bot] commented 3 months ago

New Library Review

Once these questions are answered, start a thread in #engineering-chat, ping the @app_deployers group, and call for a vote to accept the new library. Once the vote is complete, update this issue with the outcome and procede accordingly. Here is a sample post:

Hey @app_deployers,

There is a request to add a new library to App that we need to consider. Please look at this GH and then vote :+1: or :-1: on accepting this new library or not.

GH_LINK
roryabraham commented 3 months ago

After researching this, one of the things I like about moize is how the base API is extremely simple, but it's also configurable for just about any kind of memoization you might need.

puneetlath commented 3 months ago

App as of now, does not have a standardized way of handling memoization outside of React. It boils down to using custom-made solutions like Maps or serialization. This is a problem because engineers implementing and reviewing solutions for memoization outside of React have to sink time into writing and creating a custom solution that may not be consistent with other places in the codebase. This leads to inconsistency, overhead, and at times sub-optimal solutions.

Can you give some examples of such scenarios and where we encounter this problem?

roryabraham commented 3 months ago

Can you give some examples of such scenarios and where we encounter this problem?

Here are just a few examples of different caching solutions we've built ad-hoc:

A case study I was involved with can be found in this PR. The diff in src/libs/Localize/index.ts could have been reduced to this:

const translateMemoized = moize(translate);
export {translateMemoized as translate};

and lots of discussion could have been avoided with this standardization.

janicduplessis commented 3 months ago

Other case I added recently that could use this https://github.com/Expensify/App/blob/e8ae3c5acedf0e6788dc574c7b6f3043ca37092a/src/components/ThumbnailImage.tsx#L19

puneetlath commented 3 months ago

@kacper-mikolajczak can you please post this in the #expensify-open-source channel in problem/solution format along with all the information provided in the OP? And cc @app-deployers on it. Thank you!

kacper-mikolajczak commented 3 months ago

Sure thing @puneetlath! Here is a P/S post.

tgolen commented 3 months ago

Number of releases in the last year: 5

From looking at https://github.com/planttheidea/moize/releases, it looks like the last release was May 2023, last year. Correct?

puneetlath commented 3 months ago

Looks like the discussion is ongoing.

kacper-mikolajczak commented 2 months ago

Proposal

Intro

Goal of the proposal is to find API for a future memoization standard. By looking at the current use-cases in the codebase, we will try to distill minimal API surface which is going to be used for the implementation.

Examples

Lodash memoize helpers

There are two instances of lodash memoize helper in the app. Both of them are meant to be replaced with suggested solution.

The getEmojiUnicode helper has simple API with just one input, so it is going to be straightforward to replace it. No extra requirements.

https://github.com/Expensify/App/blob/92a7b1565b4e4aef0c06629a04d4715c125ed247/src/libs/EmojiUtils.ts#L69

Same goes for second use-case which is getLocaleDigits.

https://github.com/Expensify/App/blob/92a7b1565b4e4aef0c06629a04d4715c125ed247/src/libs/LocaleDigitUtils.ts#L16

Intl-like modules

Intl as a module needs to be memoized in order not to re-create new instances on every call (especially when used in a iteration with same configuration, e.g. rendering list of components).

https://github.com/Expensify/App/pull/40341

This use-case requires deep comparison of arguments, because options passed to Intl methods are possibly going to be the same across the app, but have different references.

Maps

Many samples of using Maps were provided, like ones below:

Based on those examples we would need to employ value caching mechanism (instead of function caching) which could be achieved by overloading memoize API or creating a separate like Cache.

Requirements

Here are additional concerns and requirements that were discovered either during discussion or exploration of similar solutions (moize, memoizee etc.):

Conclusion

Based on findings above and recent discussion, we can conclude that in simplest form the API could look like it:

type MapCacheConfig = {
    cacheMode: 'map';
    // Map keys are singlular values, so we cannot compare tuple of arguments directly. We are forced to serialize arguments and then compare strings.
    // It may be worth to choose if you anticipate large cache size.
    equalityCheck: 'deep';
};

type ArrayCacheConfig = {
    cacheMode: 'array';
    equalityCheck: 'shallow' | 'deep';
};

type CacheConfig = MapCacheConfig | ArrayCacheConfig;

type MemoizeConfig = {
    maxSize: number;
    maxAge: number;
    replacementPolicy: 'LRU';
} & CacheConfig;

type MemoizeStaticInstance<T> = {
    get: (key: string) => T | undefined;
    set: (key: string, value: T) => void;
    clear: () => void;
} 

type MemoizeInstance<Fn extends () => {}> = Fn & MemoizeStaticInstance<ReturnType<Fn>>;

type Memoize<Fn extends () => {}> = (f: Fn, config: MemoizeConfig) => MemoizeInstance<Fn>;
type Memoize<T> = (initialValue T[], config: MemoizeConfig) => MemoizeStaticInstance<T>;

In the next comment I will add information about possible implementation obstacles. Let me know what you think, thanks!

melvin-bot[bot] commented 2 months ago

@puneetlath Whoops! This issue is 2 days overdue. Let's get this updated quick!

mountiny commented 2 months ago

@kacper-mikolajczak What is your ETA for this POC?

kacper-mikolajczak commented 2 months ago

Hi @mountiny I have posted POC here: https://github.com/Expensify/App/pull/43538.

I will cross-post the update in the Slack thread as well - let me know what you think, thanks!

puneetlath commented 2 months ago

@kacper-mikolajczak what's the next step here?

roryabraham commented 2 months ago

@kacper-mikolajczak has a POC for a custom-build general purpose memoization tool (not moize) here. It went through some reviews but he's OOO this week so I assume it will pick back up next week

puneetlath commented 1 month ago

How's it going @kacper-mikolajczak?

mountiny commented 1 month ago

@puneetlath i believe this is now waiting for @roryabraham to check the changes made after his previous review

kacper-mikolajczak commented 1 month ago

Hi @puneetlath, exactly as @mountiny said 🫡

roryabraham commented 1 month ago

I reviewed and approved the PR last week. Looks like @kacper-mikolajczak has an updated TODO list here

mountiny commented 1 month ago

We are getting close, I think we can merge the PR tomorrow

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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

If no regressions arise, payment will be issued on 2024-08-02. :confetti_ball:

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

puneetlath commented 1 month ago

Adding @mananjadhav as it looks like they were the C+ here.

melvin-bot[bot] commented 3 weeks ago

Payment Summary

[Upwork Job]()

BugZero Checklist (@puneetlath)

puneetlath commented 3 weeks ago

Payment summary:

JmillsExpensify commented 3 weeks ago

$250 approved for @mananjadhav