feat(useMemoCache): useMemo with cache #1063

michaltarasiuk · 2023-01-02T17:35:52Z

Hook description

Like useMemo but with cache based on dependency list.

Valid use-case for the hook

Avoid expensive callculation with the same dependencies

Checklist

Have you read the contribution guidelines?
If you are porting a hook from react-use, have you checked Port remaining hooks from react-use #33 and the migration guide
to confirm that the hook has been approved for porting?
Does the code have comments in hard-to-understand areas?
Is there an existing issue for this PR?
- link issue here
Have the files been linted and formatted?
Have the docs been updated?
Have you written tests for the new hook?
Have you run the tests locally to confirm they pass?

codecov · 2023-01-02T17:38:38Z

Codecov Report

Merging #1063 (ce8ff2f) into master (34c36f2) will decrease coverage by 0.02%.
The diff coverage is 98.11%.

@@            Coverage Diff             @@
##           master    #1063      +/-   ##
==========================================
- Coverage   98.52%   98.50%   -0.02%     
==========================================
  Files          63       65       +2     
  Lines        1082     1135      +53     
  Branches      180      190      +10     
==========================================
+ Hits         1066     1118      +52     
  Misses          2        2              
- Partials       14       15       +1

Impacted Files	Coverage Δ
src/util/areHookInputsEqual.ts	`91.66% <91.66%> (ø)`
src/index.ts	`100.00% <100.00%> (ø)`
src/useMemoCache/index.ts	`100.00% <100.00%> (ø)`

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

ArttuOll · 2023-01-02T19:35:42Z

Please use the provided PR template.

xobotyi · 2023-01-02T21:10:58Z

And from the start about implementation - this implementation has several major issues

The cache is not guarded from eating all memory - you have to limit its capacity somehow, even if it will be static value like 64 entries.
The cache is slow since it is using array as storage and it's complexity of access is O(n)
You dont provide hatches to alter dependencies comparison

ArttuOll · 2023-01-05T15:23:04Z

Thanks for the PR! I will have time to review this over the weekend. Someone might also review it before me.

For now, could you expand a little on the valid use-case for this hook? What is the benefit over useMemo? Your description doesn't tell much and the docs don't either.

ArttuOll · 2023-01-08T11:42:59Z

I will have time to review this over the weekend.

I really want to have a discussion on the use-cases of this hook before reviewing this (and before any more code is written) to make sure that no unnecessary work is done by anyone. At the moment, it is not clear to me what the use-case for this hook is.

xobotyi · 2023-01-08T12:55:37Z

The usecase is pretty simple - there are no unncessary invocations of memo factory in case provided deps are the same.

4ex. in case we had as dep value 2 then 3 then 2 again - this hook will fast-return result of invocation when dep was 2 without calling it again

so it is like good-old memoization

ArttuOll · 2023-01-08T15:04:39Z

The usecase is pretty simple - there are no unncessary invocations of memo factory in case provided deps are the same.

4ex. in case we had as dep value 2 then 3 then 2 again - this hook will fast-return result of invocation when dep was 2 without calling it again

so it is like good-old memoization

I see, thanks. So this would be good if one was doing something really heavy in the factory function.

xobotyi · 2023-01-08T16:25:24Z

@michaltarasiuk you havent resolved issues - you just accepted all your's - there were changes to import paths and output bundle - your PR now will roll back all changes we've made🙃 Reread contribution guide please and appropriately rebase your changes.

michaltarasiuk · 2023-01-08T18:57:40Z

Hello @xobotyi @ArttuOll, code does not have 100% cover because value of objectIs variable is being assigned before we import and it is not possible to mock.

ArttuOll · 2023-01-15T15:02:35Z

@michaltarasiuk Could you notify us when this is ready for review or is it already?

michaltarasiuk · 2023-01-15T17:07:17Z

@michaltarasiuk Could you notify us when this is ready for review or is it already?

Hello @xobotyi @ArttuOll, ready for code review.

ArttuOll

You've done a thorough job! I noticed some things that need to be fixed and @xobotyi might find some more. Anyhow, I'm sure this will be merged eventually.

ArttuOll · 2023-01-16T17:18:13Z

src/useMemoCache/__docs__/example.stories.tsx

