From a870e2037cc040ab820a0cce5b885ca726ea7f31 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Thu, 25 Apr 2024 13:11:39 +0200 Subject: [PATCH 1/6] improve code --- lib/OnyxUtils.ts | 38 +++++++++++++++++++------------------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/lib/OnyxUtils.ts b/lib/OnyxUtils.ts index fe1b9b3f..d2487f81 100644 --- a/lib/OnyxUtils.ts +++ b/lib/OnyxUtils.ts @@ -592,14 +592,14 @@ function keysChanged( */ function keyChanged( key: TKey, - data: OnyxValue, - prevData: OnyxValue, + value: OnyxValue, + previousValue: OnyxValue, canUpdateSubscriber: (subscriber?: Mapping) => 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 +624,14 @@ function keyChanged( const cachedCollection = getCachedCollection(subscriber.key); const cachedCollectionWithoutNestedNulls = utils.removeNestedNullValues(cachedCollection) as Record; - 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; - subscriberCallback(dataWithoutNestedNulls, key); + subscriberCallback(valueWithoutNestedNulls, key); continue; } @@ -650,7 +650,7 @@ function keyChanged( 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 +671,7 @@ function keyChanged( const collection = prevState[subscriber.statePropertyName] || {}; const newCollection = { ...collection, - [key]: data, + [key]: value, }; PerformanceUtils.logSetStateCall(subscriber, collection, newCollection, 'keyChanged', key); return { @@ -685,10 +685,10 @@ function keyChanged( // 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 +700,19 @@ function keyChanged( // 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 +866,11 @@ function getCollectionDataAndSendAsObject(matchingKeys: Co function scheduleSubscriberUpdate( key: TKey, value: OnyxValue, - prevValue: OnyxValue, + previousValue: OnyxValue, canUpdateSubscriber: (subscriber?: Mapping) => boolean = () => true, ): Promise { - 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); } From 6bab8be51c3369a998d79e288b9dcac7a8d69874 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Thu, 25 Apr 2024 13:11:52 +0200 Subject: [PATCH 2/6] add test for unnecessary subscriber updates --- tests/unit/onyxTest.js | 44 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/tests/unit/onyxTest.js b/tests/unit/onyxTest.js index faeda068..3a7b7d3c 100644 --- a/tests/unit/onyxTest.js +++ b/tests/unit/onyxTest.js @@ -11,6 +11,7 @@ const ONYX_KEYS = { TEST_CONNECT_COLLECTION: 'testConnectCollection_', TEST_POLICY: 'testPolicy_', TEST_UPDATE: 'testUpdate_', + ANIMALS: 'animals_', }, }; @@ -1145,4 +1146,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); + }); + }); }); From 89dbcde538fd4b1e20e9e7d7522f2c24fee4dc1a Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Thu, 25 Apr 2024 13:12:00 +0200 Subject: [PATCH 3/6] ignore dist folder in tests --- jest.config.js | 1 + 1 file changed, 1 insertion(+) diff --git a/jest.config.js b/jest.config.js index 7e201e45..3657ec7b 100644 --- a/jest.config.js +++ b/jest.config.js @@ -1,5 +1,6 @@ module.exports = { preset: 'react-native', + roots: ['/lib', '/tests'], transform: { '\\.[jt]sx?$': 'babel-jest', }, From e61013b7cdd0bf156bcb01b67b116731b9359811 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Thu, 25 Apr 2024 13:26:24 +0200 Subject: [PATCH 4/6] fix: subscribers updated when nothing changed --- lib/Onyx.ts | 8 ++++++-- lib/OnyxUtils.ts | 41 +++++++++++++++++++++++++++++++++-------- 2 files changed, 39 insertions(+), 10 deletions(-) diff --git a/lib/Onyx.ts b/lib/Onyx.ts index ff9927c1..f81c9b52 100644 --- a/lib/Onyx.ts +++ b/lib/Onyx.ts @@ -431,6 +431,10 @@ function mergeCollection(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) { @@ -443,9 +447,9 @@ function mergeCollection(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) diff --git a/lib/OnyxUtils.ts b/lib/OnyxUtils.ts index d2487f81..3264b2b3 100644 --- a/lib/OnyxUtils.ts +++ b/lib/OnyxUtils.ts @@ -422,9 +422,22 @@ function getCachedCollection(collectionKey: TKey function keysChanged( collectionKey: TKey, partialCollection: OnyxCollection, + previousPartialCollection: OnyxCollection | undefined, notifyRegularSubscibers = true, notifyWithOnyxSubscibers = true, ): void { + const previousCollectionWithoutNestedNulls = previousPartialCollection === undefined ? {} : (utils.removeNestedNullValues(previousPartialCollection) as Record); + + // 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; + + // 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( */ 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; - // Regular Onyx.connect() subscriber found. if (typeof subscriber.callback === 'function') { if (!notifyRegularSubscibers) { @@ -474,6 +482,11 @@ function keysChanged( 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( // 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; subscriberCallback(cachedCollectionWithoutNestedNulls[subscriber.key], subscriber.key as TKey); continue; @@ -535,6 +552,10 @@ function keysChanged( // 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]; @@ -879,9 +900,13 @@ function scheduleSubscriberUpdate( * 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(key: TKey, value: OnyxCollection): Promise { - const promise = Promise.resolve().then(() => keysChanged(key, value, true, false)); - batchUpdates(() => keysChanged(key, value, false, true)); +function scheduleNotifyCollectionSubscribers( + key: TKey, + value: OnyxCollection, + previousValue?: OnyxCollection, +): Promise { + 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); } From 7112ff7395fcb5860a654f1eff21a98c92fea0c3 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Thu, 25 Apr 2024 14:19:33 +0200 Subject: [PATCH 5/6] fix: test not waiting for onyx.connect to resolve --- tests/unit/onyxTest.js | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/tests/unit/onyxTest.js b/tests/unit/onyxTest.js index 3a7b7d3c..e71945b2 100644 --- a/tests/unit/onyxTest.js +++ b/tests/unit/onyxTest.js @@ -918,16 +918,20 @@ 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).toHaveBeenNthCalledWith(1, 'taco', ONYX_KEYS.TEST_KEY); + expect(otherTestCallback).toHaveBeenNthCalledWith(1, 'pizza', ONYX_KEYS.OTHER_TEST); + Onyx.disconnect(connectionIDs); + }), + ); }); it('should merge an object with a batch of objects and undefined', () => { From e7f21cf9c3fc443ea8150970dc6df5d48dd411e2 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Thu, 25 Apr 2024 20:28:48 +0200 Subject: [PATCH 6/6] fix: test --- tests/unit/onyxTest.js | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/tests/unit/onyxTest.js b/tests/unit/onyxTest.js index e71945b2..bcd72749 100644 --- a/tests/unit/onyxTest.js +++ b/tests/unit/onyxTest.js @@ -927,8 +927,15 @@ describe('Onyx', () => { expect(collectionCallback).toHaveBeenCalledTimes(2); expect(collectionCallback).toHaveBeenNthCalledWith(1, null, undefined); expect(collectionCallback).toHaveBeenNthCalledWith(2, {[itemKey]: {a: 'a'}}); - expect(testCallback).toHaveBeenNthCalledWith(1, 'taco', ONYX_KEYS.TEST_KEY); - expect(otherTestCallback).toHaveBeenNthCalledWith(1, 'pizza', ONYX_KEYS.OTHER_TEST); + + 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); }), );