From c24b235616235ecc766088dc5bad6a2bea215f9e Mon Sep 17 00:00:00 2001 From: Tomasz Misiukiewicz Date: Tue, 5 Aug 2025 09:49:58 +0200 Subject: [PATCH 01/11] fix infinite loop --- lib/useOnyx.ts | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/lib/useOnyx.ts b/lib/useOnyx.ts index 031c2194f..62e62dd24 100644 --- a/lib/useOnyx.ts +++ b/lib/useOnyx.ts @@ -157,10 +157,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 (deepEqual(previousDependenciesRef.current, dependencies)) { + return; + } + + previousDependenciesRef.current = dependencies; + if (connectionRef.current === null || isConnectingRef.current || !onStoreChangeFnRef.current) { return; } From ce8ae856f20e36af938bb9204cee62bfd5c2e1c0 Mon Sep 17 00:00:00 2001 From: Tomasz Misiukiewicz Date: Tue, 5 Aug 2025 11:30:33 +0200 Subject: [PATCH 02/11] recompute selector when dependency change --- lib/useOnyx.ts | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/lib/useOnyx.ts b/lib/useOnyx.ts index 62e62dd24..7e9f586a7 100644 --- a/lib/useOnyx.ts +++ b/lib/useOnyx.ts @@ -75,30 +75,43 @@ function useOnyx>( const connectionRef = useRef(null); const previousKey = usePrevious(key); + const currentDependenciesRef = useRef(dependencies); + currentDependenciesRef.current = dependencies; + + const currentSelectorRef = useRef(options?.selector); + currentSelectorRef.current = 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); + const currentDependencies = currentDependenciesRef.current; + const currentSelector = currentSelectorRef.current; + + // Recompute if input changed, dependencies changed, or first time + const dependenciesChanged = !deepEqual(lastDependencies, currentDependencies); + if (!hasComputed || lastInput !== input || dependenciesChanged) { + const newOutput = currentSelector!(input); // Deep equality mode: only update if output actually changed - if (!hasComputed || !deepEqual(lastOutput, newOutput)) { + if (!hasComputed || !deepEqual(lastOutput, newOutput) || dependenciesChanged) { lastInput = input; lastOutput = newOutput; + lastDependencies = [...currentDependencies]; hasComputed = true; } } return lastOutput; }; - }, [options?.selector]); + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [options?.selector, ...dependencies]); // 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. From c0c2628d72ad44faeefe4adcfa0ed4dd2494c328 Mon Sep 17 00:00:00 2001 From: Tomasz Misiukiewicz Date: Tue, 5 Aug 2025 11:59:08 +0200 Subject: [PATCH 03/11] add unit tests --- tests/unit/useOnyxTest.ts | 216 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 216 insertions(+) diff --git a/tests/unit/useOnyxTest.ts b/tests/unit/useOnyxTest.ts index d1ba29235..b7844c69a 100644 --- a/tests/unit/useOnyxTest.ts +++ b/tests/unit/useOnyxTest.ts @@ -536,6 +536,222 @@ 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); + }); + + it('should handle dependencies with deep equality changes', async () => { + await act(async () => Onyx.set(ONYXKEYS.TEST_KEY, {items: ['a', 'b', 'c']})); + + let config = {includeItems: ['a', 'b']}; + let selectorCallCount = 0; + + const {result, rerender} = renderHook(() => + useOnyx( + ONYXKEYS.TEST_KEY, + { + selector: (data) => { + selectorCallCount++; + const typedData = data as {items?: string[]}; + if (!typedData?.items) return []; + return typedData.items.filter((item: string) => config.includeItems.includes(item)); + }, + }, + [config], + ), + ); + + await act(async () => waitForPromisesToResolve()); + + expect(result.current[0]).toEqual(['a', 'b']); + expect(selectorCallCount).toBe(1); + + // Change config to new object with same content + await act(async () => { + config = {includeItems: ['a', 'b']}; + rerender(ONYXKEYS.COLLECTION.TEST_KEY); + }); + + // Should not recompute since deep equality shows no change + expect(result.current[0]).toEqual(['a', 'b']); + expect(selectorCallCount).toBe(1); + + // Change config content + await act(async () => { + config = {includeItems: ['b', 'c']}; + rerender(ONYXKEYS.COLLECTION.TEST_KEY); + }); + + // Should recompute due to content change + expect(result.current[0]).toEqual(['b', 'c']); + expect(selectorCallCount).toBe(2); + }); }); describe('allowStaleData', () => { From 079d3b284fac4974dbea49a0c79df6f0308ecdac Mon Sep 17 00:00:00 2001 From: Tomasz Misiukiewicz Date: Tue, 5 Aug 2025 15:07:24 +0200 Subject: [PATCH 04/11] remove unnecessary dependency from memoizedSelector --- lib/useOnyx.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/useOnyx.ts b/lib/useOnyx.ts index 7e9f586a7..1fc3d6185 100644 --- a/lib/useOnyx.ts +++ b/lib/useOnyx.ts @@ -110,8 +110,7 @@ function useOnyx>( return lastOutput; }; - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [options?.selector, ...dependencies]); + }, [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. From a0c2ce8e08e5b5a64978353ced43242687478572 Mon Sep 17 00:00:00 2001 From: Tomasz Misiukiewicz Date: Thu, 7 Aug 2025 15:28:49 +0200 Subject: [PATCH 05/11] remove unnecessary test --- tests/unit/useOnyxTest.ts | 41 --------------------------------------- 1 file changed, 41 deletions(-) diff --git a/tests/unit/useOnyxTest.ts b/tests/unit/useOnyxTest.ts index b7844c69a..237965b3d 100644 --- a/tests/unit/useOnyxTest.ts +++ b/tests/unit/useOnyxTest.ts @@ -465,47 +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'}); From 7c674060d8ab104ec51676a3075114e1a2d0a69b Mon Sep 17 00:00:00 2001 From: Tomasz Misiukiewicz Date: Thu, 7 Aug 2025 15:32:39 +0200 Subject: [PATCH 06/11] fix lint --- tests/unit/useOnyxTest.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/unit/useOnyxTest.ts b/tests/unit/useOnyxTest.ts index 237965b3d..dc378bab8 100644 --- a/tests/unit/useOnyxTest.ts +++ b/tests/unit/useOnyxTest.ts @@ -465,7 +465,6 @@ describe('useOnyx', () => { expect(result.current[0]).toBe(firstResult); }); - it('should memoize primitive selector results correctly', async () => { Onyx.set(ONYXKEYS.TEST_KEY, {count: 5, name: 'test'}); From 2fdd275d09af88c9a437a482b91d1c564f2ef183 Mon Sep 17 00:00:00 2001 From: Tomasz Misiukiewicz Date: Mon, 11 Aug 2025 15:07:14 +0200 Subject: [PATCH 07/11] use useLiveRef --- lib/useOnyx.ts | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/lib/useOnyx.ts b/lib/useOnyx.ts index 1fc3d6185..3d30d4ef0 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,11 +76,8 @@ function useOnyx>( const connectionRef = useRef(null); const previousKey = usePrevious(key); - const currentDependenciesRef = useRef(dependencies); - currentDependenciesRef.current = dependencies; - - const currentSelectorRef = useRef(options?.selector); - currentSelectorRef.current = options?.selector; + const currentDependenciesRef = useLiveRef(dependencies); + const currentSelectorRef = useLiveRef(options?.selector); // Create memoized version of selector for performance const memoizedSelector = useMemo(() => { From 53199e30ded527f7e0b1e4e7f386b5eca92970fc Mon Sep 17 00:00:00 2001 From: Tomasz Misiukiewicz Date: Mon, 11 Aug 2025 15:08:25 +0200 Subject: [PATCH 08/11] update assertion --- lib/useOnyx.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/useOnyx.ts b/lib/useOnyx.ts index 3d30d4ef0..7a8d95ce4 100644 --- a/lib/useOnyx.ts +++ b/lib/useOnyx.ts @@ -95,7 +95,7 @@ function useOnyx>( // Recompute if input changed, dependencies changed, or first time const dependenciesChanged = !deepEqual(lastDependencies, currentDependencies); if (!hasComputed || lastInput !== input || dependenciesChanged) { - const newOutput = currentSelector!(input); + const newOutput = currentSelector?.(input); // Deep equality mode: only update if output actually changed if (!hasComputed || !deepEqual(lastOutput, newOutput) || dependenciesChanged) { From c891e5552ff0310cb138de2676bfff69a1b5c50f Mon Sep 17 00:00:00 2001 From: Tomasz Misiukiewicz Date: Mon, 11 Aug 2025 15:26:44 +0200 Subject: [PATCH 09/11] fix currentSelector assertion --- lib/useOnyx.ts | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/lib/useOnyx.ts b/lib/useOnyx.ts index 7a8d95ce4..e9c894805 100644 --- a/lib/useOnyx.ts +++ b/lib/useOnyx.ts @@ -95,20 +95,23 @@ function useOnyx>( // Recompute if input changed, dependencies changed, or first time const dependenciesChanged = !deepEqual(lastDependencies, currentDependencies); if (!hasComputed || lastInput !== input || dependenciesChanged) { - 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; + // 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. From 999e7b8438ba9cd7832b35f395e16f28f5fbb42f Mon Sep 17 00:00:00 2001 From: Tomasz Misiukiewicz Date: Tue, 12 Aug 2025 09:08:06 +0200 Subject: [PATCH 10/11] use shallowEqual for useOnyx dependenceis --- lib/useOnyx.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/useOnyx.ts b/lib/useOnyx.ts index e9c894805..b2c060e5d 100644 --- a/lib/useOnyx.ts +++ b/lib/useOnyx.ts @@ -93,7 +93,7 @@ function useOnyx>( const currentSelector = currentSelectorRef.current; // Recompute if input changed, dependencies changed, or first time - const dependenciesChanged = !deepEqual(lastDependencies, currentDependencies); + const dependenciesChanged = !shallowEqual(lastDependencies, currentDependencies); if (!hasComputed || lastInput !== input || dependenciesChanged) { // Only proceed if we have a valid selector if (currentSelector) { @@ -180,7 +180,7 @@ function useOnyx>( // Deep equality check to prevent infinite loops when dependencies array reference changes // but content remains the same - if (deepEqual(previousDependenciesRef.current, dependencies)) { + if (shallowEqual(previousDependenciesRef.current, dependencies)) { return; } From f36f50a6a23efdf6037b22775ee17ac9094ede40 Mon Sep 17 00:00:00 2001 From: Tomasz Misiukiewicz Date: Tue, 12 Aug 2025 09:09:14 +0200 Subject: [PATCH 11/11] remove unnecessary test --- tests/unit/useOnyxTest.ts | 47 --------------------------------------- 1 file changed, 47 deletions(-) diff --git a/tests/unit/useOnyxTest.ts b/tests/unit/useOnyxTest.ts index dc378bab8..9ea08a5be 100644 --- a/tests/unit/useOnyxTest.ts +++ b/tests/unit/useOnyxTest.ts @@ -663,53 +663,6 @@ describe('useOnyx', () => { expect(result.current[0]).toBe('constant:updated'); expect(selectorCallCount).toBe(2); }); - - it('should handle dependencies with deep equality changes', async () => { - await act(async () => Onyx.set(ONYXKEYS.TEST_KEY, {items: ['a', 'b', 'c']})); - - let config = {includeItems: ['a', 'b']}; - let selectorCallCount = 0; - - const {result, rerender} = renderHook(() => - useOnyx( - ONYXKEYS.TEST_KEY, - { - selector: (data) => { - selectorCallCount++; - const typedData = data as {items?: string[]}; - if (!typedData?.items) return []; - return typedData.items.filter((item: string) => config.includeItems.includes(item)); - }, - }, - [config], - ), - ); - - await act(async () => waitForPromisesToResolve()); - - expect(result.current[0]).toEqual(['a', 'b']); - expect(selectorCallCount).toBe(1); - - // Change config to new object with same content - await act(async () => { - config = {includeItems: ['a', 'b']}; - rerender(ONYXKEYS.COLLECTION.TEST_KEY); - }); - - // Should not recompute since deep equality shows no change - expect(result.current[0]).toEqual(['a', 'b']); - expect(selectorCallCount).toBe(1); - - // Change config content - await act(async () => { - config = {includeItems: ['b', 'c']}; - rerender(ONYXKEYS.COLLECTION.TEST_KEY); - }); - - // Should recompute due to content change - expect(result.current[0]).toEqual(['b', 'c']); - expect(selectorCallCount).toBe(2); - }); }); describe('allowStaleData', () => {