diff --git a/lib/createMemoizedSelector.ts b/lib/createMemoizedSelector.ts new file mode 100644 index 000000000..3dbdafbff --- /dev/null +++ b/lib/createMemoizedSelector.ts @@ -0,0 +1,38 @@ +import {deepEqual} from 'fast-equals'; + +/** + * Wraps a selector function so that: + * - Calling the wrapper with the same input reference twice short-circuits to the cached output + * (cheap `===` check, no recompute). + * - Calling with a different input that produces a deep-equal output returns the *previous* + * output reference, so downstream `===` comparisons treat it as unchanged. + * + * This is the minimum needed for `useSyncExternalStore` to not loop when consumers pass + * inline selectors that allocate fresh objects on every call (e.g. `(e) => ({id: e?.id})`): + * without the deep-equal fallback, every `getSnapshot` would return a new reference and React + * would re-render (or throw "getSnapshot should be cached") indefinitely. + * + * Stateful by design — each call to `createMemoizedSelector` produces an independent wrapper + * with its own `lastInput`/`lastOutput` cache, so a wrapper must not be shared across + * subscriptions that can see different inputs. + */ +function createMemoizedSelector(selector: (input: TInput) => TOutput): (input: TInput) => TOutput { + let lastInput: TInput; + let lastOutput: TOutput; + let hasComputed = false; + + return (input) => { + if (hasComputed && lastInput === input) { + return lastOutput; + } + const next = selector(input); + lastInput = input; + if (!hasComputed || !deepEqual(lastOutput, next)) { + lastOutput = next; + hasComputed = true; + } + return lastOutput; + }; +} + +export default createMemoizedSelector; diff --git a/lib/useLiveRef.ts b/lib/useLiveRef.ts deleted file mode 100644 index 869f439db..000000000 --- a/lib/useLiveRef.ts +++ /dev/null @@ -1,17 +0,0 @@ -import {useRef} from 'react'; - -/** - * Creates a mutable reference to a value, useful when you need to - * maintain a reference to a value that may change over time without triggering re-renders. - * - * @deprecated This hook breaks the Rules of React, and should not be used. - * The migration effort to remove it safely is not currently planned. - */ -function useLiveRef(value: T) { - const ref = useRef(value); - ref.current = value; - - return ref; -} - -export default useLiveRef; diff --git a/lib/useOnyx.ts b/lib/useOnyx.ts index 984c25254..ffc3e31c1 100644 --- a/lib/useOnyx.ts +++ b/lib/useOnyx.ts @@ -1,6 +1,6 @@ -import {deepEqual, shallowEqual} from 'fast-equals'; +import {shallowEqual} from 'fast-equals'; import {useCallback, useEffect, useMemo, useRef, useSyncExternalStore} from 'react'; -import type {DependencyList} from 'react'; +import createMemoizedSelector from './createMemoizedSelector'; import OnyxCache, {TASK} from './OnyxCache'; import type {Connection} from './OnyxConnectionManager'; import connectionManager from './OnyxConnectionManager'; @@ -8,7 +8,6 @@ import OnyxUtils from './OnyxUtils'; import OnyxKeys from './OnyxKeys'; import type {CollectionKeyBase, OnyxKey, OnyxValue} from './types'; import onyxSnapshotCache from './OnyxSnapshotCache'; -import useLiveRef from './useLiveRef'; type UseOnyxSelector> = (data: OnyxValue | undefined) => TReturnValue; @@ -38,49 +37,19 @@ type ResultMetadata = { type UseOnyxResult = [NonNullable | undefined, ResultMetadata]; -function useOnyx>( - key: TKey, - options?: UseOnyxOptions, - dependencies: DependencyList = [], -): UseOnyxResult { +function useOnyx>(key: TKey, options?: UseOnyxOptions): UseOnyxResult { const connectionRef = useRef(null); - const currentDependenciesRef = useLiveRef(dependencies); const selector = options?.selector; - // Create memoized version of selector for performance + // Create memoized version of selector for performance. It caches by input reference + // with a deepEqual fallback on the output to keep the returned reference stable. const memoizedSelector = useMemo((): UseOnyxSelector | null => { if (!selector) { return null; } - let lastInput: OnyxValue | undefined; - let lastOutput: TReturnValue; - let lastDependencies: DependencyList = []; - let hasComputed = false; - - return (input: OnyxValue | undefined): TReturnValue => { - const currentDependencies = currentDependenciesRef.current; - - // Recompute if input changed, dependencies changed, or first time - const dependenciesChanged = !shallowEqual(lastDependencies, currentDependencies); - if (!hasComputed || lastInput !== input || dependenciesChanged) { - const newOutput = selector(input); - - // Always track the current input to avoid re-running the selector - // when the same input is seen again (even if the output didn't change). - lastInput = input; - - // Only update the output reference if it actually changed - if (!hasComputed || !deepEqual(lastOutput, newOutput) || dependenciesChanged) { - lastOutput = newOutput; - lastDependencies = [...currentDependencies]; - hasComputed = true; - } - } - - return lastOutput; - }; - }, [currentDependenciesRef, selector]); + return createMemoizedSelector(selector); + }, [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. @@ -132,33 +101,6 @@ function useOnyx>( useEffect(() => () => onyxSnapshotCache.deregisterConsumer(key, cacheKey), [key, cacheKey]); - // 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 || connectedKeyRef.current !== key || !onStoreChangeFnRef.current) { - return; - } - - // Invalidate cache when dependencies change so selector runs with new closure values - onyxSnapshotCache.invalidateForKey(key); - shouldGetCachedValueRef.current = true; - onStoreChangeFnRef.current(); - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [...dependencies]); - // Tracks the last memoizedSelector reference that getSnapshot() has computed with. // When the selector changes, this mismatch forces getSnapshot() to re-evaluate // even if all other conditions (isFirstConnection, shouldGetCachedValue, key) are false. diff --git a/tests/unit/createMemoizedSelectorTest.ts b/tests/unit/createMemoizedSelectorTest.ts new file mode 100644 index 000000000..445911181 --- /dev/null +++ b/tests/unit/createMemoizedSelectorTest.ts @@ -0,0 +1,129 @@ +import createMemoizedSelector from '../../lib/createMemoizedSelector'; + +describe('createMemoizedSelector', () => { + it('computes the output on the first call', () => { + const selector = jest.fn((input: number) => input * 2); + const memoized = createMemoizedSelector(selector); + + expect(memoized(21)).toBe(42); + expect(selector).toHaveBeenCalledTimes(1); + }); + + it('short-circuits without recomputing when called with the same input reference', () => { + const input = {value: 1}; + const selector = jest.fn((data: {value: number}) => ({doubled: data.value * 2})); + const memoized = createMemoizedSelector(selector); + + const first = memoized(input); + const second = memoized(input); + + // Same input reference → selector not called again, same output reference returned. + expect(selector).toHaveBeenCalledTimes(1); + expect(second).toBe(first); + }); + + it('recomputes when the input reference changes', () => { + const selector = jest.fn((data: {value: number}) => data.value * 10); + const memoized = createMemoizedSelector(selector); + + expect(memoized({value: 1})).toBe(10); + expect(memoized({value: 2})).toBe(20); + expect(selector).toHaveBeenCalledTimes(2); + }); + + it('returns the previous output reference when a new input produces a deep-equal output', () => { + // New object input every call, but the selector output is structurally identical. + const selector = (data: {id: number; name: string}) => ({id: data.id}); + const memoized = createMemoizedSelector(selector); + + const first = memoized({id: 1, name: 'a'}); + const second = memoized({id: 1, name: 'b'}); // different input, deep-equal output {id: 1} + + // Output is deep-equal, so the *previous* reference is preserved for `===` consumers. + expect(second).toBe(first); + expect(second).toEqual({id: 1}); + }); + + it('returns a new output reference when a new input produces a deep-unequal output', () => { + const selector = (data: {id: number}) => ({id: data.id}); + const memoized = createMemoizedSelector(selector); + + const first = memoized({id: 1}); + const second = memoized({id: 2}); + + expect(second).not.toBe(first); + expect(second).toEqual({id: 2}); + }); + + it('preserves the output reference across an A → B(deep-equal A) → A sequence', () => { + const selector = (data: {id: number; extra: string}) => ({id: data.id}); + const memoized = createMemoizedSelector(selector); + + const a = memoized({id: 1, extra: 'x'}); + const b = memoized({id: 1, extra: 'y'}); // deep-equal output, keeps `a` + const c = memoized({id: 1, extra: 'z'}); // deep-equal output, keeps `a` + + expect(b).toBe(a); + expect(c).toBe(a); + }); + + it('handles primitive outputs', () => { + const selector = jest.fn((data: {n: number}) => data.n > 0); + const memoized = createMemoizedSelector(selector); + + expect(memoized({n: 1})).toBe(true); + // Different input, same boolean output — deepEqual(true, true) is true, value preserved. + expect(memoized({n: 5})).toBe(true); + expect(memoized({n: -1})).toBe(false); + expect(selector).toHaveBeenCalledTimes(3); + }); + + it('handles undefined input and undefined output', () => { + const selector = jest.fn((data: {x: number} | undefined) => data?.x); + const memoized = createMemoizedSelector(selector); + + expect(memoized(undefined)).toBeUndefined(); + // Same undefined input reference → short-circuits. + expect(memoized(undefined)).toBeUndefined(); + expect(selector).toHaveBeenCalledTimes(1); + }); + + it('treats the first call as a real computation even when the output is undefined', () => { + const selector = jest.fn(() => undefined); + const memoized = createMemoizedSelector(selector); + + const input1 = {a: 1}; + const input2 = {a: 2}; + + expect(memoized(input1)).toBeUndefined(); + expect(memoized(input2)).toBeUndefined(); + // Both inputs differ by reference, but both outputs are undefined (deep-equal) — recomputed + // on the second call, then collapsed to the preserved reference (both undefined anyway). + expect(selector).toHaveBeenCalledTimes(2); + }); + + it('keeps independent caches per wrapper instance', () => { + const selectorA = jest.fn((n: number) => n + 1); + const selectorB = jest.fn((n: number) => n + 100); + const memoizedA = createMemoizedSelector(selectorA); + const memoizedB = createMemoizedSelector(selectorB); + + expect(memoizedA(1)).toBe(2); + expect(memoizedB(1)).toBe(101); + expect(selectorA).toHaveBeenCalledTimes(1); + expect(selectorB).toHaveBeenCalledTimes(1); + }); + + it('preserves nested object reference identity on deep-equal recompute', () => { + const selector = (data: {id: number}) => ({meta: {id: data.id}, items: [data.id]}); + const memoized = createMemoizedSelector(selector); + + const first = memoized({id: 7}); + const second = memoized({id: 7}); // new input ref, deep-equal output + + // Whole output reference preserved, so nested members are reference-stable too. + expect(second).toBe(first); + expect(second.meta).toBe(first.meta); + expect(second.items).toBe(first.items); + }); +}); diff --git a/tests/unit/useOnyxTest.ts b/tests/unit/useOnyxTest.ts index d5b9d0017..385e285d9 100644 --- a/tests/unit/useOnyxTest.ts +++ b/tests/unit/useOnyxTest.ts @@ -713,176 +713,6 @@ 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 selector = ((data) => { - selectorCallCount++; - return `${dependencies.join(',')}:${(data as {value?: string})?.value}`; - }) as UseOnyxSelector; - - const {result, rerender} = renderHook(() => - useOnyx( - ONYXKEYS.TEST_KEY, - { - selector, - }, - 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('pending merges', () => { @@ -969,54 +799,6 @@ describe('useOnyx', () => { }); }); - describe('dependencies', () => { - it('should return the updated selected value when a external value passed to the dependencies list changes', async () => { - Onyx.mergeCollection(ONYXKEYS.COLLECTION.TEST_KEY, { - [`${ONYXKEYS.COLLECTION.TEST_KEY}entry1`]: {id: 'entry1_id', name: 'entry1_name'}, - [`${ONYXKEYS.COLLECTION.TEST_KEY}entry2`]: {id: 'entry2_id', name: 'entry2_name'}, - [`${ONYXKEYS.COLLECTION.TEST_KEY}entry3`]: {id: 'entry3_id', name: 'entry3_name'}, - } as GenericCollection); - - let externalValue = 'ex1'; - - const {result, rerender} = renderHook(() => - useOnyx( - ONYXKEYS.COLLECTION.TEST_KEY, - { - selector: ((entries: OnyxCollection<{id: string; name: string}>) => - Object.entries(entries ?? {}).reduce>>((acc, [key, value]) => { - acc[key] = `${value?.id}_${externalValue}`; - return acc; - }, {})) as UseOnyxSelector>>, - }, - [externalValue], - ), - ); - - await act(async () => waitForPromisesToResolve()); - - expect(result.current[0]).toEqual({ - [`${ONYXKEYS.COLLECTION.TEST_KEY}entry1`]: 'entry1_id_ex1', - [`${ONYXKEYS.COLLECTION.TEST_KEY}entry2`]: 'entry2_id_ex1', - [`${ONYXKEYS.COLLECTION.TEST_KEY}entry3`]: 'entry3_id_ex1', - }); - expect(result.current[1].status).toEqual('loaded'); - - externalValue = 'ex2'; - - await act(async () => { - rerender(undefined); - }); - - expect(result.current[0]).toEqual({ - [`${ONYXKEYS.COLLECTION.TEST_KEY}entry1`]: 'entry1_id_ex2', - [`${ONYXKEYS.COLLECTION.TEST_KEY}entry2`]: 'entry2_id_ex2', - [`${ONYXKEYS.COLLECTION.TEST_KEY}entry3`]: 'entry3_id_ex2', - }); - expect(result.current[1].status).toEqual('loaded'); - }); - }); - describe('skippable collection member ids', () => { it('should always return undefined entry when subscribing to a collection with skippable member ids', async () => { Onyx.mergeCollection(ONYXKEYS.COLLECTION.TEST_KEY, {