From d60d3efa4f88e62db195eacd4f5143bab6c6494f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Mon, 22 Jul 2024 08:07:37 +0200 Subject: [PATCH 1/9] wip: connection called twice, added failing unit test --- lib/Onyx.ts | 2 +- tests/unit/onyxTest.ts | 15 +++++++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/lib/Onyx.ts b/lib/Onyx.ts index a479712a..da3d12b1 100644 --- a/lib/Onyx.ts +++ b/lib/Onyx.ts @@ -87,7 +87,7 @@ function init({ * @param [mapping.callback] a method that will be called with changed data * This is used by any non-React code to connect to Onyx * @param [mapping.initWithStoredValues] If set to false, then no data will be prefilled into the - * component + * component. Default is true. * @param [mapping.waitForCollectionCallback] If set to true, it will return the entire collection to the callback as a single object * @param [mapping.selector] THIS PARAM IS ONLY USED WITH withOnyx(). If included, this will be used to subscribe to a subset of an Onyx key's data. * The sourceData and withOnyx state are passed to the selector and should return the simplified data. Using this setting on `withOnyx` can have very positive diff --git a/tests/unit/onyxTest.ts b/tests/unit/onyxTest.ts index 978c8c57..907bd313 100644 --- a/tests/unit/onyxTest.ts +++ b/tests/unit/onyxTest.ts @@ -100,6 +100,21 @@ describe('Onyx', () => { }); }); + it("shouldn't call a connection twice when setting a value", () => { + const mockCallback = jest.fn(); + + connectionID = Onyx.connect({ + key: ONYX_KEYS.TEST_KEY, + callback: mockCallback, + // True is the default, just setting it here to be explicit + initWithStoredValues: true, + }); + + return Onyx.set(ONYX_KEYS.TEST_KEY, 'test').then(() => { + expect(mockCallback).toHaveBeenCalledTimes(1); + }); + }); + it('should merge an object with another object', () => { let testKeyValue: unknown; From b7a7f2dbe1e6ceb8ce378cac1ffc2909ff04b343 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Mon, 22 Jul 2024 11:06:25 +0200 Subject: [PATCH 2/9] store last connection callback values & compare --- lib/OnyxUtils.ts | 25 +++++++++++++++++++++---- tests/unit/onyxTest.ts | 15 +++++++++++---- 2 files changed, 32 insertions(+), 8 deletions(-) diff --git a/lib/OnyxUtils.ts b/lib/OnyxUtils.ts index fb609fbd..1c407974 100644 --- a/lib/OnyxUtils.ts +++ b/lib/OnyxUtils.ts @@ -72,6 +72,9 @@ let defaultKeyStates: Record> = {}; let batchUpdatesPromise: Promise | null = null; let batchUpdatesQueue: Array<() => void> = []; +// Holds a map representing the latest data that was used to invoke a connection callback +const lastConnectionCallbackData = new Map>(); + let snapshotKey: OnyxKey | null = null; function getSnapshotKey(): OnyxKey | null { @@ -774,8 +777,8 @@ function keyChanged( value: OnyxValue, previousValue: OnyxValue, canUpdateSubscriber: (subscriber?: Mapping) => boolean = () => true, - notifyRegularSubscibers = true, - notifyWithOnyxSubscibers = true, + notifyConnectSubscribers = true, + notifyWithOnyxSubscribers = true, ): void { // Add or remove this key from the recentlyAccessedKeys lists if (value !== null) { @@ -809,9 +812,13 @@ function keyChanged( // Subscriber is a regular call to connect() and provided a callback if (typeof subscriber.callback === 'function') { - if (!notifyRegularSubscibers) { + if (!notifyConnectSubscribers) { continue; } + if (lastConnectionCallbackData.has(subscriber.connectionID) && lastConnectionCallbackData.get(subscriber.connectionID) === value) { + continue; + } + if (isCollectionKey(subscriber.key) && subscriber.waitForCollectionCallback) { const cachedCollection = getCachedCollection(subscriber.key); @@ -822,12 +829,14 @@ function keyChanged( const subscriberCallback = subscriber.callback as DefaultConnectCallback; subscriberCallback(value, key); + + lastConnectionCallbackData.set(subscriber.connectionID, value); continue; } // Subscriber connected via withOnyx() HOC if ('withOnyxInstance' in subscriber && subscriber.withOnyxInstance) { - if (!notifyWithOnyxSubscibers) { + if (!notifyWithOnyxSubscribers) { continue; } @@ -960,6 +969,14 @@ function sendDataToConnection(mapping: Mapping, valu // If we would pass undefined to setWithOnyxInstance instead, withOnyx would not set the value in the state. // withOnyx will internally replace null values with undefined and never pass null values to wrapped components. // For regular callbacks, we never want to pass null values, but always just undefined if a value is not set in cache or storage. + const valueToPass = value === null ? undefined : value; + const lastValue = lastConnectionCallbackData.get(mapping.connectionID); + + // If the value has not changed we do not need to trigger the callback + if (lastConnectionCallbackData.has(mapping.connectionID) && valueToPass === lastValue) { + return; + } + (mapping as DefaultConnectOptions).callback?.(value === null ? undefined : value, matchedKey as TKey); } diff --git a/tests/unit/onyxTest.ts b/tests/unit/onyxTest.ts index 907bd313..94956f40 100644 --- a/tests/unit/onyxTest.ts +++ b/tests/unit/onyxTest.ts @@ -168,23 +168,28 @@ describe('Onyx', () => { }); let otherTestValue: unknown; + const mockCallback = jest.fn((value) => { + otherTestValue = value; + }); const otherTestConnectionID = Onyx.connect({ key: ONYX_KEYS.OTHER_TEST, - callback: (value) => { - otherTestValue = value; - }, + callback: mockCallback, }); return waitForPromisesToResolve() .then(() => Onyx.set(ONYX_KEYS.TEST_KEY, 'test')) .then(() => { expect(testKeyValue).toBe('test'); + mockCallback.mockReset(); return Onyx.clear().then(waitForPromisesToResolve); }) .then(() => { // Test key should be cleared expect(testKeyValue).toBeUndefined(); + // Expect that the connection to a key with a default value wasn't cleared + expect(mockCallback).toHaveBeenCalledTimes(0); + // Other test key should be returned to its default state expect(otherTestValue).toBe(42); @@ -1380,7 +1385,9 @@ describe('Onyx', () => { expect(collectionCallback).toHaveBeenCalledTimes(3); expect(collectionCallback).toHaveBeenCalledWith(collectionDiff); - expect(catCallback).toHaveBeenCalledTimes(2); + // Cat hasn't changed from its original value, expect only the initial connect callback + expect(catCallback).toHaveBeenCalledTimes(1); + // Dog was modified, expect the initial connect callback and the mergeCollection callback expect(dogCallback).toHaveBeenCalledTimes(2); connectionIDs.map((id) => Onyx.disconnect(id)); From bd0a686128a8f6543ac0c15fccf9732733c70d61 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Mon, 22 Jul 2024 12:35:43 +0200 Subject: [PATCH 3/9] added alwaysNotify prop for useOnyx use case --- lib/OnyxUtils.ts | 4 +++- lib/types.ts | 5 +++++ lib/useOnyx.ts | 3 +++ 3 files changed, 11 insertions(+), 1 deletion(-) diff --git a/lib/OnyxUtils.ts b/lib/OnyxUtils.ts index 1c407974..8aaf57d0 100644 --- a/lib/OnyxUtils.ts +++ b/lib/OnyxUtils.ts @@ -971,9 +971,11 @@ function sendDataToConnection(mapping: Mapping, valu // For regular callbacks, we never want to pass null values, but always just undefined if a value is not set in cache or storage. const valueToPass = value === null ? undefined : value; const lastValue = lastConnectionCallbackData.get(mapping.connectionID); + lastConnectionCallbackData.get(mapping.connectionID); // If the value has not changed we do not need to trigger the callback - if (lastConnectionCallbackData.has(mapping.connectionID) && valueToPass === lastValue) { + const alwaysNotify = 'alwaysNotify' in mapping && mapping.alwaysNotify; + if (!alwaysNotify && lastConnectionCallbackData.has(mapping.connectionID) && valueToPass === lastValue) { return; } diff --git a/lib/types.ts b/lib/types.ts index e6a6c0ec..f3dca011 100644 --- a/lib/types.ts +++ b/lib/types.ts @@ -263,6 +263,11 @@ type Collection = { /** Represents the base options used in `Onyx.connect()` method. */ type BaseConnectOptions = { initWithStoredValues?: boolean; + /** + * If true, it will call the callback function even if the value might hasn't changed. + * @default false + */ + alwaysNotify?: boolean; }; /** Represents additional options used inside withOnyx HOC */ diff --git a/lib/useOnyx.ts b/lib/useOnyx.ts index c80bb853..0754ccd4 100644 --- a/lib/useOnyx.ts +++ b/lib/useOnyx.ts @@ -200,6 +200,9 @@ function useOnyx>(key: TKey }, initWithStoredValues: options?.initWithStoredValues, waitForCollectionCallback: OnyxUtils.isCollectionKey(key) as true, + // As we potentially calculate the result data with a selector that can return different data + // for the same input data, we always want to receive the connect callback. + alwaysNotify: true, }); return () => { From d7a6ca0a8a36297a67deb0f48531f68214446641 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Tue, 23 Jul 2024 16:11:35 +0200 Subject: [PATCH 4/9] Update tests/unit/onyxTest.ts Co-authored-by: Vit Horacek <36083550+mountiny@users.noreply.github.com> --- tests/unit/onyxTest.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/unit/onyxTest.ts b/tests/unit/onyxTest.ts index 94956f40..d4879ee9 100644 --- a/tests/unit/onyxTest.ts +++ b/tests/unit/onyxTest.ts @@ -1387,6 +1387,7 @@ describe('Onyx', () => { // Cat hasn't changed from its original value, expect only the initial connect callback expect(catCallback).toHaveBeenCalledTimes(1); + // Dog was modified, expect the initial connect callback and the mergeCollection callback expect(dogCallback).toHaveBeenCalledTimes(2); From ad77f7c0706944c8e3ed14a0641871b99be5279f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Tue, 23 Jul 2024 17:44:59 +0200 Subject: [PATCH 5/9] remove alwaysNotify flag, fix test --- lib/OnyxUtils.ts | 3 +-- lib/types.ts | 5 ----- lib/useOnyx.ts | 3 --- tests/unit/onyxTest.ts | 35 +++++++++++++++++++++++++++++++++++ tests/unit/useOnyxTest.ts | 4 +++- 5 files changed, 39 insertions(+), 11 deletions(-) diff --git a/lib/OnyxUtils.ts b/lib/OnyxUtils.ts index 8aaf57d0..3f093d55 100644 --- a/lib/OnyxUtils.ts +++ b/lib/OnyxUtils.ts @@ -974,8 +974,7 @@ function sendDataToConnection(mapping: Mapping, valu lastConnectionCallbackData.get(mapping.connectionID); // If the value has not changed we do not need to trigger the callback - const alwaysNotify = 'alwaysNotify' in mapping && mapping.alwaysNotify; - if (!alwaysNotify && lastConnectionCallbackData.has(mapping.connectionID) && valueToPass === lastValue) { + if (lastConnectionCallbackData.has(mapping.connectionID) && valueToPass === lastValue) { return; } diff --git a/lib/types.ts b/lib/types.ts index f3dca011..e6a6c0ec 100644 --- a/lib/types.ts +++ b/lib/types.ts @@ -263,11 +263,6 @@ type Collection = { /** Represents the base options used in `Onyx.connect()` method. */ type BaseConnectOptions = { initWithStoredValues?: boolean; - /** - * If true, it will call the callback function even if the value might hasn't changed. - * @default false - */ - alwaysNotify?: boolean; }; /** Represents additional options used inside withOnyx HOC */ diff --git a/lib/useOnyx.ts b/lib/useOnyx.ts index 0754ccd4..c80bb853 100644 --- a/lib/useOnyx.ts +++ b/lib/useOnyx.ts @@ -200,9 +200,6 @@ function useOnyx>(key: TKey }, initWithStoredValues: options?.initWithStoredValues, waitForCollectionCallback: OnyxUtils.isCollectionKey(key) as true, - // As we potentially calculate the result data with a selector that can return different data - // for the same input data, we always want to receive the connect callback. - alwaysNotify: true, }); return () => { diff --git a/tests/unit/onyxTest.ts b/tests/unit/onyxTest.ts index d4879ee9..03f5d3cc 100644 --- a/tests/unit/onyxTest.ts +++ b/tests/unit/onyxTest.ts @@ -197,6 +197,41 @@ describe('Onyx', () => { }); }); + it('should reset Onyx state to empty after calling clear()', () => { + const mockCallback = jest.fn(); + connectionID = Onyx.connect({ + key: ONYX_KEYS.COLLECTION.TEST_KEY, + waitForCollectionCallback: true, + callback: mockCallback, + }); + + return Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, { + [`${ONYX_KEYS.COLLECTION.TEST_KEY}1`]: 'test1', + } as GenericCollection) + .then(() => { + expect(mockCallback).toHaveBeenCalledTimes(2); // TODO: should only be 1, fixed here https://github.com/Expensify/react-native-onyx/pull/570 + mockCallback.mockReset(); + return Onyx.clear(); + }) + .then(() => { + // After clearing we expect the callback to be called with an empty object + expect(mockCallback).toHaveBeenCalledTimes(1); + expect(mockCallback).toHaveBeenCalledWith({}); + + // Make sure that the onyx cache and storage are empty + + mockCallback.mockReset(); + + return Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, { + [`${ONYX_KEYS.COLLECTION.TEST_KEY}2`]: 'test2', + } as GenericCollection); + }) + .then(() => { + expect(mockCallback).toHaveBeenCalledTimes(1); + expect(mockCallback).toHaveBeenCalledWith({[`${ONYX_KEYS.COLLECTION.TEST_KEY}2`]: 'test2'}); + }); + }); + it('should not notify subscribers after they have disconnected', () => { let testKeyValue: unknown; connectionID = Onyx.connect({ diff --git a/tests/unit/useOnyxTest.ts b/tests/unit/useOnyxTest.ts index 4a7af480..2a58efa4 100644 --- a/tests/unit/useOnyxTest.ts +++ b/tests/unit/useOnyxTest.ts @@ -265,7 +265,7 @@ describe('useOnyx', () => { let selector = (entry: OnyxEntry<{id: string; name: string}>) => `id - ${entry?.id}, name - ${entry?.name}`; - const {result} = renderHook(() => + const {result, rerender} = renderHook(() => useOnyx(ONYXKEYS.TEST_KEY, { // @ts-expect-error bypass selector, @@ -276,6 +276,8 @@ describe('useOnyx', () => { expect(result.current[1].status).toEqual('loaded'); selector = (entry: OnyxEntry<{id: string; name: string}>) => `id - ${entry?.id}, name - ${entry?.name} - selector changed`; + // In a react app we expect the selector ref to change during a rerender (see selectorRef/useLiveRef) + rerender(undefined); await act(async () => Onyx.merge(ONYXKEYS.TEST_KEY, {id: 'changed_id', name: 'changed_name'})); From bc8face5ea40cc2dcbed0ab5d90df8f65d115517 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Tue, 23 Jul 2024 17:47:41 +0200 Subject: [PATCH 6/9] removed onyx clear test (meant for a different PR) --- tests/unit/onyxTest.ts | 35 ----------------------------------- 1 file changed, 35 deletions(-) diff --git a/tests/unit/onyxTest.ts b/tests/unit/onyxTest.ts index 03f5d3cc..d4879ee9 100644 --- a/tests/unit/onyxTest.ts +++ b/tests/unit/onyxTest.ts @@ -197,41 +197,6 @@ describe('Onyx', () => { }); }); - it('should reset Onyx state to empty after calling clear()', () => { - const mockCallback = jest.fn(); - connectionID = Onyx.connect({ - key: ONYX_KEYS.COLLECTION.TEST_KEY, - waitForCollectionCallback: true, - callback: mockCallback, - }); - - return Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, { - [`${ONYX_KEYS.COLLECTION.TEST_KEY}1`]: 'test1', - } as GenericCollection) - .then(() => { - expect(mockCallback).toHaveBeenCalledTimes(2); // TODO: should only be 1, fixed here https://github.com/Expensify/react-native-onyx/pull/570 - mockCallback.mockReset(); - return Onyx.clear(); - }) - .then(() => { - // After clearing we expect the callback to be called with an empty object - expect(mockCallback).toHaveBeenCalledTimes(1); - expect(mockCallback).toHaveBeenCalledWith({}); - - // Make sure that the onyx cache and storage are empty - - mockCallback.mockReset(); - - return Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, { - [`${ONYX_KEYS.COLLECTION.TEST_KEY}2`]: 'test2', - } as GenericCollection); - }) - .then(() => { - expect(mockCallback).toHaveBeenCalledTimes(1); - expect(mockCallback).toHaveBeenCalledWith({[`${ONYX_KEYS.COLLECTION.TEST_KEY}2`]: 'test2'}); - }); - }); - it('should not notify subscribers after they have disconnected', () => { let testKeyValue: unknown; connectionID = Onyx.connect({ From 5e571f580f770c65d776348bd383a8a7816f30aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Wed, 24 Jul 2024 09:24:34 +0200 Subject: [PATCH 7/9] reuse valueToPass --- lib/OnyxUtils.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/OnyxUtils.ts b/lib/OnyxUtils.ts index 3f093d55..9a017f3c 100644 --- a/lib/OnyxUtils.ts +++ b/lib/OnyxUtils.ts @@ -978,7 +978,7 @@ function sendDataToConnection(mapping: Mapping, valu return; } - (mapping as DefaultConnectOptions).callback?.(value === null ? undefined : value, matchedKey as TKey); + (mapping as DefaultConnectOptions).callback?.(valueToPass, matchedKey as TKey); } /** From 3161a0fa4e16b461a00b1ebb08f789e611b2828a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Wed, 24 Jul 2024 09:25:46 +0200 Subject: [PATCH 8/9] better explained --- lib/OnyxUtils.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/OnyxUtils.ts b/lib/OnyxUtils.ts index 9a017f3c..cac8b0b1 100644 --- a/lib/OnyxUtils.ts +++ b/lib/OnyxUtils.ts @@ -72,7 +72,7 @@ let defaultKeyStates: Record> = {}; let batchUpdatesPromise: Promise | null = null; let batchUpdatesQueue: Array<() => void> = []; -// Holds a map representing the latest data that was used to invoke a connection callback +// Used for comparison with a new update to avoid invoking the Onyx.connect callback with the same data. const lastConnectionCallbackData = new Map>(); let snapshotKey: OnyxKey | null = null; From 69d6152d6225b11697c86c168556767a099bf6fa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Wed, 24 Jul 2024 12:22:18 +0200 Subject: [PATCH 9/9] skip merge, just confirm selector changes --- tests/unit/useOnyxTest.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/tests/unit/useOnyxTest.ts b/tests/unit/useOnyxTest.ts index 2a58efa4..256c7e11 100644 --- a/tests/unit/useOnyxTest.ts +++ b/tests/unit/useOnyxTest.ts @@ -279,9 +279,7 @@ describe('useOnyx', () => { // In a react app we expect the selector ref to change during a rerender (see selectorRef/useLiveRef) rerender(undefined); - await act(async () => Onyx.merge(ONYXKEYS.TEST_KEY, {id: 'changed_id', name: 'changed_name'})); - - expect(result.current[0]).toEqual('id - changed_id, name - changed_name - selector changed'); + expect(result.current[0]).toEqual('id - test_id, name - test_name - selector changed'); expect(result.current[1].status).toEqual('loaded'); });