+/* eslint-disable react-hooks/exhaustive-deps */
+import React, { useMemo, useRef, useState } from 'react';
+import { useMemoCache } from '../..';
+
+const dependencyListMapper = {
+ true: [1, 3],
+ false: [2, 4],
+};
+
+const getSummary = (numbers: number[]) => numbers.reduce((a, b) => a + b);
+
+const boolToString = (bool: boolean) => String(bool) as 'true' | 'false';
+
+export const Example: React.FC = () => {
+ const isTruthy = useRef(false);
+
+ const [dependencyList, setDependencyList] = useState(
+ () => dependencyListMapper[boolToString(isTruthy.current)]
+ );
+
+ const memoSpy = useRef(0);
+ const memoCacheSpy = useRef(0);
+
+ const memo = useMemo(() => {
+ memoSpy.current++;
+
+ return getSummary(dependencyList);
+ }, dependencyList);
+
+ const memoCache = useMemoCache(() => {
+ memoCacheSpy.current++;
+
+ return getSummary(dependencyList);
+ }, dependencyList);
+
+ const toggleDependencyList = () => {
+ isTruthy.current = !isTruthy.current;
+
+ setDependencyList(dependencyListMapper[boolToString(isTruthy.current)]);
+ };
+
+ return (
+ <div>
+ <section>
+ <h1>Memo</h1>
+ <p>summary: {memo}</p>
+ <p>calls: {memoSpy.current}</p>
+ </section>
+ <section>
+ <h1>Memo with cache</h1>
+ <p>summary: {memoCache}</p>
+ <p>calls: {memoCacheSpy.current}</p>
+ </section>
+ <button onClick={toggleDependencyList}>toggle dependency list</button>
+ </div>
+ );
+};


The example is good, but I think the summary value just being toggled between 4 and 6 is really confusing. I think the example needs to do something a bit more realistic to really illustrate the functionality of the hook. Maybe you could do something like let the user input a number and then count all Fibonacci numbers until that point while displaying the number of calls made to the factory functions below.

Also, make sure that the example is understandable by using the example program and reading the example code. Storybook doesn't show all code that you have in the example.stories.tsx file. For example, now getSummary and dependencyListMapper are not visible in the example code, which prevented me from understanding the example when reading through Storybook.

Lastly, getSummary should be renamed to sum, if you intend to keep it.

ArttuOll · 2023-01-16T17:18:47Z

src/useMemoCache/__docs__/story.mdx

+<Meta title="Miscellaneous/useMemoCache" component={Example} />
+
+# useMemoCache
+


Hook description missing here. You have it in other places and it's good.

ArttuOll · 2023-01-16T17:34:51Z

src/useMemoCache/__docs__/story.mdx

