Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat: Call useOnyx selector for entire collection #585

Merged
Merged
Show file tree
Hide file tree
Changes from 15 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
78 changes: 56 additions & 22 deletions lib/useOnyx.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,10 @@
import {deepEqual, shallowEqual} from 'fast-equals';
import {useCallback, useEffect, useRef, useSyncExternalStore} from 'react';
import type {IsEqual} from 'type-fest';
import OnyxCache from './OnyxCache';
import type {Connection} from './OnyxConnectionManager';
import connectionManager from './OnyxConnectionManager';
import OnyxUtils from './OnyxUtils';
import type {CollectionKeyBase, OnyxCollection, OnyxEntry, OnyxKey, OnyxValue, Selector} from './types';
import type {CollectionKeyBase, KeyValueMapping, OnyxCollection, OnyxKey, OnyxValue} from './types';
import useLiveRef from './useLiveRef';
import usePrevious from './usePrevious';

Expand Down Expand Up @@ -33,43 +32,78 @@ type UseOnyxInitialValueOption<TInitialValue> = {
initialValue?: TInitialValue;
};

type UseOnyxSelector<TKey extends OnyxKey, TReturnValue = OnyxValue<TKey>> = (data: OnyxValue<TKey> | undefined) => TReturnValue;
kacper-mikolajczak marked this conversation as resolved.
Show resolved Hide resolved

type UseOnyxSelectorOption<TKey extends OnyxKey, TReturnValue> = {
/**
* This will be used to subscribe to a subset of an Onyx key's data.
* Using this setting on `useOnyx` can have very positive performance benefits because the component will only re-render
* when the subset of data changes. Otherwise, any change of data on any property would normally
* cause the component to re-render (and that can be expensive from a performance standpoint).
* @see `useOnyx` cannot return `null` and so selector will replace `null` with `undefined` to maintain compatibility.
*/
selector?: Selector<TKey, unknown, TReturnValue>;
selector?: UseOnyxSelector<TKey, TReturnValue>;
kacper-mikolajczak marked this conversation as resolved.
Show resolved Hide resolved
};

type UseOnyxOptions<TKey extends OnyxKey, TReturnValue> = BaseUseOnyxOptions & UseOnyxInitialValueOption<TReturnValue> & UseOnyxSelectorOption<TKey, TReturnValue>;

type FetchStatus = 'loading' | 'loaded';

type SelectedValue<TKey, TValue> = TKey extends CollectionKeyBase ? OnyxCollection<TValue> : OnyxEntry<TValue>;

type CachedValue<TKey extends OnyxKey, TValue> = IsEqual<TValue, OnyxValue<TKey>> extends true ? TValue : SelectedValue<TKey, TValue>;

type ResultMetadata = {
status: FetchStatus;
};

type UseOnyxResult<TKey extends OnyxKey, TValue> = [CachedValue<TKey, TValue>, ResultMetadata];
type UseOnyxResult<TValue> = [NonNullable<TValue> | undefined, ResultMetadata];

/**
* Gets the cached value from the Onyx cache. If the key is a collection key, it will return all the values in the collection.
* It is a fork of `tryGetCachedValue` from `OnyxUtils` caused by different selector logic in `useOnyx`. It should be unified in the future, when `withOnyx` is removed.
*/
kacper-mikolajczak marked this conversation as resolved.
Show resolved Hide resolved
function tryGetCachedValue<TKey extends OnyxKey>(key: TKey): OnyxValue<OnyxKey> {
if (!OnyxUtils.isCollectionKey(key)) {
return OnyxCache.get(key);
}

const allCacheKeys = OnyxCache.getAllKeys();

// It is possible we haven't loaded all keys yet so we do not know if the
// collection actually exists.
if (allCacheKeys.size === 0) {
return;
}

const values: OnyxCollection<KeyValueMapping[TKey]> = {};
allCacheKeys.forEach((cacheKey) => {
if (!cacheKey.startsWith(key)) {
return;
}

values[cacheKey] = OnyxCache.get(cacheKey);
});

return values;
}

