diff --git a/lib/useOnyx.ts b/lib/useOnyx.ts index 9393486e..7e6bf44c 100644 --- a/lib/useOnyx.ts +++ b/lib/useOnyx.ts @@ -7,7 +7,6 @@ import connectionManager from './OnyxConnectionManager'; import OnyxUtils from './OnyxUtils'; import OnyxKeys from './OnyxKeys'; import type {CollectionKeyBase, OnyxKey, OnyxValue} from './types'; -import usePrevious from './usePrevious'; import onyxSnapshotCache from './OnyxSnapshotCache'; import useLiveRef from './useLiveRef'; @@ -56,8 +55,6 @@ function useOnyx>( dependencies: DependencyList = [], ): UseOnyxResult { const connectionRef = useRef(null); - const previousKey = usePrevious(key); - const currentDependenciesRef = useLiveRef(dependencies); const selector = options?.selector; @@ -113,9 +110,12 @@ function useOnyx>( }, ]); - // Indicates if it's the first Onyx connection of this hook or not, as we don't want certain use cases - // in `getSnapshot()` to be satisfied several times. - const isFirstConnectionRef = useRef(true); + // Tracks which key has completed its first Onyx connection callback. When this doesn't match the + // current key, getSnapshot() treats the hook as being in its "first connection" state for that key. + // This is key-aware by design: when the key changes, connectedKeyRef still holds the old key (or null + // after cleanup), so the hook automatically enters first-connection mode for the new key without any + // explicit reset logic — eliminating the race condition where cleanup could clobber a boolean flag. + const connectedKeyRef = useRef(null); // Indicates if the hook is connecting to an Onyx key. const isConnectingRef = useRef(false); @@ -157,7 +157,7 @@ function useOnyx>( previousDependenciesRef.current = dependencies; - if (connectionRef.current === null || isConnectingRef.current || !onStoreChangeFnRef.current) { + if (connectionRef.current === null || isConnectingRef.current || connectedKeyRef.current !== key || !onStoreChangeFnRef.current) { return; } @@ -193,7 +193,8 @@ function useOnyx>( // Check if we have any cache for this Onyx key // Don't use cache for first connection with initWithStoredValues: false // Also don't use cache during active data updates (when shouldGetCachedValueRef is true) - if (!(isFirstConnectionRef.current && options?.initWithStoredValues === false) && !shouldGetCachedValueRef.current) { + const isFirstConnection = connectedKeyRef.current !== key; + if (!(isFirstConnection && options?.initWithStoredValues === false) && !shouldGetCachedValueRef.current) { const cachedResult = onyxSnapshotCache.getCachedResult>(key, cacheKey); if (cachedResult !== undefined) { resultRef.current = cachedResult; @@ -202,7 +203,7 @@ function useOnyx>( } // We return the initial result right away during the first connection if `initWithStoredValues` is set to `false`. - if (isFirstConnectionRef.current && options?.initWithStoredValues === false) { + if (isFirstConnection && options?.initWithStoredValues === false) { const result = resultRef.current; // Store result in snapshot cache @@ -214,7 +215,7 @@ function useOnyx>( // so we can return any cached value right away. For the case where the key has changed, If we don't return the cached value right away, then the UI will show the incorrect (previous) value for a brief period which looks like a UI glitch to the user. After the connection is made, we only // update `newValueRef` when `Onyx.connect()` callback is fired. const hasSelectorChanged = lastComputedSelectorRef.current !== memoizedSelector; - if (isFirstConnectionRef.current || shouldGetCachedValueRef.current || key !== previousKey || hasSelectorChanged) { + if (isFirstConnection || shouldGetCachedValueRef.current || hasSelectorChanged) { // Gets the value from cache and maps it with selector. It changes `null` to `undefined` for `useOnyx` compatibility. const value = OnyxUtils.tryGetCachedValue(key) as OnyxValue; const selectedValue = memoizedSelector ? memoizedSelector(value) : value; @@ -233,7 +234,7 @@ function useOnyx>( // If we have pending merge operations for the key during the first connection, we set the new value to `undefined` // and fetch status to `loading` to simulate that it is still being loaded until we have the most updated data. - if (isFirstConnectionRef.current && OnyxUtils.hasPendingMergeForKey(key)) { + if (isFirstConnection && OnyxUtils.hasPendingMergeForKey(key)) { newValueRef.current = undefined; newFetchStatus = 'loading'; } @@ -258,7 +259,7 @@ function useOnyx>( // OR we have a pending `Onyx.clear()` task (if `Onyx.clear()` is running cache might not be available anymore // OR the subscriber is triggered (the value is gotten from the storage) // so we update the cached value/result right away in order to prevent infinite loading state issues). - const shouldUpdateResult = !areValuesEqual || (previousValueRef.current === null && (hasCacheForKey || OnyxCache.hasPendingTask(TASK.CLEAR) || !isFirstConnectionRef.current)); + const shouldUpdateResult = !areValuesEqual || (previousValueRef.current === null && (hasCacheForKey || OnyxCache.hasPendingTask(TASK.CLEAR) || !isFirstConnection)); if (shouldUpdateResult) { previousValueRef.current = newValueRef.current; @@ -278,10 +279,18 @@ function useOnyx>( } return resultRef.current; - }, [options?.initWithStoredValues, key, memoizedSelector, cacheKey, previousKey]); + }, [options?.initWithStoredValues, key, memoizedSelector, cacheKey]); const subscribe = useCallback( (onStoreChange: () => void) => { + // Reset internal state so the hook properly transitions through loading + // for the new key instead of preserving stale state from the previous one. + previousValueRef.current = null; + newValueRef.current = null; + shouldGetCachedValueRef.current = true; + sourceValueRef.current = undefined; + resultRef.current = [undefined, {status: options?.initWithStoredValues === false ? 'loaded' : 'loading'}]; + isConnectingRef.current = true; onStoreChangeFnRef.current = onStoreChange; @@ -291,9 +300,9 @@ function useOnyx>( isConnectingRef.current = false; onStoreChangeFnRef.current = onStoreChange; - // Signals that the first connection was made, so some logics in `getSnapshot()` - // won't be executed anymore. - isFirstConnectionRef.current = false; + // Signals that the first connection was made for this key, so some logics + // in `getSnapshot()` won't be executed anymore. + connectedKeyRef.current = key; // Signals that we want to get the newest cached value again in `getSnapshot()`. shouldGetCachedValueRef.current = true; @@ -320,7 +329,7 @@ function useOnyx>( } connectionManager.disconnect(connectionRef.current); - isFirstConnectionRef.current = false; + connectedKeyRef.current = null; isConnectingRef.current = false; onStoreChangeFnRef.current = null; }; diff --git a/lib/usePrevious.ts b/lib/usePrevious.ts deleted file mode 100644 index e03ab7a6..00000000 --- a/lib/usePrevious.ts +++ /dev/null @@ -1,16 +0,0 @@ -import {useEffect, useRef} from 'react'; - -/** - * Returns the previous value of the provided value. - */ -function usePrevious(value: T): T { - const ref = useRef(value); - - useEffect(() => { - ref.current = value; - }, [value]); - - return ref.current; -} - -export default usePrevious; diff --git a/tests/unit/useOnyxTest.ts b/tests/unit/useOnyxTest.ts index a59ead4a..c2fc2e99 100644 --- a/tests/unit/useOnyxTest.ts +++ b/tests/unit/useOnyxTest.ts @@ -35,6 +35,212 @@ beforeEach(async () => { }); describe('useOnyx', () => { + describe('dynamic key', () => { + beforeEach(() => { + jest.spyOn(console, 'error').mockImplementation(jest.fn); + }); + + afterEach(() => { + (console.error as unknown as jest.SpyInstance>).mockRestore(); + }); + + it('should not throw any errors when changing from a collection member key to another one', async () => { + const {rerender} = renderHook((key: string) => useOnyx(key), {initialProps: `${ONYXKEYS.COLLECTION.TEST_KEY}1` as string}); + + try { + await act(async () => { + rerender(`${ONYXKEYS.COLLECTION.TEST_KEY}2`); + }); + } catch (e) { + fail('Expected to not throw any errors.'); + } + }); + + it('should transition through loading when switching between collection member keys that both resolve to undefined', async () => { + const {result, rerender} = renderHook((key: string) => useOnyx(key), {initialProps: `${ONYXKEYS.COLLECTION.TEST_KEY}1` as string}); + + // Wait for initial key to fully load + await act(async () => waitForPromisesToResolve()); + + expect(result.current[0]).toBeUndefined(); + expect(result.current[1].status).toEqual('loaded'); + + // Switch to another collection member key that also has no data + rerender(`${ONYXKEYS.COLLECTION.TEST_KEY}2`); + + expect(result.current[0]).toBeUndefined(); + expect(result.current[1].status).toEqual('loading'); + + await act(async () => waitForPromisesToResolve()); + + expect(result.current[0]).toBeUndefined(); + expect(result.current[1].status).toEqual('loaded'); + }); + + it('should return cached value immediately with loaded status when switching to a key that has data', async () => { + Onyx.set(`${ONYXKEYS.COLLECTION.TEST_KEY}2`, 'test_value'); + + const {result, rerender} = renderHook((key: string) => useOnyx(key), {initialProps: `${ONYXKEYS.COLLECTION.TEST_KEY}1` as string}); + + await act(async () => waitForPromisesToResolve()); + + expect(result.current[0]).toBeUndefined(); + expect(result.current[1].status).toEqual('loaded'); + + // Switch to a key that has cached data + rerender(`${ONYXKEYS.COLLECTION.TEST_KEY}2`); + + // Value and loaded status should be available synchronously, without waiting for promises + expect(result.current[0]).toEqual('test_value'); + expect(result.current[1].status).toEqual('loaded'); + + await act(async () => waitForPromisesToResolve()); + + expect(result.current[0]).toEqual('test_value'); + expect(result.current[1].status).toEqual('loaded'); + }); + + it('should clear previous data and transition through loading when switching from a key with data to one without', async () => { + Onyx.set(`${ONYXKEYS.COLLECTION.TEST_KEY}1`, 'initial_value'); + + const {result, rerender} = renderHook((key: string) => useOnyx(key), {initialProps: `${ONYXKEYS.COLLECTION.TEST_KEY}1` as string}); + + await act(async () => waitForPromisesToResolve()); + + expect(result.current[0]).toEqual('initial_value'); + expect(result.current[1].status).toEqual('loaded'); + + // Switch to a key that has no data + rerender(`${ONYXKEYS.COLLECTION.TEST_KEY}2`); + + expect(result.current[0]).toBeUndefined(); + expect(result.current[1].status).toEqual('loading'); + + await act(async () => waitForPromisesToResolve()); + + expect(result.current[0]).toBeUndefined(); + expect(result.current[1].status).toEqual('loaded'); + }); + + it('should return the new value when switching from a key with data to another key with different data', async () => { + Onyx.set(`${ONYXKEYS.COLLECTION.TEST_KEY}1`, 'value_one'); + Onyx.set(`${ONYXKEYS.COLLECTION.TEST_KEY}2`, 'value_two'); + + const {result, rerender} = renderHook((key: string) => useOnyx(key), {initialProps: `${ONYXKEYS.COLLECTION.TEST_KEY}1` as string}); + + await act(async () => waitForPromisesToResolve()); + + expect(result.current[0]).toEqual('value_one'); + expect(result.current[1].status).toEqual('loaded'); + + // Switch to another key that also has data + rerender(`${ONYXKEYS.COLLECTION.TEST_KEY}2`); + + await act(async () => waitForPromisesToResolve()); + + expect(result.current[0]).toEqual('value_two'); + expect(result.current[1].status).toEqual('loaded'); + }); + + it('should apply the selector against the new key data when switching keys', async () => { + Onyx.set(`${ONYXKEYS.COLLECTION.TEST_KEY}1`, {id: 'entry1_id', name: 'entry1_name'}); + Onyx.set(`${ONYXKEYS.COLLECTION.TEST_KEY}2`, {id: 'entry2_id', name: 'entry2_name'}); + + const selector = ((entry: OnyxEntry<{id: string; name: string}>) => entry?.name) as UseOnyxSelector; + + const {result, rerender} = renderHook((key: string) => useOnyx(key, {selector}), {initialProps: `${ONYXKEYS.COLLECTION.TEST_KEY}1` as string}); + + await act(async () => waitForPromisesToResolve()); + + expect(result.current[0]).toEqual('entry1_name'); + expect(result.current[1].status).toEqual('loaded'); + + // Switch key — selector should run against the new key's data + rerender(`${ONYXKEYS.COLLECTION.TEST_KEY}2`); + + await act(async () => waitForPromisesToResolve()); + + expect(result.current[0]).toEqual('entry2_name'); + expect(result.current[1].status).toEqual('loaded'); + }); + + it('should handle rapid key switching and settle on the final key value', async () => { + Onyx.set(`${ONYXKEYS.COLLECTION.TEST_KEY}1`, 'value_one'); + Onyx.set(`${ONYXKEYS.COLLECTION.TEST_KEY}2`, 'value_two'); + Onyx.set(`${ONYXKEYS.COLLECTION.TEST_KEY}3`, 'value_three'); + + const {result, rerender} = renderHook((key: string) => useOnyx(key), {initialProps: `${ONYXKEYS.COLLECTION.TEST_KEY}1` as string}); + + await act(async () => waitForPromisesToResolve()); + + expect(result.current[0]).toEqual('value_one'); + + // Rapidly switch keys without waiting for promises between switches + rerender(`${ONYXKEYS.COLLECTION.TEST_KEY}2`); + rerender(`${ONYXKEYS.COLLECTION.TEST_KEY}3`); + + await act(async () => waitForPromisesToResolve()); + + expect(result.current[0]).toEqual('value_three'); + expect(result.current[1].status).toEqual('loaded'); + }); + + it('should correctly switch between non-collection keys', async () => { + Onyx.set(ONYXKEYS.TEST_KEY, 'value_one'); + Onyx.set(ONYXKEYS.TEST_KEY_2, 'value_two'); + + const {result, rerender} = renderHook((key: string) => useOnyx(key), {initialProps: ONYXKEYS.TEST_KEY as string}); + + await act(async () => waitForPromisesToResolve()); + + expect(result.current[0]).toEqual('value_one'); + expect(result.current[1].status).toEqual('loaded'); + + rerender(ONYXKEYS.TEST_KEY_2); + + // Cached value should be available synchronously + expect(result.current[0]).toEqual('value_two'); + expect(result.current[1].status).toEqual('loaded'); + + await act(async () => waitForPromisesToResolve()); + + expect(result.current[0]).toEqual('value_two'); + expect(result.current[1].status).toEqual('loaded'); + }); + + it('should correctly return to a previously used key (A → B → A round-trip)', async () => { + Onyx.set(`${ONYXKEYS.COLLECTION.TEST_KEY}1`, 'value_one'); + Onyx.set(`${ONYXKEYS.COLLECTION.TEST_KEY}2`, 'value_two'); + + const {result, rerender} = renderHook((key: string) => useOnyx(key), {initialProps: `${ONYXKEYS.COLLECTION.TEST_KEY}1` as string}); + + await act(async () => waitForPromisesToResolve()); + + expect(result.current[0]).toEqual('value_one'); + expect(result.current[1].status).toEqual('loaded'); + + // A → B + rerender(`${ONYXKEYS.COLLECTION.TEST_KEY}2`); + + await act(async () => waitForPromisesToResolve()); + + expect(result.current[0]).toEqual('value_two'); + expect(result.current[1].status).toEqual('loaded'); + + // B → A + rerender(`${ONYXKEYS.COLLECTION.TEST_KEY}1`); + + // Cached value should be available synchronously + expect(result.current[0]).toEqual('value_one'); + expect(result.current[1].status).toEqual('loaded'); + + await act(async () => waitForPromisesToResolve()); + + expect(result.current[0]).toEqual('value_one'); + expect(result.current[1].status).toEqual('loaded'); + }); + }); + describe('misc', () => { it('should initially return loading state while loading non-existent key, and then return `undefined` and loaded state', async () => { const {result} = renderHook(() => useOnyx(ONYXKEYS.TEST_KEY)); @@ -711,6 +917,33 @@ describe('useOnyx', () => { expect(result.current[0]).toEqual('test_selected'); expect(result.current[1].status).toEqual('loaded'); }); + + it('should suppress stored values for the new key when switching keys with initWithStoredValues: false', async () => { + await StorageMock.setItem(ONYXKEYS.TEST_KEY, 'stored_value_one'); + await StorageMock.setItem(ONYXKEYS.TEST_KEY_2, 'stored_value_two'); + + const {result, rerender} = renderHook((key: string) => useOnyx(key, {initWithStoredValues: false}), {initialProps: ONYXKEYS.TEST_KEY as string}); + + await act(async () => waitForPromisesToResolve()); + + // initWithStoredValues: false — stored value should be suppressed + expect(result.current[0]).toBeUndefined(); + expect(result.current[1].status).toEqual('loaded'); + + rerender(ONYXKEYS.TEST_KEY_2); + + await act(async () => waitForPromisesToResolve()); + + // Stored value for the new key should also be suppressed + expect(result.current[0]).toBeUndefined(); + expect(result.current[1].status).toEqual('loaded'); + + // But live updates should still come through + await act(async () => Onyx.merge(ONYXKEYS.TEST_KEY_2, 'live_value')); + + expect(result.current[0]).toEqual('live_value'); + expect(result.current[1].status).toEqual('loaded'); + }); }); describe('multiple usage', () => {