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.

Sign up for GitHub

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

More minor fixes for the Onyx bump in E/App #561

Merged
merged 12 commits into from
Jun 11, 2024
Merged

More minor fixes for the Onyx bump in E/App #561

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
9324ca5
fix: initial loading state of useOnyx
chrispader Jun 10, 2024
9cd73cf
fix: initial values and loading states
chrispader Jun 11, 2024
43335ee
improve Onyx.clear, clearing values from cache
chrispader Jun 11, 2024
7165f1c
update tests to reflect new Onyx cache behaviour
chrispader Jun 11, 2024
1a983e5
undo unnecessary changes to one test
chrispader Jun 11, 2024
0ca3476
fix: improve code
chrispader Jun 11, 2024
5da5e50
rename test
chrispader Jun 11, 2024
a0991a9
fix: update useOnyx result when cache was set for the first time
chrispader Jun 11, 2024
9c94b0c
remove unused check
chrispader Jun 11, 2024
ec1c9ba
add back getCachedValue function
chrispader Jun 11, 2024
9134e7e
remove unnecessary check
chrispader Jun 11, 2024
cdbd255
fix: typo
chrispader Jun 11, 2024
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
6 changes: 3 additions & 3 deletions lib/Onyx.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -575,17 +575,17 @@ function clear(keysToPreserve: OnyxKey[] = []): Promise<void> {
// since collection key subscribers need to be updated differently
if (!isKeyToPreserve) {
const oldValue = cache.get(key);
const newValue = defaultKeyStates[key] ?? undefined;
const newValue = defaultKeyStates[key] ?? null;
if (newValue !== oldValue) {
cache.set(key, newValue);
const collectionKey = key.substring(0, key.indexOf('_') + 1);
if (collectionKey) {
if (!keyValuesToResetAsCollection[collectionKey]) {
keyValuesToResetAsCollection[collectionKey] = {};
}
keyValuesToResetAsCollection[collectionKey]![key] = newValue;
keyValuesToResetAsCollection[collectionKey]![key] = newValue ?? undefined;
} else {
keyValuesToResetIndividually[key] = newValue;
keyValuesToResetIndividually[key] = newValue ?? undefined;
}
}
}
Expand Down
33 changes: 18 additions & 15 deletions lib/useOnyx.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import type {CollectionKeyBase, OnyxCollection, OnyxKey, OnyxValue, Selector} fr
import useLiveRef from './useLiveRef';
import usePrevious from './usePrevious';
import Onyx from './Onyx';
import cache from './OnyxCache';

type BaseUseOnyxOptions = {
/**
Expand Down Expand Up @@ -53,10 +54,6 @@ type ResultMetadata = {

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

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

function useOnyx<TKey extends OnyxKey, TReturnValue = OnyxValue<TKey>>(
key: TKey,
options?: BaseUseOnyxOptions & UseOnyxInitialValueOption<TReturnValue> & Required<UseOnyxSelectorOption<TKey, TReturnValue>>,
Expand All @@ -73,8 +70,8 @@ function useOnyx<TKey extends OnyxKey, TReturnValue = OnyxValue<TKey>>(key: TKey
const selectorRef = useLiveRef(options?.selector);

// Stores the previous cached value as it's necessary to compare with the new value in `getSnapshot()`.
// We initialize it to `undefined` to simulate that we don't have any value from cache yet.
const cachedValueRef = useRef<CachedValue<TKey, TReturnValue> | undefined>(undefined);
// We initialize it to `null` to simulate that we don't have any value from cache yet.
const cachedValueRef = useRef<CachedValue<TKey, 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.
Expand Down Expand Up @@ -120,7 +117,8 @@ function useOnyx<TKey extends OnyxKey, TReturnValue = OnyxValue<TKey>>(key: TKey
// If `newValue` is `null` or any other value if means that the cache does have a value for that key.
chrispader marked this conversation as resolved.
Show resolved Hide resolved
// This difference between `undefined` and other values is crucial and it's used to address the following
// conditions and use cases.
let newValue = getCachedValue<TKey, TReturnValue>(key, selectorRef.current);
let newValue = (OnyxUtils.tryGetCachedValue(key, {selector: selectorRef.current}) ?? undefined) as CachedValue<TKey, TReturnValue> | undefined;
chrispader marked this conversation as resolved.
Show resolved Hide resolved
const hasCacheForKey = cache.hasCacheForKey(key);

// Since the fetch status can be different given the use cases below, we define the variable right away.
let newFetchStatus: FetchStatus | undefined;
Expand All @@ -133,24 +131,29 @@ function useOnyx<TKey extends OnyxKey, TReturnValue = OnyxValue<TKey>>(key: TKey
newFetchStatus = 'loading';
}

// If data is not present in cache (if it's `undefined`) and `initialValue` is set during the first connection,
// If `options.initWithStoredValues` is explicitly set to `false`, the fetch status should always be `loaded`.
if (options?.initWithStoredValues === false) {
chrispader marked this conversation as resolved.
Show resolved Hide resolved
newFetchStatus = 'loaded';
}

// 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 && newValue === undefined && options?.initialValue !== undefined) {
newValue = options?.initialValue as CachedValue<TKey, TReturnValue>;
if (isFirstConnectionRef.current && !hasCacheForKey && options?.initialValue !== null && options?.initialValue !== undefined) {
chrispader marked this conversation as resolved.
Show resolved Hide resolved
newValue = (options?.initialValue ?? undefined) as CachedValue<TKey, TReturnValue>;
newFetchStatus = 'loaded';
}

// If the previously cached value is different from the new value, we update both cached value
// and the result to be returned by the hook.
if (!deepEqual(cachedValueRef.current, newValue)) {
// If the cache was set for the first time, we also update the cached value and the result.
const isCacheSetFirstTime = cachedValueRef.current === null && hasCacheForKey;
if (isCacheSetFirstTime || !deepEqual(cachedValueRef.current ?? undefined, newValue)) {
cachedValueRef.current = newValue;

// If the new value is `null` we default it to `undefined` to ensure the consumer get a consistent result from the hook.
resultRef.current = [(cachedValueRef.current ?? undefined) as CachedValue<TKey, TReturnValue>, {status: newFetchStatus ?? 'loaded'}];
resultRef.current = [cachedValueRef.current as CachedValue<TKey, TReturnValue>, {status: newFetchStatus ?? 'loaded'}];
}

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

const subscribe = useCallback(
(onStoreChange: () => void) => {
Expand Down
20 changes: 10 additions & 10 deletions tests/unit/useOnyxTest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ describe('useOnyx', () => {
expect(result.current[1].status).toEqual('loaded');
});

it('should initially return null while loading non-cached key, and then return value and loaded state', async () => {
it('should initially return `undefined` while loading non-cached key, and then return value and loaded state', async () => {
await StorageMock.setItem(ONYXKEYS.TEST_KEY, 'test');

const {result} = renderHook(() => useOnyx(ONYXKEYS.TEST_KEY));
Expand Down Expand Up @@ -244,8 +244,6 @@ describe('useOnyx', () => {
});

it('should return initial value if selected data is undefined', async () => {
Onyx.set(ONYXKEYS.TEST_KEY, 'test_id_1');

const {result} = renderHook(() =>
useOnyx(ONYXKEYS.TEST_KEY, {
// @ts-expect-error bypass
Expand All @@ -257,6 +255,8 @@ describe('useOnyx', () => {
expect(result.current[0]).toEqual('initial value');
expect(result.current[1].status).toEqual('loaded');

Onyx.set(ONYXKEYS.TEST_KEY, 'test_id_1');
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious: Why did you move the operation to here? Shouldn't we have tests for both cases?

Copy link
Contributor Author

@chrispader chrispader Jun 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The moment we use Onyx.set or Onyx.merge (or any setter function) it will store the value in cache synchronously and update the storageKeys Set in OnyxCache. The storage will be updates asynchronously,

If we set the key before asserting for the initialValue, we will always get undefined, because there IS a value in cache already, even though the storage promise hasn't resolved yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the explanation!


await act(async () => waitForPromisesToResolve());

expect(result.current[0]).toBeUndefined();
Expand Down Expand Up @@ -320,12 +320,6 @@ describe('useOnyx', () => {
});

it('should return initial value and loaded state while we have pending merges for the key, and then return updated value and loaded state', async () => {
Onyx.set(ONYXKEYS.TEST_KEY, 'test1');

Onyx.merge(ONYXKEYS.TEST_KEY, 'test2');
Onyx.merge(ONYXKEYS.TEST_KEY, 'test3');
Onyx.merge(ONYXKEYS.TEST_KEY, 'test4');

const {result} = renderHook(() =>
useOnyx(ONYXKEYS.TEST_KEY, {
initialValue: 'initial value',
Expand All @@ -335,6 +329,12 @@ describe('useOnyx', () => {
expect(result.current[0]).toEqual('initial value');
expect(result.current[1].status).toEqual('loaded');

Onyx.set(ONYXKEYS.TEST_KEY, 'test1');
Copy link
Contributor

Choose a reason for hiding this comment

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

Same: Why did you move the operation to here? Shouldn't we have tests for both cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above


Onyx.merge(ONYXKEYS.TEST_KEY, 'test2');
Onyx.merge(ONYXKEYS.TEST_KEY, 'test3');
Onyx.merge(ONYXKEYS.TEST_KEY, 'test4');

await act(async () => waitForPromisesToResolve());

expect(result.current[0]).toEqual('test4');
Expand All @@ -361,7 +361,7 @@ describe('useOnyx', () => {
});

describe('initWithStoredValues', () => {
it('should return null and loaded state, and after merge return updated value and loaded state', async () => {
it('should return `undefined` and loaded state, and after merge return updated value and loaded state', async () => {
await StorageMock.setItem(ONYXKEYS.TEST_KEY, 'test1');

const {result} = renderHook(() => useOnyx(ONYXKEYS.TEST_KEY, {initWithStoredValues: false}));
Expand Down
Loading