From bb8a025bbcefddad8956926a0651d39497133006 Mon Sep 17 00:00:00 2001 From: Tomasz Misiukiewicz Date: Thu, 24 Jul 2025 09:06:01 +0200 Subject: [PATCH 1/3] add memoizedSelector --- lib/useOnyx.ts | 38 +++++++++++++++++++++++++++++--------- 1 file changed, 29 insertions(+), 9 deletions(-) diff --git a/lib/useOnyx.ts b/lib/useOnyx.ts index 8432e7ea1..0aaa0a6da 100644 --- a/lib/useOnyx.ts +++ b/lib/useOnyx.ts @@ -7,7 +7,6 @@ import connectionManager from './OnyxConnectionManager'; import OnyxUtils from './OnyxUtils'; import * as GlobalSettings from './GlobalSettings'; import type {CollectionKeyBase, OnyxKey, OnyxValue} from './types'; -import useLiveRef from './useLiveRef'; import usePrevious from './usePrevious'; import decorateWithMetrics from './metrics'; import * as Logger from './Logger'; @@ -76,8 +75,30 @@ function useOnyx>( const connectionRef = useRef(null); const previousKey = usePrevious(key); - // Used to stabilize the selector reference and avoid unnecessary calls to `getSnapshot()`. - const selectorRef = 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 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; + } + } + + return lastOutput; + }; + }, [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. @@ -180,7 +201,7 @@ function useOnyx>( if (isFirstConnectionRef.current || shouldGetCachedValueRef.current) { // 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 = selectorRef.current ? selectorRef.current(value) : value; + const selectedValue = memoizedSelector ? memoizedSelector(value) : value; newValueRef.current = (selectedValue ?? undefined) as TReturnValue | undefined; // This flag is `false` when the original Onyx value (without selector) is not defined yet. @@ -205,11 +226,10 @@ function useOnyx>( newFetchStatus = 'loading'; } - // We do a deep equality check if `selector` is defined, since each `tryGetCachedValue()` call will - // generate a plain new primitive/object/array that was created using the `selector` function. - // For the other cases we will only deal with object reference checks, so just a shallow equality check is enough. + // Use deep equality for all selectors to maintain existing behavior + // TODO: Optimize based on memoizeOutput setting once we resolve the loading state issue let areValuesEqual: boolean; - if (selectorRef.current) { + if (memoizedSelector) { areValuesEqual = deepEqual(previousValueRef.current ?? undefined, newValueRef.current); } else { areValuesEqual = shallowEqual(previousValueRef.current ?? undefined, newValueRef.current); @@ -244,7 +264,7 @@ function useOnyx>( } return resultRef.current; - }, [options?.initWithStoredValues, options?.allowStaleData, options?.canBeMissing, key, selectorRef]); + }, [options?.initWithStoredValues, options?.allowStaleData, options?.canBeMissing, key, memoizedSelector]); const subscribe = useCallback( (onStoreChange: () => void) => { From edb9b9f7bbb65e4da7d9862bae8e7295cbbf2770 Mon Sep 17 00:00:00 2001 From: Tomasz Misiukiewicz Date: Thu, 24 Jul 2025 09:30:35 +0200 Subject: [PATCH 2/3] remove redundant deepEqual --- lib/useOnyx.ts | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/lib/useOnyx.ts b/lib/useOnyx.ts index 0aaa0a6da..031c2194f 100644 --- a/lib/useOnyx.ts +++ b/lib/useOnyx.ts @@ -226,11 +226,15 @@ function useOnyx>( newFetchStatus = 'loading'; } - // Use deep equality for all selectors to maintain existing behavior - // TODO: Optimize based on memoizeOutput setting once we resolve the loading state issue + // Optimized equality checking - eliminated redundant deep equality: + // - Memoized selectors already handle deep equality internally, so we can use fast reference equality + // - Non-selector cases use shallow equality for object reference checks + // - Normalize null to undefined to ensure consistent comparison (both represent "no value") let areValuesEqual: boolean; if (memoizedSelector) { - areValuesEqual = deepEqual(previousValueRef.current ?? undefined, newValueRef.current); + const normalizedPrevious = previousValueRef.current ?? undefined; + const normalizedNew = newValueRef.current ?? undefined; + areValuesEqual = normalizedPrevious === normalizedNew; } else { areValuesEqual = shallowEqual(previousValueRef.current ?? undefined, newValueRef.current); } From f6a4aa43b8ef3e55c381b29928219b0af203b3b3 Mon Sep 17 00:00:00 2001 From: Tomasz Misiukiewicz Date: Tue, 29 Jul 2025 10:10:20 +0200 Subject: [PATCH 3/3] add tests for memoized selectors --- tests/unit/useOnyxTest.ts | 150 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 150 insertions(+) diff --git a/tests/unit/useOnyxTest.ts b/tests/unit/useOnyxTest.ts index 7618ec9a9..d1ba29235 100644 --- a/tests/unit/useOnyxTest.ts +++ b/tests/unit/useOnyxTest.ts @@ -386,6 +386,156 @@ describe('useOnyx', () => { expect(result.current[0]).toEqual('id - test_id, name - test_name - selector changed'); expect(result.current[1].status).toEqual('loaded'); }); + + it('should memoize selector output and return same reference when input unchanged', async () => { + Onyx.set(ONYXKEYS.TEST_KEY, {id: 'test_id', name: 'test_name', count: 1}); + + const {result} = renderHook(() => + useOnyx(ONYXKEYS.TEST_KEY, { + // @ts-expect-error bypass + selector: (entry: OnyxEntry<{id: string; name: string; count: number}>) => ({ + id: entry?.id, + name: entry?.name, + }), + }), + ); + + await act(async () => waitForPromisesToResolve()); + + const firstResult = result.current[0]; + + // Trigger another render without changing the data + await act(async () => waitForPromisesToResolve()); + + // Should return the exact same reference due to memoization + expect(result.current[0]).toBe(firstResult); + }); + + it('should return new reference when selector input changes', async () => { + Onyx.set(ONYXKEYS.TEST_KEY, {id: 'test_id', name: 'test_name'}); + + const {result} = renderHook(() => + useOnyx(ONYXKEYS.TEST_KEY, { + // @ts-expect-error bypass + selector: (entry: OnyxEntry<{id: string; name: string}>) => ({ + id: entry?.id, + name: entry?.name, + }), + }), + ); + + await act(async () => waitForPromisesToResolve()); + + const firstResult = result.current[0]; + + // Change the data + await act(async () => Onyx.merge(ONYXKEYS.TEST_KEY, {id: 'changed_id'})); + + // Should return a new reference since data changed + expect(result.current[0]).not.toBe(firstResult); + expect(result.current[0]).toEqual({id: 'changed_id', name: 'test_name'}); + }); + + it('should memoize selector output using deep equality check', async () => { + let selectorCallCount = 0; + + Onyx.set(ONYXKEYS.TEST_KEY, {id: 'test_id', name: 'test_name'}); + + const {result} = renderHook(() => + useOnyx(ONYXKEYS.TEST_KEY, { + // @ts-expect-error bypass + selector: (entry: OnyxEntry<{id: string; name: string}>) => { + selectorCallCount++; + return {id: entry?.id, name: entry?.name}; + }, + }), + ); + + await act(async () => waitForPromisesToResolve()); + + const firstResult = result.current[0]; + const initialCallCount = selectorCallCount; + + // Add a property that will change object reference but keep selected data same + await act(async () => Onyx.merge(ONYXKEYS.TEST_KEY, {extraProp: 'new'})); + + // Selector should be called again due to input object reference change + expect(selectorCallCount).toBe(initialCallCount + 1); + // But output should be the same reference due to deep equality check in memoized selector + 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'}); + + const {result} = renderHook(() => + useOnyx(ONYXKEYS.TEST_KEY, { + // @ts-expect-error bypass + selector: (entry: OnyxEntry<{count: number; name: string}>) => entry?.count || 0, + }), + ); + + await act(async () => waitForPromisesToResolve()); + + const firstResult = result.current[0]; + expect(firstResult).toBe(5); + + // Change unrelated property + await act(async () => Onyx.merge(ONYXKEYS.TEST_KEY, {name: 'changed'})); + + // Should return the same primitive value (number 5) + expect(result.current[0]).toBe(firstResult); + expect(result.current[0]).toBe(5); + + // Change the selected property + await act(async () => Onyx.merge(ONYXKEYS.TEST_KEY, {count: 10})); + + // Should return new value + expect(result.current[0]).not.toBe(firstResult); + expect(result.current[0]).toBe(10); + }); }); describe('allowStaleData', () => {