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);
+ });
+ });
 });