Skip to content

Commit

Permalink
fix: subscribers updated when nothing changed
Browse files Browse the repository at this point in the history
  • Loading branch information
chrispader committed Apr 25, 2024
1 parent 89dbcde commit e61013b
Show file tree
Hide file tree
Showing 2 changed files with 39 additions and 10 deletions.
8 changes: 6 additions & 2 deletions lib/Onyx.ts
Original file line number Diff line number Diff line change
Expand Up @@ -431,6 +431,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) {
Expand All @@ -443,9 +447,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(mergedCollection);
return OnyxUtils.scheduleNotifyCollectionSubscribers(collectionKey, mergedCollection);
return OnyxUtils.scheduleNotifyCollectionSubscribers(collectionKey, mergedCollection, previousCollection);
});

return Promise.all(promises)
Expand Down
41 changes: 33 additions & 8 deletions lib/OnyxUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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().
Expand All @@ -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) {
Expand All @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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];
Expand Down Expand Up @@ -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);
}

Expand Down

0 comments on commit e61013b

Please sign in to comment.