From 106161a13b3d1cf3870fa1bed0c5834200d7fa1d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A1bio=20Henriques?= Date: Thu, 4 Jul 2024 18:06:40 +0100 Subject: [PATCH 1/4] Fix useOnyx equality check --- lib/useOnyx.ts | 7 +++---- tests/unit/useOnyxTest.ts | 27 ++++++++++++++++++++++++--- 2 files changed, 27 insertions(+), 7 deletions(-) diff --git a/lib/useOnyx.ts b/lib/useOnyx.ts index e523eaff..c80bb853 100644 --- a/lib/useOnyx.ts +++ b/lib/useOnyx.ts @@ -159,12 +159,11 @@ function useOnyx>(key: TKey newFetchStatus = 'loaded'; } - // We do a deep equality check if we are subscribed to a collection key and `selector` is defined, - // since each `OnyxUtils.tryGetCachedValue()` call will generate a plain new collection object with new records as well, - // all of them created using the `selector` function. + // We do a deep equality check if `selector` is defined, since each `OnyxUtils.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. let areValuesEqual: boolean; - if (OnyxUtils.isCollectionKey(key) && selectorRef.current) { + if (selectorRef.current) { areValuesEqual = deepEqual(previousValueRef.current ?? undefined, newValueRef.current); } else { areValuesEqual = shallowEqual(previousValueRef.current ?? undefined, newValueRef.current); diff --git a/tests/unit/useOnyxTest.ts b/tests/unit/useOnyxTest.ts index 42f1c9f1..c8f5b897 100644 --- a/tests/unit/useOnyxTest.ts +++ b/tests/unit/useOnyxTest.ts @@ -198,21 +198,42 @@ describe('useOnyx', () => { it('should not change selected data if a property outside that data was changed', async () => { Onyx.set(ONYXKEYS.TEST_KEY, {id: 'test_id', name: 'test_name'}); - const {result} = renderHook(() => + // primitive + const {result: primitiveResult} = renderHook(() => + useOnyx(ONYXKEYS.TEST_KEY, { + // @ts-expect-error bypass + selector: (entry: OnyxEntry<{id: string; name: string}>) => entry?.id, + }), + ); + + // object + const {result: objectResult} = renderHook(() => useOnyx(ONYXKEYS.TEST_KEY, { // @ts-expect-error bypass selector: (entry: OnyxEntry<{id: string; name: string}>) => ({id: entry?.id}), }), ); + // array + const {result: arrayResult} = renderHook(() => + useOnyx(ONYXKEYS.TEST_KEY, { + // @ts-expect-error bypass + selector: (entry: OnyxEntry<{id: string; name: string}>) => [{id: entry?.id}], + }), + ); + await act(async () => waitForPromisesToResolve()); - const oldResult = result.current; + const oldPrimitiveResult = primitiveResult.current; + const oldObjectResult = objectResult.current; + const oldArrayResult = arrayResult.current; await act(async () => Onyx.merge(ONYXKEYS.TEST_KEY, {name: 'test_name_changed'})); // must be the same reference - expect(oldResult).toBe(result.current); + expect(oldPrimitiveResult).toBe(primitiveResult.current); + expect(oldObjectResult).toBe(objectResult.current); + expect(oldArrayResult).toBe(arrayResult.current); }); it('should not change selected collection data if a property outside that data was changed', async () => { From f439153823c962aa302471eebffbcd308f87adf6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A1bio=20Henriques?= Date: Thu, 4 Jul 2024 18:46:48 +0100 Subject: [PATCH 2/4] Adjust logic --- lib/useOnyx.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/useOnyx.ts b/lib/useOnyx.ts index c80bb853..81e54711 100644 --- a/lib/useOnyx.ts +++ b/lib/useOnyx.ts @@ -159,11 +159,12 @@ function useOnyx>(key: TKey newFetchStatus = 'loaded'; } - // We do a deep equality check if `selector` is defined, since each `OnyxUtils.tryGetCachedValue()` call will - // generate a plain new primitive/object/array that was created using the `selector` function. + // We do a deep equality check if we are subscribed to a collection key OR `selector` is defined, + // since each `OnyxUtils.tryGetCachedValue()` call will generate a plain new collection object when using a collection key, + // or a 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. let areValuesEqual: boolean; - if (selectorRef.current) { + if (OnyxUtils.isCollectionKey(key) || selectorRef.current) { areValuesEqual = deepEqual(previousValueRef.current ?? undefined, newValueRef.current); } else { areValuesEqual = shallowEqual(previousValueRef.current ?? undefined, newValueRef.current); From 09f246503c1b6ce8059e2b58da3eab56f9ca22c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A1bio=20Henriques?= Date: Fri, 5 Jul 2024 12:02:25 +0100 Subject: [PATCH 3/4] Revert "Adjust logic" This reverts commit f439153823c962aa302471eebffbcd308f87adf6. --- lib/useOnyx.ts | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/lib/useOnyx.ts b/lib/useOnyx.ts index 81e54711..c80bb853 100644 --- a/lib/useOnyx.ts +++ b/lib/useOnyx.ts @@ -159,12 +159,11 @@ function useOnyx>(key: TKey newFetchStatus = 'loaded'; } - // We do a deep equality check if we are subscribed to a collection key OR `selector` is defined, - // since each `OnyxUtils.tryGetCachedValue()` call will generate a plain new collection object when using a collection key, - // or a new primitive/object/array that was created using the `selector` function. + // We do a deep equality check if `selector` is defined, since each `OnyxUtils.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. let areValuesEqual: boolean; - if (OnyxUtils.isCollectionKey(key) || selectorRef.current) { + if (selectorRef.current) { areValuesEqual = deepEqual(previousValueRef.current ?? undefined, newValueRef.current); } else { areValuesEqual = shallowEqual(previousValueRef.current ?? undefined, newValueRef.current); From b99cc27db62b2aeafbee93478d682ec6259de7d8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A1bio=20Henriques?= Date: Fri, 5 Jul 2024 14:13:46 +0100 Subject: [PATCH 4/4] Remove some @ts-expect-error --- tests/unit/useOnyxTest.ts | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/tests/unit/useOnyxTest.ts b/tests/unit/useOnyxTest.ts index c8f5b897..4a7af480 100644 --- a/tests/unit/useOnyxTest.ts +++ b/tests/unit/useOnyxTest.ts @@ -3,6 +3,7 @@ import type {OnyxEntry} from '../../lib'; import Onyx, {useOnyx} from '../../lib'; import waitForPromisesToResolve from '../utils/waitForPromisesToResolve'; import StorageMock from '../../lib/storage'; +import type GenericCollection from '../utils/GenericCollection'; const ONYXKEYS = { TEST_KEY: 'test', @@ -163,12 +164,11 @@ describe('useOnyx', () => { }); it('should return selected data from a collection key', async () => { - // @ts-expect-error bypass 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); const {result} = renderHook(() => useOnyx(ONYXKEYS.COLLECTION.TEST_KEY, { @@ -237,12 +237,11 @@ describe('useOnyx', () => { }); it('should not change selected collection data if a property outside that data was changed', async () => { - // @ts-expect-error bypass 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); const {result} = renderHook(() => useOnyx(ONYXKEYS.COLLECTION.TEST_KEY, {