diff --git a/lib/useOnyx.ts b/lib/useOnyx.ts index 031c2194f..b2c060e5d 100644 --- a/lib/useOnyx.ts +++ b/lib/useOnyx.ts @@ -10,6 +10,7 @@ import type {CollectionKeyBase, OnyxKey, OnyxValue} from './types'; import usePrevious from './usePrevious'; import decorateWithMetrics from './metrics'; import * as Logger from './Logger'; +import useLiveRef from './useLiveRef'; type UseOnyxSelector> = (data: OnyxValue | undefined) => TReturnValue; @@ -75,30 +76,42 @@ function useOnyx>( const connectionRef = useRef(null); const previousKey = usePrevious(key); + const currentDependenciesRef = useLiveRef(dependencies); + const currentSelectorRef = useLiveRef(options?.selector); + // Create memoized version of selector for performance const memoizedSelector = useMemo(() => { if (!options?.selector) return null; let lastInput: OnyxValue | undefined; let lastOutput: TReturnValue; + let lastDependencies: DependencyList = []; let hasComputed = false; return (input: OnyxValue | undefined): TReturnValue => { - // Always recompute when input changes - if (!hasComputed || lastInput !== input) { - const newOutput = options.selector!(input); - - // Deep equality mode: only update if output actually changed - if (!hasComputed || !deepEqual(lastOutput, newOutput)) { - lastInput = input; - lastOutput = newOutput; - hasComputed = true; + const currentDependencies = currentDependenciesRef.current; + const currentSelector = currentSelectorRef.current; + + // Recompute if input changed, dependencies changed, or first time + const dependenciesChanged = !shallowEqual(lastDependencies, currentDependencies); + if (!hasComputed || lastInput !== input || dependenciesChanged) { + // Only proceed if we have a valid selector + if (currentSelector) { + const newOutput = currentSelector(input); + + // Deep equality mode: only update if output actually changed + if (!hasComputed || !deepEqual(lastOutput, newOutput) || dependenciesChanged) { + lastInput = input; + lastOutput = newOutput; + lastDependencies = [...currentDependencies]; + hasComputed = true; + } } } return lastOutput; }; - }, [options?.selector]); + }, [currentDependenciesRef, currentSelectorRef, options?.selector]); // Stores the previous cached value as it's necessary to compare with the new value in `getSnapshot()`. // We initialize it to `null` to simulate that we don't have any value from cache yet. @@ -157,10 +170,22 @@ function useOnyx>( ); }, [previousKey, key, options?.allowDynamicKey]); + // Track previous dependencies to prevent infinite loops + const previousDependenciesRef = useRef([]); + 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 // re-running the hook and returning the latest value to the consumer. + + // Deep equality check to prevent infinite loops when dependencies array reference changes + // but content remains the same + if (shallowEqual(previousDependenciesRef.current, dependencies)) { + return; + } + + previousDependenciesRef.current = dependencies; + if (connectionRef.current === null || isConnectingRef.current || !onStoreChangeFnRef.current) { return; } diff --git a/tests/unit/useOnyxTest.ts b/tests/unit/useOnyxTest.ts index d1ba29235..9ea08a5be 100644 --- a/tests/unit/useOnyxTest.ts +++ b/tests/unit/useOnyxTest.ts @@ -465,48 +465,6 @@ describe('useOnyx', () => { expect(result.current[0]).toBe(firstResult); }); - it('should use reference equality for memoized selectors instead of deep equality', async () => { - // This test verifies the optimization where memoized selectors use reference equality - const complexObject = { - nested: { - array: [1, 2, 3], - object: {prop: 'value'}, - }, - }; - - Onyx.set(ONYXKEYS.TEST_KEY, complexObject); - - const {result} = renderHook(() => - useOnyx(ONYXKEYS.TEST_KEY, { - // @ts-expect-error bypass - selector: (entry: OnyxEntry) => entry?.nested, - }), - ); - - await act(async () => waitForPromisesToResolve()); - - const firstResult = result.current[0]; - - // Set the exact same object reference - await act(async () => Onyx.set(ONYXKEYS.TEST_KEY, complexObject)); - - // Should return same reference due to memoization - expect(result.current[0]).toBe(firstResult); - - // Set different object with same content - const differentObjectSameContent = { - nested: { - array: [1, 2, 3], - object: {prop: 'value'}, - }, - }; - - await act(async () => Onyx.set(ONYXKEYS.TEST_KEY, differentObjectSameContent)); - - // Should return same reference due to deep equality in memoized selector - expect(result.current[0]).toBe(firstResult); - }); - it('should memoize primitive selector results correctly', async () => { Onyx.set(ONYXKEYS.TEST_KEY, {count: 5, name: 'test'}); @@ -536,6 +494,175 @@ describe('useOnyx', () => { expect(result.current[0]).not.toBe(firstResult); expect(result.current[0]).toBe(10); }); + + it('should recompute selector when dependencies change even if input data stays the same', async () => { + const testCollection = { + [`${ONYXKEYS.COLLECTION.TEST_KEY}1`]: {id: '1', value: 'item1'}, + [`${ONYXKEYS.COLLECTION.TEST_KEY}2`]: {id: '2', value: 'item2'}, + [`${ONYXKEYS.COLLECTION.TEST_KEY}3`]: {id: '3', value: 'item3'}, + }; + + await act(async () => Onyx.mergeCollection(ONYXKEYS.COLLECTION.TEST_KEY, testCollection as GenericCollection)); + + let filterIds = ['1']; + let selectorCallCount = 0; + + const {result, rerender} = renderHook(() => + useOnyx( + ONYXKEYS.COLLECTION.TEST_KEY, + { + selector: (collection) => { + selectorCallCount++; + return filterIds.map((id) => (collection as OnyxCollection)?.[`${ONYXKEYS.COLLECTION.TEST_KEY}${id}`]).filter(Boolean); + }, + }, + [filterIds], + ), + ); + + await act(async () => waitForPromisesToResolve()); + + // Record count after initial stabilization + const initialCallCount = selectorCallCount; + const initialResult = result.current[0]; + + // Should return item with id '1' + expect(initialResult).toEqual([{id: '1', value: 'item1'}]); + + // Change dependencies without changing underlying data + await act(async () => { + filterIds = ['1', '2']; + rerender(ONYXKEYS.COLLECTION.TEST_KEY); + }); + + // Selector should recompute and return items with id '1' and '2' + expect(result.current[0]).toEqual([ + {id: '1', value: 'item1'}, + {id: '2', value: 'item2'}, + ]); + expect(selectorCallCount).toBeGreaterThan(initialCallCount); + + // Record count after first dependency change + const firstChangeCallCount = selectorCallCount; + + // Change dependencies again + await act(async () => { + filterIds = ['2', '3']; + rerender(ONYXKEYS.COLLECTION.TEST_KEY); + }); + + // Selector should recompute and return items with id '2' and '3' + expect(result.current[0]).toEqual([ + {id: '2', value: 'item2'}, + {id: '3', value: 'item3'}, + ]); + expect(selectorCallCount).toBeGreaterThan(firstChangeCallCount); + }); + + it('should handle complex dependency scenarios with multiple values', async () => { + type TestItem = {id: string; category: string; priority: number}; + const testData = { + [`${ONYXKEYS.COLLECTION.TEST_KEY}item1`]: {id: 'item1', category: 'A', priority: 1}, + [`${ONYXKEYS.COLLECTION.TEST_KEY}item2`]: {id: 'item2', category: 'B', priority: 2}, + [`${ONYXKEYS.COLLECTION.TEST_KEY}item3`]: {id: 'item3', category: 'A', priority: 3}, + [`${ONYXKEYS.COLLECTION.TEST_KEY}item4`]: {id: 'item4', category: 'B', priority: 4}, + }; + + await act(async () => Onyx.mergeCollection(ONYXKEYS.COLLECTION.TEST_KEY, testData as GenericCollection)); + + let categoryFilter = 'A'; + let sortAscending = true; + + const {result, rerender} = renderHook(() => + useOnyx( + ONYXKEYS.COLLECTION.TEST_KEY, + { + selector: (collection) => { + const typedCollection = collection as OnyxCollection; + if (!typedCollection) return []; + + const filtered = Object.values(typedCollection).filter((item) => item?.category === categoryFilter); + + return filtered.sort((a, b) => (sortAscending ? (a?.priority ?? 0) - (b?.priority ?? 0) : (b?.priority ?? 0) - (a?.priority ?? 0))); + }, + }, + [categoryFilter, sortAscending], + ), + ); + + await act(async () => waitForPromisesToResolve()); + + // Should return category A items sorted ascending + expect(result.current[0]).toEqual([ + {id: 'item1', category: 'A', priority: 1}, + {id: 'item3', category: 'A', priority: 3}, + ]); + + // Change sort order only + await act(async () => { + sortAscending = false; + rerender(ONYXKEYS.COLLECTION.TEST_KEY); + }); + + // Should return category A items sorted descending + expect(result.current[0]).toEqual([ + {id: 'item3', category: 'A', priority: 3}, + {id: 'item1', category: 'A', priority: 1}, + ]); + + // Change category filter + await act(async () => { + categoryFilter = 'B'; + rerender(ONYXKEYS.COLLECTION.TEST_KEY); + }); + + // Should return category B items sorted descending + expect(result.current[0]).toEqual([ + {id: 'item4', category: 'B', priority: 4}, + {id: 'item2', category: 'B', priority: 2}, + ]); + }); + + it('should not trigger unnecessary recomputations when dependencies remain the same', async () => { + await act(async () => Onyx.set(ONYXKEYS.TEST_KEY, {value: 'test'})); + + const dependencies = ['constant']; + let selectorCallCount = 0; + + const {result, rerender} = renderHook(() => + useOnyx( + ONYXKEYS.TEST_KEY, + { + selector: (data) => { + selectorCallCount++; + return `${dependencies.join(',')}:${(data as {value?: string})?.value}`; + }, + }, + dependencies, + ), + ); + + await act(async () => waitForPromisesToResolve()); + + expect(result.current[0]).toBe('constant:test'); + expect(selectorCallCount).toBe(1); + + // Force rerender without changing dependencies + await act(async () => { + rerender(ONYXKEYS.COLLECTION.TEST_KEY); + }); + + // Selector should not recompute since dependencies haven't changed + expect(result.current[0]).toBe('constant:test'); + expect(selectorCallCount).toBe(1); + + // Update underlying data + await act(async () => Onyx.merge(ONYXKEYS.TEST_KEY, {value: 'updated'})); + + // Selector should recompute due to data change + expect(result.current[0]).toBe('constant:updated'); + expect(selectorCallCount).toBe(2); + }); }); describe('allowStaleData', () => {