Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 34 additions & 10 deletions lib/useOnyx.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -76,8 +75,30 @@ function useOnyx<TKey extends OnyxKey, TReturnValue = OnyxValue<TKey>>(
const connectionRef = useRef<Connection | null>(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<TKey> | undefined;
let lastOutput: TReturnValue;
let hasComputed = false;

return (input: OnyxValue<TKey> | 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;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought this should return options?.selector 🤔

If this condition is not passed:

 if (!hasComputed || lastInput !== input) {

Then this useMemo will return undefined.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the input hasn't changed, then there is no point of running a selector - we should expect that for the same input each time we'll get the exact same output. I think there is no way of returning undefined here because those conditions will be met at least once (hasComputed is false by default and selector input will be always different than undefined)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad. I see the point here. Let me do some testings.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just done a round of testing. I can see we can skip a bunch of deepEqual, that's very nice 🎉 .

One question, what do you do to collect the Onyx execution time?

perf.test.mov

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hungvu193 I wrap the code I want to investigate with something like:

const time = performance.now();

...

console.log('Execution time', performance.now() - time);

Alternatively, I use "Performance" tab in Chrome DevTools to record profiles, upload them to https://www.speedscope.app/ and analyze Onyx internals there

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Is there anything else to do before making this as ready?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll do some manual tests today as well and prepare the PR for a review 👍

};
}, [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.
Expand Down Expand Up @@ -180,7 +201,7 @@ function useOnyx<TKey extends OnyxKey, TReturnValue = OnyxValue<TKey>>(
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<TKey>;
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.
Expand All @@ -205,12 +226,15 @@ function useOnyx<TKey extends OnyxKey, TReturnValue = OnyxValue<TKey>>(
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.
// 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 (selectorRef.current) {
areValuesEqual = deepEqual(previousValueRef.current ?? undefined, newValueRef.current);
if (memoizedSelector) {
const normalizedPrevious = previousValueRef.current ?? undefined;
const normalizedNew = newValueRef.current ?? undefined;
areValuesEqual = normalizedPrevious === normalizedNew;
} else {
areValuesEqual = shallowEqual(previousValueRef.current ?? undefined, newValueRef.current);
}
Expand Down Expand Up @@ -244,7 +268,7 @@ function useOnyx<TKey extends OnyxKey, TReturnValue = OnyxValue<TKey>>(
}

return resultRef.current;
}, [options?.initWithStoredValues, options?.allowStaleData, options?.canBeMissing, key, selectorRef]);
}, [options?.initWithStoredValues, options?.allowStaleData, options?.canBeMissing, key, memoizedSelector]);

const subscribe = useCallback(
(onStoreChange: () => void) => {
Expand Down
150 changes: 150 additions & 0 deletions tests/unit/useOnyxTest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<typeof complexObject>) => 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', () => {
Expand Down