From 5d16bb5d2dfe7653f46cff7ca3eb3dc70d7d9594 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A1bio=20Henriques?= Date: Mon, 25 Nov 2024 16:02:55 +0000 Subject: [PATCH 1/3] useEffect approach for dependency list in useOnyx --- lib/useOnyx.ts | 28 +++++++++++++++++++++- tests/unit/useOnyxTest.ts | 49 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 76 insertions(+), 1 deletion(-) diff --git a/lib/useOnyx.ts b/lib/useOnyx.ts index fa460dd7..dbd72633 100644 --- a/lib/useOnyx.ts +++ b/lib/useOnyx.ts @@ -1,5 +1,6 @@ import {deepEqual, shallowEqual} from 'fast-equals'; import {useCallback, useEffect, useRef, useSyncExternalStore} from 'react'; +import type {DependencyList} from 'react'; import OnyxCache, {TASK} from './OnyxCache'; import type {Connection} from './OnyxConnectionManager'; import connectionManager from './OnyxConnectionManager'; @@ -104,12 +105,18 @@ function getCachedValue(key: TKey, selector?: UseO function useOnyx>( key: TKey, options?: BaseUseOnyxOptions & UseOnyxInitialValueOption & Required>, + dependencies?: DependencyList, ): UseOnyxResult; function useOnyx>( key: TKey, options?: BaseUseOnyxOptions & UseOnyxInitialValueOption>, + dependencies?: DependencyList, ): UseOnyxResult; -function useOnyx>(key: TKey, options?: UseOnyxOptions): UseOnyxResult { +function useOnyx>( + key: TKey, + options?: UseOnyxOptions, + dependencies: DependencyList = [], +): UseOnyxResult { const connectionRef = useRef(null); const previousKey = usePrevious(key); @@ -137,6 +144,10 @@ function useOnyx>(key: TKey // in `getSnapshot()` to be satisfied several times. const isFirstConnectionRef = useRef(true); + const isConnectingRef = useRef(false); + + const onStoreChangeFnRef = useRef<(() => void) | null>(null); + // Indicates if we should get the newest cached value from Onyx during `getSnapshot()` execution. const shouldGetCachedValueRef = useRef(true); @@ -168,6 +179,14 @@ function useOnyx>(key: TKey ); }, [previousKey, key]); + // eslint-disable-next-line rulesdir/prefer-early-return + useEffect(() => { + if (connectionRef.current !== null && !isConnectingRef.current && onStoreChangeFnRef.current) { + shouldGetCachedValueRef.current = true; + onStoreChangeFnRef.current(); + } + }, [...dependencies]); + // Mimics withOnyx's checkEvictableKeys() behavior. const checkEvictableKey = useCallback(() => { if (options?.canEvict === undefined || !connectionRef.current) { @@ -255,9 +274,14 @@ function useOnyx>(key: TKey const subscribe = useCallback( (onStoreChange: () => void) => { + isConnectingRef.current = true; + onStoreChangeFnRef.current = onStoreChange; + connectionRef.current = connectionManager.connect({ key, callback: () => { + isConnectingRef.current = false; + // Signals that the first connection was made, so some logics in `getSnapshot()` // won't be executed anymore. isFirstConnectionRef.current = false; @@ -281,6 +305,8 @@ function useOnyx>(key: TKey } connectionManager.disconnect(connectionRef.current); + onStoreChangeFnRef.current = null; + isConnectingRef.current = false; isFirstConnectionRef.current = false; }; }, diff --git a/tests/unit/useOnyxTest.ts b/tests/unit/useOnyxTest.ts index 6e74ee4a..ebc975b8 100644 --- a/tests/unit/useOnyxTest.ts +++ b/tests/unit/useOnyxTest.ts @@ -610,6 +610,55 @@ describe('useOnyx', () => { }); }); + describe('dependencies', () => { + it('should 1', async () => { + Onyx.mergeCollection(ONYXKEYS.COLLECTION.TEST_KEY, { + [`${ONYXKEYS.COLLECTION.TEST_KEY}entry1`]: {id: 'entry1_id', name: 'entry1_name'}, + [`${ONYXKEYS.COLLECTION.TEST_KEY}entry2`]: {id: 'entry2_id', name: 'entry2_name'}, + [`${ONYXKEYS.COLLECTION.TEST_KEY}entry3`]: {id: 'entry3_id', name: 'entry3_name'}, + } as GenericCollection); + + let externalValue = 'ex1'; + + const {result, rerender} = renderHook(() => + useOnyx( + ONYXKEYS.COLLECTION.TEST_KEY, + { + // @ts-expect-error bypass + selector: (entries: OnyxCollection<{id: string; name: string}>) => + Object.entries(entries ?? {}).reduce>>((acc, [key, value]) => { + acc[key] = `${value?.id}_${externalValue}`; + return acc; + }, {}), + }, + [externalValue], + ), + ); + + await act(async () => waitForPromisesToResolve()); + + expect(result.current[0]).toEqual({ + [`${ONYXKEYS.COLLECTION.TEST_KEY}entry1`]: 'entry1_id_ex1', + [`${ONYXKEYS.COLLECTION.TEST_KEY}entry2`]: 'entry2_id_ex1', + [`${ONYXKEYS.COLLECTION.TEST_KEY}entry3`]: 'entry3_id_ex1', + }); + expect(result.current[1].status).toEqual('loaded'); + + externalValue = 'ex2'; + + await act(async () => { + rerender(undefined); + }); + + expect(result.current[0]).toEqual({ + [`${ONYXKEYS.COLLECTION.TEST_KEY}entry1`]: 'entry1_id_ex2', + [`${ONYXKEYS.COLLECTION.TEST_KEY}entry2`]: 'entry2_id_ex2', + [`${ONYXKEYS.COLLECTION.TEST_KEY}entry3`]: 'entry3_id_ex2', + }); + expect(result.current[1].status).toEqual('loaded'); + }); + }); + // This test suite must be the last one to avoid problems when running the other tests here. describe('canEvict', () => { const error = (key: string) => `canEvict can't be used on key '${key}'. This key must explicitly be flagged as safe for removal by adding it to Onyx.init({safeEvictionKeys: []}).`; From 7c02929f55055a305e06b131ca8cb51a50d13577 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A1bio=20Henriques?= Date: Wed, 4 Dec 2024 22:31:11 +0000 Subject: [PATCH 2/3] Add comments --- lib/useOnyx.ts | 15 +++++++++++---- tests/unit/useOnyxTest.ts | 2 +- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/lib/useOnyx.ts b/lib/useOnyx.ts index dbd72633..cad6f53d 100644 --- a/lib/useOnyx.ts +++ b/lib/useOnyx.ts @@ -144,8 +144,10 @@ function useOnyx>( // in `getSnapshot()` to be satisfied several times. const isFirstConnectionRef = useRef(true); + // Indicates if the hook is connecting to a Onyx key. const isConnectingRef = useRef(false); + // Stores the `onStoreChange` function, which can be used to trigger a `getSnapshot` update when desired. const onStoreChangeFnRef = useRef<(() => void) | null>(null); // Indicates if we should get the newest cached value from Onyx during `getSnapshot()` execution. @@ -179,12 +181,17 @@ function useOnyx>( ); }, [previousKey, key]); - // eslint-disable-next-line rulesdir/prefer-early-return useEffect(() => { - if (connectionRef.current !== null && !isConnectingRef.current && onStoreChangeFnRef.current) { - shouldGetCachedValueRef.current = true; - onStoreChangeFnRef.current(); + // This effect will only run if the `dependencies` array changes. If it changes it will force the hook + // to trigger a `getSnapshot` update by calling the stored `onStoreChange` function reference, thus + // re-running the hook and returning the latest value to the consumer. + if (connectionRef.current === null || isConnectingRef.current || !onStoreChangeFnRef.current) { + return; } + + shouldGetCachedValueRef.current = true; + onStoreChangeFnRef.current(); + // eslint-disable-next-line react-hooks/exhaustive-deps }, [...dependencies]); // Mimics withOnyx's checkEvictableKeys() behavior. diff --git a/tests/unit/useOnyxTest.ts b/tests/unit/useOnyxTest.ts index ebc975b8..2c874469 100644 --- a/tests/unit/useOnyxTest.ts +++ b/tests/unit/useOnyxTest.ts @@ -611,7 +611,7 @@ describe('useOnyx', () => { }); describe('dependencies', () => { - it('should 1', async () => { + it('should return the updated selected value when a external value passed to the dependencies list changes', async () => { Onyx.mergeCollection(ONYXKEYS.COLLECTION.TEST_KEY, { [`${ONYXKEYS.COLLECTION.TEST_KEY}entry1`]: {id: 'entry1_id', name: 'entry1_name'}, [`${ONYXKEYS.COLLECTION.TEST_KEY}entry2`]: {id: 'entry2_id', name: 'entry2_name'}, From 6217ef91081cb8afc1d5e239cda0f4f8fb48b4f6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A1bio=20Henriques?= Date: Thu, 5 Dec 2024 17:16:11 +0000 Subject: [PATCH 3/3] Minor adjustments --- lib/useOnyx.ts | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/lib/useOnyx.ts b/lib/useOnyx.ts index cad6f53d..15ccc06c 100644 --- a/lib/useOnyx.ts +++ b/lib/useOnyx.ts @@ -147,7 +147,7 @@ function useOnyx>( // Indicates if the hook is connecting to a Onyx key. const isConnectingRef = useRef(false); - // Stores the `onStoreChange` function, which can be used to trigger a `getSnapshot` update when desired. + // Stores the `onStoreChange()` function, which can be used to trigger a `getSnapshot()` update when desired. const onStoreChangeFnRef = useRef<(() => void) | null>(null); // Indicates if we should get the newest cached value from Onyx during `getSnapshot()` execution. @@ -183,7 +183,7 @@ function useOnyx>( useEffect(() => { // This effect will only run if the `dependencies` array changes. If it changes it will force the hook - // to trigger a `getSnapshot` update by calling the stored `onStoreChange` function reference, thus + // to trigger a `getSnapshot()` update by calling the stored `onStoreChange()` function reference, thus // re-running the hook and returning the latest value to the consumer. if (connectionRef.current === null || isConnectingRef.current || !onStoreChangeFnRef.current) { return; @@ -288,6 +288,7 @@ function useOnyx>( key, callback: () => { isConnectingRef.current = false; + onStoreChangeFnRef.current = onStoreChange; // Signals that the first connection was made, so some logics in `getSnapshot()` // won't be executed anymore. @@ -312,9 +313,9 @@ function useOnyx>( } connectionManager.disconnect(connectionRef.current); - onStoreChangeFnRef.current = null; - isConnectingRef.current = false; isFirstConnectionRef.current = false; + isConnectingRef.current = false; + onStoreChangeFnRef.current = null; }; }, [key, options?.initWithStoredValues, options?.reuseConnection, checkEvictableKey],