/**
* Gets the value from cache and maps it with selector. It changes `null` to `undefined` for `useOnyx` compatibility.
*/
function getCachedValue<TKey extends OnyxKey, TValue>(key: TKey, selector?: UseOnyxSelector<TKey, TValue>) {
const value = tryGetCachedValue(key) as OnyxValue<TKey>;

const selectedValue = selector ? selector(value) : (value as TValue);

Comment on lines +91 to 94
Copy link
Contributor

@hannojg hannojg Oct 4, 2024

Choose a reason for hiding this comment

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

just wondering for the future if the onyx cache could also have a cache of key + selector function, so that when the selector function remains the same, the data remains the same we don't have to rerun the selector (as selectors could be expensive to run) [maybe this should be opt in though]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for review Hanno!

What came up as a next improvement while working on this issue was the idea to separate selector from useOnyx internals entirely so it won't interfere with Onyx cache and only transform the cached data at the end of the pipeline.

It should have many benefits, but I am still working on the PoC whether it is feasible for us to implement 🤞

function getCachedValue<TKey extends OnyxKey, TValue>(key: TKey, selector?: Selector<TKey, unknown, unknown>): CachedValue<TKey, TValue> | undefined {
return (OnyxUtils.tryGetCachedValue(key, {selector}) ?? undefined) as CachedValue<TKey, TValue> | undefined;
return selectedValue ?? undefined;
}

function useOnyx<TKey extends OnyxKey, TReturnValue = OnyxValue<TKey>>(
key: TKey,
options?: BaseUseOnyxOptions & UseOnyxInitialValueOption<TReturnValue> & Required<UseOnyxSelectorOption<TKey, TReturnValue>>,
): UseOnyxResult<TKey, TReturnValue>;
): UseOnyxResult<TReturnValue>;
function useOnyx<TKey extends OnyxKey, TReturnValue = OnyxValue<TKey>>(
key: TKey,
options?: BaseUseOnyxOptions & UseOnyxInitialValueOption<NoInfer<TReturnValue>>,
): UseOnyxResult<TKey, TReturnValue>;
function useOnyx<TKey extends OnyxKey, TReturnValue = OnyxValue<TKey>>(key: TKey, options?: UseOnyxOptions<TKey, TReturnValue>): UseOnyxResult<TKey, TReturnValue> {
): UseOnyxResult<TReturnValue>;
function useOnyx<TKey extends OnyxKey, TReturnValue = OnyxValue<TKey>>(key: TKey, options?: UseOnyxOptions<TKey, TReturnValue>): UseOnyxResult<TReturnValue> {
const connectionRef = useRef<Connection | null>(null);
const previousKey = usePrevious(key);

Expand All @@ -78,16 +112,16 @@ function useOnyx<TKey extends OnyxKey, TReturnValue = OnyxValue<TKey>>(key: TKey

// 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.
const previousValueRef = useRef<CachedValue<TKey, TReturnValue> | undefined | null>(null);
const previousValueRef = useRef<TReturnValue | undefined | null>(null);

// Stores the newest cached value in order to compare with the previous one and optimize `getSnapshot()` execution.
const newValueRef = useRef<CachedValue<TKey, TReturnValue> | undefined | null>(null);
const newValueRef = useRef<TReturnValue | undefined | null>(null);

// Stores the previously result returned by the hook, containing the data from cache and the fetch status.
// We initialize it to `undefined` and `loading` fetch status to simulate the initial result when the hook is loading from the cache.
// However, if `initWithStoredValues` is `false` we set the fetch status to `loaded` since we want to signal that data is ready.
const resultRef = useRef<UseOnyxResult<TKey, TReturnValue>>([
undefined as CachedValue<TKey, TReturnValue>,
const resultRef = useRef<UseOnyxResult<TReturnValue>>([
undefined,
{
status: options?.initWithStoredValues === false ? 'loaded' : 'loading',
},
Expand Down Expand Up @@ -159,7 +193,7 @@ function useOnyx<TKey extends OnyxKey, TReturnValue = OnyxValue<TKey>>(key: TKey
// If `newValueRef.current` is `null` or any other value it means that the cache does have a value for that key.
// This difference between `undefined` and other values is crucial and it's used to address the following
// conditions and use cases.
newValueRef.current = getCachedValue<TKey, TReturnValue>(key, selectorRef.current);
newValueRef.current = getCachedValue(key, selectorRef.current);

// We set this flag to `false` again since we don't want to get the newest cached value every time `getSnapshot()` is executed,
// and only when `Onyx.connect()` callback is fired.
Expand All @@ -182,7 +216,7 @@ function useOnyx<TKey extends OnyxKey, TReturnValue = OnyxValue<TKey>>(key: TKey
// If data is not present in cache and `initialValue` is set during the first connection,
// we set the new value to `initialValue` and fetch status to `loaded` since we already have some data to return to the consumer.
if (isFirstConnectionRef.current && !hasCacheForKey && options?.initialValue !== undefined) {
newValueRef.current = (options?.initialValue ?? undefined) as CachedValue<TKey, TReturnValue>;
newValueRef.current = (options?.initialValue ?? undefined) as TReturnValue;
newFetchStatus = 'loaded';
}

Expand All @@ -204,11 +238,11 @@ function useOnyx<TKey extends OnyxKey, TReturnValue = OnyxValue<TKey>>(key: TKey
previousValueRef.current = newValueRef.current;

// If the new value is `null` we default it to `undefined` to ensure the consumer gets a consistent result from the hook.
resultRef.current = [previousValueRef.current as CachedValue<TKey, TReturnValue>, {status: newFetchStatus ?? 'loaded'}];
resultRef.current = [previousValueRef.current ?? undefined, {status: newFetchStatus ?? 'loaded'}];
kacper-mikolajczak marked this conversation as resolved.
Show resolved Hide resolved
}

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

const subscribe = useCallback(
(onStoreChange: () => void) => {
Expand Down Expand Up @@ -247,7 +281,7 @@ function useOnyx<TKey extends OnyxKey, TReturnValue = OnyxValue<TKey>>(key: TKey
checkEvictableKey();
}, [checkEvictableKey]);

const result = useSyncExternalStore<UseOnyxResult<TKey, TReturnValue>>(subscribe, getSnapshot);
const result = useSyncExternalStore<UseOnyxResult<TReturnValue>>(subscribe, getSnapshot);

return result;
}
Expand Down
8 changes: 6 additions & 2 deletions tests/unit/useOnyxTest.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import {act, renderHook} from '@testing-library/react-native';
import type {OnyxEntry} from '../../lib';
import type {OnyxCollection, OnyxEntry} from '../../lib';
import Onyx, {useOnyx} from '../../lib';
import OnyxUtils from '../../lib/OnyxUtils';
import StorageMock from '../../lib/storage';
Expand Down Expand Up @@ -176,7 +176,11 @@ describe('useOnyx', () => {
const {result} = renderHook(() =>
useOnyx(ONYXKEYS.COLLECTION.TEST_KEY, {
// @ts-expect-error bypass
selector: (entry: OnyxEntry<{id: string; name: string}>) => entry?.id,
selector: (entries: OnyxCollection<{id: string; name: string}>) =>
Object.entries(entries ?? {}).reduce<NonNullable<OnyxCollection<string>>>((acc, [key, value]) => {
acc[key] = value?.id;
return acc;
}, {}),
}),
);

Expand Down
Loading