From 9324ca518886ce09ac3d8f4069e91b8744310926 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Mon, 10 Jun 2024 18:11:34 +0200 Subject: [PATCH 01/12] fix: initial loading state of useOnyx --- lib/useOnyx.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/useOnyx.ts b/lib/useOnyx.ts index 230cbafa..854ddc77 100644 --- a/lib/useOnyx.ts +++ b/lib/useOnyx.ts @@ -74,7 +74,7 @@ function useOnyx>(key: TKey // Stores the previous cached value as it's necessary to compare with the new value in `getSnapshot()`. // We initialize it to `undefined` to simulate that we don't have any value from cache yet. - const cachedValueRef = useRef | undefined>(undefined); + const cachedValueRef = useRef | undefined | null>(null); // Stores the previously result returned by the hook, containing the data from cache and the fetch status. // We initialize it to `undefined` and `loading` fetch status to simulate the initial result when the hook is loading from the cache. @@ -142,11 +142,11 @@ function useOnyx>(key: TKey // If the previously cached value is different from the new value, we update both cached value // and the result to be returned by the hook. - if (!deepEqual(cachedValueRef.current, newValue)) { + // If `cachedValueRef.current` is null, it means that we don't have any value from cache yet. In this case, we will always update the result as part of the the initial update. + if (cachedValueRef.current === null || !deepEqual(cachedValueRef.current, newValue)) { cachedValueRef.current = newValue; - // If the new value is `null` we default it to `undefined` to ensure the consumer get a consistent result from the hook. - resultRef.current = [(cachedValueRef.current ?? undefined) as CachedValue, {status: newFetchStatus ?? 'loaded'}]; + resultRef.current = [cachedValueRef.current as CachedValue, {status: newFetchStatus ?? 'loaded'}]; } return resultRef.current; From 9cd73cf9b70f1a42a0a04a2e9753c95953919275 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Tue, 11 Jun 2024 14:14:25 +0200 Subject: [PATCH 02/12] fix: initial values and loading states --- lib/useOnyx.ts | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/lib/useOnyx.ts b/lib/useOnyx.ts index 854ddc77..1d1fcb12 100644 --- a/lib/useOnyx.ts +++ b/lib/useOnyx.ts @@ -6,6 +6,7 @@ import type {CollectionKeyBase, OnyxCollection, OnyxKey, OnyxValue, Selector} fr import useLiveRef from './useLiveRef'; import usePrevious from './usePrevious'; import Onyx from './Onyx'; +import cache from './OnyxCache'; type BaseUseOnyxOptions = { /** @@ -73,7 +74,7 @@ function useOnyx>(key: TKey const selectorRef = useLiveRef(options?.selector); // Stores the previous cached value as it's necessary to compare with the new value in `getSnapshot()`. - // We initialize it to `undefined` to simulate that we don't have any value from cache yet. + // We initialize it to `null` to simulate that we don't have any value from cache yet. const cachedValueRef = useRef | undefined | null>(null); // Stores the previously result returned by the hook, containing the data from cache and the fetch status. @@ -121,6 +122,7 @@ function useOnyx>(key: TKey // This difference between `undefined` and other values is crucial and it's used to address the following // conditions and use cases. let newValue = getCachedValue(key, selectorRef.current); + const hasCacheForKey = cache.hasCacheForKey(key); // Since the fetch status can be different given the use cases below, we define the variable right away. let newFetchStatus: FetchStatus | undefined; @@ -133,24 +135,27 @@ function useOnyx>(key: TKey newFetchStatus = 'loading'; } - // If data is not present in cache (if it's `undefined`) and `initialValue` is set during the first connection, + // If data is not present in cache and `initialValue` is set during the first connection, // we set the new value to `initialValue` and fetch status to `loaded` since we already have some data to return to the consumer. - if (isFirstConnectionRef.current && newValue === undefined && options?.initialValue !== undefined) { + if (isFirstConnectionRef.current && !hasCacheForKey && options?.initialValue !== undefined) { newValue = options?.initialValue as CachedValue; newFetchStatus = 'loaded'; } // If the previously cached value is different from the new value, we update both cached value // and the result to be returned by the hook. - // If `cachedValueRef.current` is null, it means that we don't have any value from cache yet. In this case, we will always update the result as part of the the initial update. - if (cachedValueRef.current === null || !deepEqual(cachedValueRef.current, newValue)) { - cachedValueRef.current = newValue; + if (!deepEqual(cachedValueRef.current, newValue)) { + // If there is no cached value yet and the value read from cache is `undefined`, we set the fetch status to `loading`. + if (cachedValueRef.current === null && newValue === undefined) newFetchStatus = 'loading'; + // If `options.initiWithStoredValues` is explicitly set to `false`, the fetch status should always be `loaded`. + if (options?.initWithStoredValues === false) newFetchStatus = 'loaded'; + cachedValueRef.current = newValue; resultRef.current = [cachedValueRef.current as CachedValue, {status: newFetchStatus ?? 'loaded'}]; } return resultRef.current; - }, [key, selectorRef, options?.allowStaleData, options?.initialValue]); + }, [key, selectorRef, options?.allowStaleData, options?.initialValue, options?.initWithStoredValues]); const subscribe = useCallback( (onStoreChange: () => void) => { From 43335eed490726f1c23852b5038a6fdac1fc0788 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Tue, 11 Jun 2024 14:14:50 +0200 Subject: [PATCH 03/12] improve Onyx.clear, clearing values from cache --- lib/Onyx.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/Onyx.ts b/lib/Onyx.ts index 5d2acdec..65a932dc 100644 --- a/lib/Onyx.ts +++ b/lib/Onyx.ts @@ -575,7 +575,7 @@ function clear(keysToPreserve: OnyxKey[] = []): Promise { // since collection key subscribers need to be updated differently if (!isKeyToPreserve) { const oldValue = cache.get(key); - const newValue = defaultKeyStates[key] ?? undefined; + const newValue = defaultKeyStates[key] ?? null; if (newValue !== oldValue) { cache.set(key, newValue); const collectionKey = key.substring(0, key.indexOf('_') + 1); @@ -583,9 +583,9 @@ function clear(keysToPreserve: OnyxKey[] = []): Promise { if (!keyValuesToResetAsCollection[collectionKey]) { keyValuesToResetAsCollection[collectionKey] = {}; } - keyValuesToResetAsCollection[collectionKey]![key] = newValue; + keyValuesToResetAsCollection[collectionKey]![key] = newValue ?? undefined; } else { - keyValuesToResetIndividually[key] = newValue; + keyValuesToResetIndividually[key] = newValue ?? undefined; } } } From 7165f1c26623a1532bbf23ad59b0d2e31736f3d3 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Tue, 11 Jun 2024 14:15:11 +0200 Subject: [PATCH 04/12] update tests to reflect new Onyx cache behaviour --- tests/unit/useOnyxTest.ts | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/tests/unit/useOnyxTest.ts b/tests/unit/useOnyxTest.ts index 04160c51..973eef12 100644 --- a/tests/unit/useOnyxTest.ts +++ b/tests/unit/useOnyxTest.ts @@ -98,13 +98,13 @@ describe('useOnyx', () => { }); it('should initially return null while loading non-cached key, and then return value and loaded state', async () => { - await StorageMock.setItem(ONYXKEYS.TEST_KEY, 'test'); - const {result} = renderHook(() => useOnyx(ONYXKEYS.TEST_KEY)); expect(result.current[0]).toBeUndefined(); expect(result.current[1].status).toEqual('loading'); + await StorageMock.setItem(ONYXKEYS.TEST_KEY, 'test'); + await act(async () => waitForPromisesToResolve()); expect(result.current[0]).toEqual('test'); @@ -244,8 +244,6 @@ describe('useOnyx', () => { }); it('should return initial value if selected data is undefined', async () => { - Onyx.set(ONYXKEYS.TEST_KEY, 'test_id_1'); - const {result} = renderHook(() => useOnyx(ONYXKEYS.TEST_KEY, { // @ts-expect-error bypass @@ -257,6 +255,8 @@ describe('useOnyx', () => { expect(result.current[0]).toEqual('initial value'); expect(result.current[1].status).toEqual('loaded'); + Onyx.set(ONYXKEYS.TEST_KEY, 'test_id_1'); + await act(async () => waitForPromisesToResolve()); expect(result.current[0]).toBeUndefined(); @@ -320,12 +320,6 @@ describe('useOnyx', () => { }); it('should return initial value and loaded state while we have pending merges for the key, and then return updated value and loaded state', async () => { - Onyx.set(ONYXKEYS.TEST_KEY, 'test1'); - - Onyx.merge(ONYXKEYS.TEST_KEY, 'test2'); - Onyx.merge(ONYXKEYS.TEST_KEY, 'test3'); - Onyx.merge(ONYXKEYS.TEST_KEY, 'test4'); - const {result} = renderHook(() => useOnyx(ONYXKEYS.TEST_KEY, { initialValue: 'initial value', @@ -335,6 +329,12 @@ describe('useOnyx', () => { expect(result.current[0]).toEqual('initial value'); expect(result.current[1].status).toEqual('loaded'); + Onyx.set(ONYXKEYS.TEST_KEY, 'test1'); + + Onyx.merge(ONYXKEYS.TEST_KEY, 'test2'); + Onyx.merge(ONYXKEYS.TEST_KEY, 'test3'); + Onyx.merge(ONYXKEYS.TEST_KEY, 'test4'); + await act(async () => waitForPromisesToResolve()); expect(result.current[0]).toEqual('test4'); @@ -361,7 +361,7 @@ describe('useOnyx', () => { }); describe('initWithStoredValues', () => { - it('should return null and loaded state, and after merge return updated value and loaded state', async () => { + it('should return `undefined` and loaded state, and after merge return updated value and loaded state', async () => { await StorageMock.setItem(ONYXKEYS.TEST_KEY, 'test1'); const {result} = renderHook(() => useOnyx(ONYXKEYS.TEST_KEY, {initWithStoredValues: false})); From 1a983e53fa6ad9465206d10bef0d08f00a28f228 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Tue, 11 Jun 2024 14:19:48 +0200 Subject: [PATCH 05/12] undo unnecessary changes to one test --- tests/unit/useOnyxTest.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/unit/useOnyxTest.ts b/tests/unit/useOnyxTest.ts index 973eef12..4b826a9e 100644 --- a/tests/unit/useOnyxTest.ts +++ b/tests/unit/useOnyxTest.ts @@ -98,13 +98,13 @@ describe('useOnyx', () => { }); it('should initially return null while loading non-cached key, and then return value and loaded state', async () => { + await StorageMock.setItem(ONYXKEYS.TEST_KEY, 'test'); + const {result} = renderHook(() => useOnyx(ONYXKEYS.TEST_KEY)); expect(result.current[0]).toBeUndefined(); expect(result.current[1].status).toEqual('loading'); - await StorageMock.setItem(ONYXKEYS.TEST_KEY, 'test'); - await act(async () => waitForPromisesToResolve()); expect(result.current[0]).toEqual('test'); From 0ca347643185db6de79a35b350befd8c6aba061b Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Tue, 11 Jun 2024 15:16:19 +0200 Subject: [PATCH 06/12] fix: improve code --- lib/useOnyx.ts | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/lib/useOnyx.ts b/lib/useOnyx.ts index 1d1fcb12..34fde5b4 100644 --- a/lib/useOnyx.ts +++ b/lib/useOnyx.ts @@ -54,10 +54,6 @@ type ResultMetadata = { type UseOnyxResult = [CachedValue, ResultMetadata]; -function getCachedValue(key: TKey, selector?: Selector): CachedValue | undefined { - return OnyxUtils.tryGetCachedValue(key, {selector}) as CachedValue | undefined; -} - function useOnyx>( key: TKey, options?: BaseUseOnyxOptions & UseOnyxInitialValueOption & Required>, @@ -121,7 +117,7 @@ function useOnyx>(key: TKey // If `newValue` is `null` or any other value if means that the cache does have a value for that key. // This difference between `undefined` and other values is crucial and it's used to address the following // conditions and use cases. - let newValue = getCachedValue(key, selectorRef.current); + let newValue = OnyxUtils.tryGetCachedValue(key, {selector: selectorRef.current}) as CachedValue | undefined; const hasCacheForKey = cache.hasCacheForKey(key); // Since the fetch status can be different given the use cases below, we define the variable right away. @@ -137,18 +133,23 @@ function useOnyx>(key: TKey // If data is not present in cache and `initialValue` is set during the first connection, // we set the new value to `initialValue` and fetch status to `loaded` since we already have some data to return to the consumer. - if (isFirstConnectionRef.current && !hasCacheForKey && options?.initialValue !== undefined) { + const hasInitialValue = isFirstConnectionRef.current && !hasCacheForKey && options?.initialValue !== undefined; + if (hasInitialValue) { newValue = options?.initialValue as CachedValue; newFetchStatus = 'loaded'; } + // If `options.initiWithStoredValues` is explicitly set to `false`, the fetch status should always be `loaded`. + if (options?.initWithStoredValues === false) { + newFetchStatus = 'loaded'; + } + // If the previously cached value is different from the new value, we update both cached value // and the result to be returned by the hook. if (!deepEqual(cachedValueRef.current, newValue)) { - // If there is no cached value yet and the value read from cache is `undefined`, we set the fetch status to `loading`. - if (cachedValueRef.current === null && newValue === undefined) newFetchStatus = 'loading'; - // If `options.initiWithStoredValues` is explicitly set to `false`, the fetch status should always be `loaded`. - if (options?.initWithStoredValues === false) newFetchStatus = 'loaded'; + // If there is no cached value yet, the value read from cache is `undefined` and the fetch status hasn't already been set, + // we set the fetch status to `loading`. + if (!newFetchStatus && cachedValueRef.current === null && !hasCacheForKey) newFetchStatus = 'loading'; cachedValueRef.current = newValue; resultRef.current = [cachedValueRef.current as CachedValue, {status: newFetchStatus ?? 'loaded'}]; From 5da5e50c80400446a2eb86dddecd48412ab37ac7 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Tue, 11 Jun 2024 15:16:24 +0200 Subject: [PATCH 07/12] rename test --- tests/unit/useOnyxTest.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/useOnyxTest.ts b/tests/unit/useOnyxTest.ts index 4b826a9e..bd9a4430 100644 --- a/tests/unit/useOnyxTest.ts +++ b/tests/unit/useOnyxTest.ts @@ -97,7 +97,7 @@ describe('useOnyx', () => { expect(result.current[1].status).toEqual('loaded'); }); - it('should initially return null while loading non-cached key, and then return value and loaded state', async () => { + it('should initially return `undefined` while loading non-cached key, and then return value and loaded state', async () => { await StorageMock.setItem(ONYXKEYS.TEST_KEY, 'test'); const {result} = renderHook(() => useOnyx(ONYXKEYS.TEST_KEY)); From a0991a9c88ab712624a7bee90ca26c0ba1aa7d68 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Tue, 11 Jun 2024 16:12:29 +0200 Subject: [PATCH 08/12] fix: update useOnyx result when cache was set for the first time --- lib/useOnyx.ts | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/lib/useOnyx.ts b/lib/useOnyx.ts index 34fde5b4..58169bc7 100644 --- a/lib/useOnyx.ts +++ b/lib/useOnyx.ts @@ -117,7 +117,7 @@ function useOnyx>(key: TKey // If `newValue` is `null` or any other value if means that the cache does have a value for that key. // This difference between `undefined` and other values is crucial and it's used to address the following // conditions and use cases. - let newValue = OnyxUtils.tryGetCachedValue(key, {selector: selectorRef.current}) as CachedValue | undefined; + let newValue = (OnyxUtils.tryGetCachedValue(key, {selector: selectorRef.current}) ?? undefined) as CachedValue | undefined; const hasCacheForKey = cache.hasCacheForKey(key); // Since the fetch status can be different given the use cases below, we define the variable right away. @@ -131,26 +131,23 @@ function useOnyx>(key: TKey newFetchStatus = 'loading'; } - // If data is not present in cache and `initialValue` is set during the first connection, - // we set the new value to `initialValue` and fetch status to `loaded` since we already have some data to return to the consumer. - const hasInitialValue = isFirstConnectionRef.current && !hasCacheForKey && options?.initialValue !== undefined; - if (hasInitialValue) { - newValue = options?.initialValue as CachedValue; + // If `options.initWithStoredValues` is explicitly set to `false`, the fetch status should always be `loaded`. + if (options?.initWithStoredValues === false) { newFetchStatus = 'loaded'; } - // If `options.initiWithStoredValues` is explicitly set to `false`, the fetch status should always be `loaded`. - if (options?.initWithStoredValues === false) { + // If data is not present in cache and `initialValue` is set during the first connection, + // we set the new value to `initialValue` and fetch status to `loaded` since we already have some data to return to the consumer. + if (isFirstConnectionRef.current && !hasCacheForKey && options?.initialValue !== null && options?.initialValue !== undefined) { + newValue = (options?.initialValue ?? undefined) as CachedValue; newFetchStatus = 'loaded'; } // If the previously cached value is different from the new value, we update both cached value // and the result to be returned by the hook. - if (!deepEqual(cachedValueRef.current, newValue)) { - // If there is no cached value yet, the value read from cache is `undefined` and the fetch status hasn't already been set, - // we set the fetch status to `loading`. - if (!newFetchStatus && cachedValueRef.current === null && !hasCacheForKey) newFetchStatus = 'loading'; - + // If the cache was set for the first time, we also update the cached value and the result. + const isCacheSetFirstTime = cachedValueRef.current === null && hasCacheForKey; + if (isCacheSetFirstTime || !deepEqual(cachedValueRef.current ?? undefined, newValue)) { cachedValueRef.current = newValue; resultRef.current = [cachedValueRef.current as CachedValue, {status: newFetchStatus ?? 'loaded'}]; } From 9c94b0ca17270c91c8b10a3ae9ae7b15b48e13e9 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Tue, 11 Jun 2024 17:16:21 +0200 Subject: [PATCH 09/12] remove unused check --- lib/useOnyx.ts | 5 ----- 1 file changed, 5 deletions(-) diff --git a/lib/useOnyx.ts b/lib/useOnyx.ts index 58169bc7..f54469ed 100644 --- a/lib/useOnyx.ts +++ b/lib/useOnyx.ts @@ -131,11 +131,6 @@ function useOnyx>(key: TKey newFetchStatus = 'loading'; } - // If `options.initWithStoredValues` is explicitly set to `false`, the fetch status should always be `loaded`. - if (options?.initWithStoredValues === false) { - newFetchStatus = 'loaded'; - } - // If data is not present in cache and `initialValue` is set during the first connection, // we set the new value to `initialValue` and fetch status to `loaded` since we already have some data to return to the consumer. if (isFirstConnectionRef.current && !hasCacheForKey && options?.initialValue !== null && options?.initialValue !== undefined) { From ec1c9bae5b72460cc87f16bfb77a5e5e9ae2f091 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Tue, 11 Jun 2024 17:17:59 +0200 Subject: [PATCH 10/12] add back getCachedValue function --- lib/useOnyx.ts | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/lib/useOnyx.ts b/lib/useOnyx.ts index f54469ed..f7817940 100644 --- a/lib/useOnyx.ts +++ b/lib/useOnyx.ts @@ -54,6 +54,10 @@ type ResultMetadata = { type UseOnyxResult = [CachedValue, ResultMetadata]; +function getCachedValue(key: TKey, selector?: Selector): CachedValue | undefined { + return (OnyxUtils.tryGetCachedValue(key, {selector}) ?? undefined) as CachedValue | undefined; +} + function useOnyx>( key: TKey, options?: BaseUseOnyxOptions & UseOnyxInitialValueOption & Required>, @@ -117,7 +121,7 @@ function useOnyx>(key: TKey // If `newValue` is `null` or any other value if means that the cache does have a value for that key. // This difference between `undefined` and other values is crucial and it's used to address the following // conditions and use cases. - let newValue = (OnyxUtils.tryGetCachedValue(key, {selector: selectorRef.current}) ?? undefined) as CachedValue | undefined; + let newValue = getCachedValue(key, selectorRef.current); const hasCacheForKey = cache.hasCacheForKey(key); // Since the fetch status can be different given the use cases below, we define the variable right away. @@ -148,7 +152,7 @@ function useOnyx>(key: TKey } return resultRef.current; - }, [key, selectorRef, options?.allowStaleData, options?.initialValue, options?.initWithStoredValues]); + }, [key, selectorRef, options?.allowStaleData, options?.initialValue]); const subscribe = useCallback( (onStoreChange: () => void) => { From 9134e7e2f32317742d793fdfe707cd40332b79c4 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Tue, 11 Jun 2024 17:18:25 +0200 Subject: [PATCH 11/12] remove unnecessary check --- lib/useOnyx.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/useOnyx.ts b/lib/useOnyx.ts index f7817940..d673ab8a 100644 --- a/lib/useOnyx.ts +++ b/lib/useOnyx.ts @@ -137,7 +137,7 @@ function useOnyx>(key: TKey // If data is not present in cache and `initialValue` is set during the first connection, // we set the new value to `initialValue` and fetch status to `loaded` since we already have some data to return to the consumer. - if (isFirstConnectionRef.current && !hasCacheForKey && options?.initialValue !== null && options?.initialValue !== undefined) { + if (isFirstConnectionRef.current && !hasCacheForKey && options?.initialValue !== undefined) { newValue = (options?.initialValue ?? undefined) as CachedValue; newFetchStatus = 'loaded'; } From cdbd255262f03969a23abbc6e10c8f3eb3eb902a Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Tue, 11 Jun 2024 17:57:01 +0200 Subject: [PATCH 12/12] fix: typo --- lib/useOnyx.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/useOnyx.ts b/lib/useOnyx.ts index d673ab8a..45db3d78 100644 --- a/lib/useOnyx.ts +++ b/lib/useOnyx.ts @@ -118,7 +118,7 @@ function useOnyx>(key: TKey const getSnapshot = useCallback(() => { // We get the value from the cache, supplying a selector too in case it's defined. // If `newValue` is `undefined` it means that the cache doesn't have a value for that key yet. - // If `newValue` is `null` or any other value if means that the cache does have a value for that key. + // If `newValue` is `null` or any other value it means that the cache does have a value for that key. // This difference between `undefined` and other values is crucial and it's used to address the following // conditions and use cases. let newValue = getCachedValue(key, selectorRef.current);