+function useMemoCache<State, Deps extends DependencyList>(factory: () => State, deps: Deps): State;
+```
+
+#### Importing
+
+<ImportPath />
+
+#### Arguments
+
+- **factory** _`() => unknown`_ - useMemo factory function.
+- **deps** _`DependencyList`_ - useMemo dependency list.


The function reference and the arguments description are missing the customAreHookInputsEqual argument.

ArttuOll · 2023-01-16T17:38:44Z

src/useMemoCache/__tests__/dom.ts

+ expect(spy).toHaveBeenCalledTimes(2);
+ });
+
+ it('should handle unstable refference of `areHookInputsEqual`', () => {


reference

ArttuOll · 2023-01-16T17:46:53Z

src/useMemoCache/index.ts

+import { useMemo } from 'react';
+
+import type { DependencyList } from 'react';
+import { areHookInputsEqual as nativeAreHookInputsEqual } from '../util/areHookInputsEqual';
+import { useSyncedRef } from '../useSyncedRef';
+
+// eslint-disable-next-line symbol-description
+const none = Symbol();
+
+type None = typeof none;
+type CachedItem<State> = { state: State; dependencyList: DependencyList };
+
+export const createMemoCache = <State>(
+ customAreHookInputsEqual?: typeof nativeAreHookInputsEqual
+) => {
+ const cache = new Map<string, Set<CachedItem<State>>>();
+ const areHookInputsEqual = customAreHookInputsEqual ?? nativeAreHookInputsEqual;
+
+ const get = (dependencyList: DependencyList) => {
+ const key = String(dependencyList);
+ const cached = cache.get(key);
+
+ if (!cached) {
+ return none;
+ }
+
+ const cachedItem = [...cached.values()].find((item) =>
+ areHookInputsEqual(item.dependencyList, dependencyList)
+ );
+
+ if (cachedItem) {
+ return cachedItem.state;
+ }
+
+ return none;
+ };
+
+ const set = (dependencyList: DependencyList, state: State) => {
+ const key = String(dependencyList);
+
+ const hasCachedItem = cache.has(key);
+
+ if (!hasCachedItem) {
+ cache.set(key, new Set());
+ }
+
+ const cachedItem = cache.get(key);
+ const canAddToItem =
+ cachedItem &&
+ ![...cachedItem.values()].some((item) =>
+ areHookInputsEqual(item.dependencyList, dependencyList)
+ );
+
+ if (canAddToItem) {
+ cachedItem.add({ dependencyList, state });
+ }
+ };
+
+ const isNone = (state: None | State): state is None => state === none;
+
+ return {
+ get,
+ set,
+ isNone,
+ };
+};
+
+/**
+ * Like useMemo but with cache based on dependency list.
+ */
+export const useMemoCache = <State>(
+ factory: () => State,
+ deps: DependencyList,
+ customAreHookInputsEqual?: typeof nativeAreHookInputsEqual
+): State => {
+ const syncedCustomAreHookInputsEqual = useSyncedRef(customAreHookInputsEqual);
+ const memoCache = useMemo(
+ () => createMemoCache<State>(syncedCustomAreHookInputsEqual.current),
+ [syncedCustomAreHookInputsEqual]
+ );
+
+ const memo = useMemo(() => {
+ const cachedState = memoCache.get(deps);
+
+ if (!memoCache.isNone(cachedState)) {
+ return cachedState;
+ }
+
+ const state = factory();
+
+ memoCache.set(deps, state);
+
+ return state;
+ // eslint-disable-next-line react-hooks/exhaustive-deps
+ }, deps);
+
+ return memo;
+};


I don't see anywhere the part which would prevent the cache from becoming infinitely large. As requested earlier, you should add such a limiter, which would allow, say, 64 entries, and write tests for it.

michaltarasiuk · 2023-01-17T21:52:22Z

Hello @ArttuOll @xobotyi, Code review fixes are added, but I not sure how guard from eating all memory should be implemented.

ArttuOll · 2023-01-18T18:28:27Z

Hello @ArttuOll @xobotyi, Code review fixes are added, but I not sure how guard from eating all memory should be implemented.

Looking at the code quickly in the browser, I think you can just not allow the cache (which is a Map) to have over 64 entries. You can check the cache size in set. Something like:

if (!hasCachedItem && cacheHasSpaceLeft) {
...
}

michaltarasiuk · 2023-01-19T07:43:24Z

Hello @ArttuOll @xobotyi, logic of max entries is ready 🎉.

ArttuOll · 2023-01-19T15:29:08Z

src/useMemoCache/index.ts

@@ -14,7 +14,10 @@ type CachedItem<State> = { state: State; dependencyList: DependencyList };
 export const createMemoCache = <State>(
 customAreHookInputsEqual?: typeof nativeAreHookInputsEqual
 ) => {
- const cache = new Set<CachedItem<State>>();
+ const MAX_ENTRIES = 64;
+ let indexCounter = 0;


Can't you just check cache.size to determine the current size of the cache? This counter seems unnecessary.

Hello, indexCounter is used to determine which element should be removed when threshold is hit

Oh, I see. Now I wonder if it would be better to just stop accepting new items to the cache after the maximum size is reached since the cache items are basically equally "important" so there is no reason to be deleting one item instead of another when the cache is filled.

michaltarasiuk · 2023-01-19T16:56:07Z

Changed to use Array instead of Map. The overall number of elements stays the same, but indexes now range from 0 - 63.

ArttuOll · 2023-01-19T17:00:00Z

Changed to use Array instead of Map. The overall number of elements stays the same, but indexes now range from 0 - 63.

This brings the time complexity of accessing the cache back to O(n), which is not ideal.

michaltarasiuk · 2023-01-19T17:15:51Z

Previously i haven't used advantages that Map gives. Search complexity is O(n^2), but i think it doesn't make sense to optmize since it can hold only 64 items.

michaltarasiuk · 2023-01-23T08:07:56Z

Hello @ArttuOll @xobotyi, please let me know if there is any way of use Map. I don't have any idea.

In previous approach I didn't use advantages of Map, because when function get of createCache instance was invoked the Map was converted to Array in order to iterate over values.

ArttuOll · 2023-01-23T08:22:19Z

Hello @ArttuOll @xobotyi, please let me know if there is any way of use Map. I don't have any idea.

In previous approach I didn't use advantages of Map, because when function get of createCache instance was invoked the Map was converted to Array in order to iterate over values.

At this point, we're just waiting for what @xobotyi has to say. He's been busy lately, so let's be patient!

michaltarasiuk · 2023-03-08T15:42:14Z

Hello @xobotyi, can you review pull request?

michaltarasiuk added 3 commits January 2, 2023 14:47

feat: add useMemoCache hook

9d528b2

feat: add example

c95a94c

fix: example of useMemoCache hook

5d792bd

michaltarasiuk added 13 commits January 3, 2023 08:24

fix: performence

ad7faa9

fix: cached item from Array to Set

0054efa

feat: add test cases for areHookInputsEqual

ed4d919

feat: set downlevelIteration to true

a10ecee

feat: add custom areHookInputsEqual

f5dfafa

fix: deps of cache memo

2fc981a

fix: reference of customAreHookInputsEqual

078f3a1

fix: remove export of objectIs

f0f898d

feat: resolve conflicts

b9347e0

feat: cover objectIs helper by test cases

daa1f6e

feat: cover is helper by test cases

a458b1d

fix: unstable refference of customAreHookInputsEqual

fc2dd98

fix: docs of useMemoCache

e253c3a

feat: resolve conflicts

8cf0cd1

michaltarasiuk added 2 commits January 8, 2023 17:27

feat: remove dist

afe9ffd

feat: adapt to requirements

6e6b7c9

michaltarasiuk added 2 commits January 8, 2023 23:25

feat: add assertion before add to cache

09c81e6

fix: naming

22e5240

ArttuOll suggested changes Jan 16, 2023

View reviewed changes

michalt-monogo added 3 commits January 17, 2023 21:20

fix: code review fix

82b423f

feat: useMemo to useCustomCompareMemo

dfd6913

feat: simplify cache

216a58a

michaltarasiuk requested a review from ArttuOll January 17, 2023 21:52

michalt-monogo added 2 commits January 19, 2023 00:47

feat: add max entries logic

6cda934

fix: index counter

2c82fc9

ArttuOll suggested changes Jan 19, 2023

View reviewed changes

feat: simplified cache

ce8ff2f

michaltarasiuk requested a review from ArttuOll January 19, 2023 17:21

xobotyi force-pushed the master branch from 8279633 to fe83195 Compare June 30, 2023 07:47

xobotyi force-pushed the master branch from 70c6799 to 6383cdd Compare December 20, 2023 23:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(useMemoCache): useMemo with cache #1063

feat(useMemoCache): useMemo with cache #1063

michaltarasiuk commented Jan 2, 2023 •

edited

codecov bot commented Jan 2, 2023 •

edited

ArttuOll commented Jan 2, 2023

xobotyi commented Jan 2, 2023 •

edited

ArttuOll commented Jan 5, 2023

ArttuOll commented Jan 8, 2023

xobotyi commented Jan 8, 2023

ArttuOll commented Jan 8, 2023

xobotyi commented Jan 8, 2023

michaltarasiuk commented Jan 8, 2023 •

edited

ArttuOll commented Jan 15, 2023

michaltarasiuk commented Jan 15, 2023

ArttuOll left a comment

ArttuOll Jan 16, 2023

ArttuOll Jan 16, 2023

ArttuOll Jan 16, 2023

ArttuOll Jan 16, 2023

ArttuOll Jan 16, 2023

michaltarasiuk commented Jan 17, 2023

ArttuOll commented Jan 18, 2023

michaltarasiuk commented Jan 19, 2023

ArttuOll Jan 19, 2023

michaltarasiuk Jan 19, 2023

ArttuOll Jan 19, 2023

michaltarasiuk commented Jan 19, 2023

ArttuOll commented Jan 19, 2023

michaltarasiuk commented Jan 19, 2023 •

edited

michaltarasiuk commented Jan 23, 2023 •

edited

ArttuOll commented Jan 23, 2023

michaltarasiuk commented Mar 8, 2023

		<Meta title="Miscellaneous/useMemoCache" component={Example} />

		# useMemoCache

feat(useMemoCache): useMemo with cache #1063

Are you sure you want to change the base?

feat(useMemoCache): useMemo with cache #1063

Conversation

michaltarasiuk commented Jan 2, 2023 • edited

Hook description

Valid use-case for the hook

Checklist

codecov bot commented Jan 2, 2023 • edited

Codecov Report

ArttuOll commented Jan 2, 2023

xobotyi commented Jan 2, 2023 • edited

ArttuOll commented Jan 5, 2023

ArttuOll commented Jan 8, 2023

xobotyi commented Jan 8, 2023

ArttuOll commented Jan 8, 2023

xobotyi commented Jan 8, 2023

michaltarasiuk commented Jan 8, 2023 • edited

ArttuOll commented Jan 15, 2023

michaltarasiuk commented Jan 15, 2023

ArttuOll left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaltarasiuk commented Jan 17, 2023

ArttuOll commented Jan 18, 2023

michaltarasiuk commented Jan 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaltarasiuk commented Jan 19, 2023

ArttuOll commented Jan 19, 2023

michaltarasiuk commented Jan 19, 2023 • edited

michaltarasiuk commented Jan 23, 2023 • edited

ArttuOll commented Jan 23, 2023

michaltarasiuk commented Mar 8, 2023

michaltarasiuk commented Jan 2, 2023 •

edited

codecov bot commented Jan 2, 2023 •

edited

xobotyi commented Jan 2, 2023 •

edited

michaltarasiuk commented Jan 8, 2023 •

edited

michaltarasiuk commented Jan 19, 2023 •

edited

michaltarasiuk commented Jan 23, 2023 •

edited