Merge pull request #570 from margelo/fix/connection-called-twice-on-set

fix: connection could be called twice on set

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

lib/OnyxUtils.ts

@@ -72,6 +72,9 @@ let defaultKeyStates: Record<OnyxKey, OnyxValue<OnyxKey>> = {};
 let batchUpdatesPromise: Promise<void> | null = null;
 let batchUpdatesQueue: Array<() => void> = [];
 
+// Used for comparison with a new update to avoid invoking the Onyx.connect callback with the same data.
+const lastConnectionCallbackData = new Map<number, OnyxValue<OnyxKey>>();
+
 let snapshotKey: OnyxKey | null = null;
 
 function getSnapshotKey(): OnyxKey | null {
@@ -774,8 +777,8 @@ function keyChanged<TKey extends OnyxKey>(
     value: OnyxValue<TKey>,
     previousValue: OnyxValue<TKey>,
     canUpdateSubscriber: (subscriber?: Mapping<OnyxKey>) => 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<TKey extends OnyxKey>(
 
         // 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<TKey extends OnyxKey>(
 
             const subscriberCallback = subscriber.callback as DefaultConnectCallback<TKey>;
             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,7 +969,16 @@ function sendDataToConnection<TKey extends OnyxKey>(mapping: Mapping<TKey>, 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.
-    (mapping as DefaultConnectOptions<TKey>).callback?.(value === null ? undefined : value, matchedKey as TKey);
+    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) {
+        return;
+    }
+
+    (mapping as DefaultConnectOptions<TKey>).callback?.(valueToPass, matchedKey as TKey);
 }
 
 /**

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;
 
@@ -153,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);
 
@@ -1365,7 +1385,10 @@ 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));

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,10 +276,10 @@ 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'}));
-
-            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');
         });