Merge pull request #541 from margelo/@chrispader/collection-subscribe…

…r-updated-when-nothing-changed

Fix: collection subscriber updated when nothing changed

jest.config.js

@@ -1,5 +1,6 @@
 module.exports = {
     preset: 'react-native',
+    roots: ['<rootDir>/lib', '<rootDir>/tests'],
     transform: {
         '\\.[jt]sx?$': 'babel-jest',
     },

lib/Onyx.ts

@@ -459,6 +459,10 @@ function mergeCollection<TKey extends CollectionKeyBase, TMap>(collectionKey: TK
 
             const promises = [];
 
+            // We need to get the previously existing values so we can compare the new ones
+            // against them, to avoid unnecessary subscriber updates.
+            const previousCollectionPromise = Promise.all(existingKeys.map((key) => OnyxUtils.get(key).then((value) => [key, value]))).then(Object.fromEntries);
+
             // New keys will be added via multiSet while existing keys will be updated using multiMerge
             // This is because setting a key that doesn't exist yet with multiMerge will throw errors
             if (keyValuePairsForExistingCollection.length > 0) {
@@ -474,9 +478,9 @@ function mergeCollection<TKey extends CollectionKeyBase, TMap>(collectionKey: TK
 
             // Prefill cache if necessary by calling get() on any existing keys and then merge original data to cache
             // and update all subscribers
-            const promiseUpdate = Promise.all(existingKeys.map(OnyxUtils.get)).then(() => {
+            const promiseUpdate = previousCollectionPromise.then((previousCollection) => {
                 cache.merge(finalMergedCollection);
-                return OnyxUtils.scheduleNotifyCollectionSubscribers(collectionKey, finalMergedCollection);
+                return OnyxUtils.scheduleNotifyCollectionSubscribers(collectionKey, finalMergedCollection, previousCollection);
             });
 
             return Promise.all(promises)

lib/OnyxUtils.ts

@@ -422,9 +422,22 @@ function getCachedCollection<TKey extends CollectionKeyBase>(collectionKey: TKey
 function keysChanged<TKey extends CollectionKeyBase>(
     collectionKey: TKey,
     partialCollection: OnyxCollection<KeyValueMapping[TKey]>,
+    previousPartialCollection: OnyxCollection<KeyValueMapping[TKey]> | undefined,
     notifyRegularSubscibers = true,
     notifyWithOnyxSubscibers = true,
 ): void {
+    const previousCollectionWithoutNestedNulls = previousPartialCollection === undefined ? {} : (utils.removeNestedNullValues(previousPartialCollection) as Record<string, unknown>);
+
+    // We prepare the "cached collection" which is the entire collection + the new partial data that
+    // was merged in via mergeCollection().
+    const cachedCollection = getCachedCollection(collectionKey);
+    const cachedCollectionWithoutNestedNulls = utils.removeNestedNullValues(cachedCollection) as Record<string, unknown>;
+
+    // If the previous collection equals the new collection then we do not need to notify any subscribers.
+    if (previousPartialCollection !== undefined && deepEqual(cachedCollectionWithoutNestedNulls, previousCollectionWithoutNestedNulls)) {
+        return;
+    }
+
     // We are iterating over all subscribers similar to keyChanged(). However, we are looking for subscribers who are subscribing to either a collection key or
     // individual collection key member for the collection that is being updated. It is important to note that the collection parameter cane be a PARTIAL collection
     // and does not represent all of the combined keys and values for a collection key. It is just the "new" data that was merged in via mergeCollection().
@@ -450,11 +463,6 @@ function keysChanged<TKey extends CollectionKeyBase>(
          */
         const isSubscribedToCollectionMemberKey = isCollectionMemberKey(collectionKey, subscriber.key);
 
-        // We prepare the "cached collection" which is the entire collection + the new partial data that
-        // was merged in via mergeCollection().
-        const cachedCollection = getCachedCollection(collectionKey);
-        const cachedCollectionWithoutNestedNulls = utils.removeNestedNullValues(cachedCollection) as Record<string, unknown>;
-
         // Regular Onyx.connect() subscriber found.
         if (typeof subscriber.callback === 'function') {
             if (!notifyRegularSubscibers) {
@@ -474,6 +482,11 @@ function keysChanged<TKey extends CollectionKeyBase>(
                 const dataKeys = Object.keys(partialCollection ?? {});
                 for (let j = 0; j < dataKeys.length; j++) {
                     const dataKey = dataKeys[j];
+
+                    if (deepEqual(cachedCollectionWithoutNestedNulls[dataKey], previousCollectionWithoutNestedNulls[dataKey])) {
+                        continue;
+                    }
+
                     subscriber.callback(cachedCollectionWithoutNestedNulls[dataKey], dataKey);
                 }
                 continue;
@@ -482,6 +495,10 @@ function keysChanged<TKey extends CollectionKeyBase>(
             // And if the subscriber is specifically only tracking a particular collection member key then we will
             // notify them with the cached data for that key only.
             if (isSubscribedToCollectionMemberKey) {
+                if (deepEqual(cachedCollectionWithoutNestedNulls[subscriber.key], previousCollectionWithoutNestedNulls[subscriber.key])) {
+                    continue;
+                }
+
                 const subscriberCallback = subscriber.callback as DefaultConnectCallback<TKey>;
                 subscriberCallback(cachedCollectionWithoutNestedNulls[subscriber.key], subscriber.key as TKey);
                 continue;
@@ -535,6 +552,10 @@ function keysChanged<TKey extends CollectionKeyBase>(
 
             // If a React component is only interested in a single key then we can set the cached value directly to the state name.
             if (isSubscribedToCollectionMemberKey) {
+                if (deepEqual(cachedCollectionWithoutNestedNulls[subscriber.key], previousCollectionWithoutNestedNulls[subscriber.key])) {
+                    continue;
+                }
+
                 // However, we only want to update this subscriber if the partial data contains a change.
                 // Otherwise, we would update them with a value they already have and trigger an unnecessary re-render.
                 const dataFromCollection = partialCollection?.[subscriber.key];
@@ -592,14 +613,14 @@ function keysChanged<TKey extends CollectionKeyBase>(
  */
 function keyChanged<TKey extends OnyxKey>(
     key: TKey,
-    data: OnyxValue<TKey>,
-    prevData: OnyxValue<TKey>,
+    value: OnyxValue<TKey>,
+    previousValue: OnyxValue<TKey>,
     canUpdateSubscriber: (subscriber?: Mapping<OnyxKey>) => boolean = () => true,
     notifyRegularSubscibers = true,
     notifyWithOnyxSubscibers = true,
 ): void {
     // Add or remove this key from the recentlyAccessedKeys lists
-    if (data !== null) {
+    if (value !== null) {
         addLastAccessedKey(key);
     } else {
         removeLastAccessedKey(key);
@@ -624,14 +645,14 @@ function keyChanged<TKey extends OnyxKey>(
                 const cachedCollection = getCachedCollection(subscriber.key);
                 const cachedCollectionWithoutNestedNulls = utils.removeNestedNullValues(cachedCollection) as Record<string, unknown>;
 
-                cachedCollectionWithoutNestedNulls[key] = data;
+                cachedCollectionWithoutNestedNulls[key] = value;
                 subscriber.callback(cachedCollectionWithoutNestedNulls);
                 continue;
             }
 
-            const dataWithoutNestedNulls = utils.removeNestedNullValues(data);
+            const valueWithoutNestedNulls = utils.removeNestedNullValues(value);
             const subscriberCallback = subscriber.callback as DefaultConnectCallback<TKey>;
-            subscriberCallback(dataWithoutNestedNulls, key);
+            subscriberCallback(valueWithoutNestedNulls, key);
             continue;
         }
 
@@ -650,7 +671,7 @@ function keyChanged<TKey extends OnyxKey>(
                     subscriber.withOnyxInstance.setStateProxy((prevState) => {
                         const prevWithOnyxData = prevState[subscriber.statePropertyName];
                         const newWithOnyxData = {
-                            [key]: selector(data, subscriber.withOnyxInstance.state),
+                            [key]: selector(value, subscriber.withOnyxInstance.state),
                         };
                         const prevDataWithNewData = {
                             ...prevWithOnyxData,
@@ -671,7 +692,7 @@ function keyChanged<TKey extends OnyxKey>(
                     const collection = prevState[subscriber.statePropertyName] || {};
                     const newCollection = {
                         ...collection,
-                        [key]: data,
+                        [key]: value,
                     };
                     PerformanceUtils.logSetStateCall(subscriber, collection, newCollection, 'keyChanged', key);
                     return {
@@ -685,10 +706,10 @@ function keyChanged<TKey extends OnyxKey>(
             // returned by the selector and only if the selected data has changed.
             if (selector) {
                 subscriber.withOnyxInstance.setStateProxy(() => {
-                    const previousValue = selector(prevData, subscriber.withOnyxInstance.state);
-                    const newValue = selector(data, subscriber.withOnyxInstance.state);
+                    const prevValue = selector(previousValue, subscriber.withOnyxInstance.state);
+                    const newValue = selector(value, subscriber.withOnyxInstance.state);
 
-                    if (!deepEqual(previousValue, newValue)) {
+                    if (!deepEqual(prevValue, newValue)) {
                         return {
                             [subscriber.statePropertyName]: newValue,
                         };
@@ -700,19 +721,19 @@ function keyChanged<TKey extends OnyxKey>(
 
             // If we did not match on a collection key then we just set the new data to the state property
             subscriber.withOnyxInstance.setStateProxy((prevState) => {
-                const prevWithOnyxData = prevState[subscriber.statePropertyName];
+                const prevWithOnyxValue = prevState[subscriber.statePropertyName];
 
                 // Avoids triggering unnecessary re-renders when feeding empty objects
-                if (utils.isEmptyObject(data) && utils.isEmptyObject(prevWithOnyxData)) {
+                if (utils.isEmptyObject(value) && utils.isEmptyObject(prevWithOnyxValue)) {
                     return null;
                 }
-                if (prevWithOnyxData === data) {
+                if (prevWithOnyxValue === value) {
                     return null;
                 }
 
-                PerformanceUtils.logSetStateCall(subscriber, prevData, data, 'keyChanged', key);
+                PerformanceUtils.logSetStateCall(subscriber, previousValue, value, 'keyChanged', key);
                 return {
-                    [subscriber.statePropertyName]: data,
+                    [subscriber.statePropertyName]: value,
                 };
             });
             continue;
@@ -866,11 +887,11 @@ function getCollectionDataAndSendAsObject<TKey extends OnyxKey>(matchingKeys: Co
 function scheduleSubscriberUpdate<TKey extends OnyxKey>(
     key: TKey,
     value: OnyxValue<TKey>,
-    prevValue: OnyxValue<TKey>,
+    previousValue: OnyxValue<TKey>,
     canUpdateSubscriber: (subscriber?: Mapping<OnyxKey>) => boolean = () => true,
 ): Promise<void> {
-    const promise = Promise.resolve().then(() => keyChanged(key, value, prevValue, canUpdateSubscriber, true, false));
-    batchUpdates(() => keyChanged(key, value, prevValue, canUpdateSubscriber, false, true));
+    const promise = Promise.resolve().then(() => keyChanged(key, value, previousValue, canUpdateSubscriber, true, false));
+    batchUpdates(() => keyChanged(key, value, previousValue, canUpdateSubscriber, false, true));
     return Promise.all([maybeFlushBatchUpdates(), promise]).then(() => undefined);
 }
 
@@ -879,9 +900,13 @@ function scheduleSubscriberUpdate<TKey extends OnyxKey>(
  * so that keysChanged() is triggered for the collection and not keyChanged(). If this was not done, then the
  * subscriber callbacks receive the data in a different format than they normally expect and it breaks code.
  */
-function scheduleNotifyCollectionSubscribers<TKey extends OnyxKey>(key: TKey, value: OnyxCollection<KeyValueMapping[TKey]>): Promise<void> {
-    const promise = Promise.resolve().then(() => keysChanged(key, value, true, false));
-    batchUpdates(() => keysChanged(key, value, false, true));
+function scheduleNotifyCollectionSubscribers<TKey extends OnyxKey>(
+    key: TKey,
+    value: OnyxCollection<KeyValueMapping[TKey]>,
+    previousValue?: OnyxCollection<KeyValueMapping[TKey]>,
+): Promise<void> {
+    const promise = Promise.resolve().then(() => keysChanged(key, value, previousValue, true, false));
+    batchUpdates(() => keysChanged(key, value, previousValue, false, true));
     return Promise.all([maybeFlushBatchUpdates(), promise]).then(() => undefined);
 }
 

tests/unit/onyxTest.js

@@ -11,6 +11,7 @@ const ONYX_KEYS = {
         TEST_CONNECT_COLLECTION: 'testConnectCollection_',
         TEST_POLICY: 'testPolicy_',
         TEST_UPDATE: 'testUpdate_',
+        ANIMALS: 'animals_',
     },
 };
 
@@ -960,16 +961,27 @@ describe('Onyx', () => {
         connectionIDs.push(Onyx.connect({key: ONYX_KEYS.TEST_KEY, callback: testCallback}));
         connectionIDs.push(Onyx.connect({key: ONYX_KEYS.OTHER_TEST, callback: otherTestCallback}));
         connectionIDs.push(Onyx.connect({key: ONYX_KEYS.COLLECTION.TEST_UPDATE, callback: collectionCallback, waitForCollectionCallback: true}));
-        return Onyx.update([
-            {onyxMethod: Onyx.METHOD.SET, key: ONYX_KEYS.TEST_KEY, value: 'taco'},
-            {onyxMethod: Onyx.METHOD.MERGE, key: ONYX_KEYS.OTHER_TEST, value: 'pizza'},
-            {onyxMethod: Onyx.METHOD.MERGE_COLLECTION, key: ONYX_KEYS.COLLECTION.TEST_UPDATE, value: {[itemKey]: {a: 'a'}}},
-        ]).then(() => {
-            expect(collectionCallback).toHaveBeenNthCalledWith(1, {[itemKey]: {a: 'a'}});
-            expect(testCallback).toHaveBeenNthCalledWith(1, 'taco', ONYX_KEYS.TEST_KEY);
-            expect(otherTestCallback).toHaveBeenNthCalledWith(1, 'pizza', ONYX_KEYS.OTHER_TEST);
-            Onyx.disconnect(connectionIDs);
-        });
+        return waitForPromisesToResolve().then(() =>
+            Onyx.update([
+                {onyxMethod: Onyx.METHOD.SET, key: ONYX_KEYS.TEST_KEY, value: 'taco'},
+                {onyxMethod: Onyx.METHOD.MERGE, key: ONYX_KEYS.OTHER_TEST, value: 'pizza'},
+                {onyxMethod: Onyx.METHOD.MERGE_COLLECTION, key: ONYX_KEYS.COLLECTION.TEST_UPDATE, value: {[itemKey]: {a: 'a'}}},
+            ]).then(() => {
+                expect(collectionCallback).toHaveBeenCalledTimes(2);
+                expect(collectionCallback).toHaveBeenNthCalledWith(1, null, undefined);
+                expect(collectionCallback).toHaveBeenNthCalledWith(2, {[itemKey]: {a: 'a'}});
+
+                expect(testCallback).toHaveBeenCalledTimes(2);
+                expect(testCallback).toHaveBeenNthCalledWith(1, null, undefined);
+                expect(testCallback).toHaveBeenNthCalledWith(2, 'taco', ONYX_KEYS.TEST_KEY);
+
+                expect(otherTestCallback).toHaveBeenCalledTimes(2);
+                // We set an initial value of 42 for ONYX_KEYS.OTHER_TEST in Onyx.init()
+                expect(otherTestCallback).toHaveBeenNthCalledWith(1, 42, ONYX_KEYS.OTHER_TEST);
+                expect(otherTestCallback).toHaveBeenNthCalledWith(2, 'pizza', ONYX_KEYS.OTHER_TEST);
+                Onyx.disconnect(connectionIDs);
+            }),
+        );
     });
 
     it('should merge an object with a batch of objects and undefined', () => {
@@ -1188,4 +1200,47 @@ describe('Onyx', () => {
             });
         });
     });
+
+    it('should not call a collection item subscriber if the value did not change', () => {
+        const connectionIDs = [];
+
+        const cat = `${ONYX_KEYS.COLLECTION.ANIMALS}cat`;
+        const dog = `${ONYX_KEYS.COLLECTION.ANIMALS}dog`;
+
+        const collectionCallback = jest.fn();
+        const catCallback = jest.fn();
+        const dogCallback = jest.fn();
+
+        connectionIDs.push(
+            Onyx.connect({
+                key: ONYX_KEYS.COLLECTION.ANIMALS,
+                callback: collectionCallback,
+                waitForCollectionCallback: true,
+            }),
+        );
+        connectionIDs.push(Onyx.connect({key: cat, callback: catCallback}));
+        connectionIDs.push(Onyx.connect({key: dog, callback: dogCallback}));
+
+        const initialValue = {name: 'Fluffy'};
+
+        const collectionDiff = {
+            [cat]: initialValue,
+            [dog]: {name: 'Rex'},
+        };
+
+        return Onyx.set(cat, initialValue)
+            .then(() => {
+                Onyx.mergeCollection(ONYX_KEYS.COLLECTION.ANIMALS, collectionDiff);
+                return waitForPromisesToResolve();
+            })
+            .then(() => {
+                expect(collectionCallback).toHaveBeenCalledTimes(3);
+                expect(collectionCallback).toHaveBeenCalledWith(collectionDiff);
+
+                expect(catCallback).toHaveBeenCalledTimes(2);
+                expect(dogCallback).toHaveBeenCalledTimes(2);
+
+                _.map(connectionIDs, Onyx.disconnect);
+            });
+    });
 });