diff --git a/lib/Onyx.ts b/lib/Onyx.ts index 5b9065c7e..a826bf44c 100644 --- a/lib/Onyx.ts +++ b/lib/Onyx.ts @@ -162,6 +162,13 @@ function disconnect(connection: Connection): void { * @param options optional configuration object */ function set(key: TKey, value: OnyxSetInput, options?: SetOptions): Promise { + return setInternal(key, value, options); +} +/** + * @param isFromUpdate - Whether this call originates from Onyx.update() + * When isFromUpdate = true (called from Onyx.update()), useOnyx hook subscribers are batched to escape excessive re-renders in components + */ +function setInternal(key: TKey, value: OnyxSetInput, options?: SetOptions, isFromUpdate = false): Promise { // When we use Onyx.set to set a key we want to clear the current delta changes from Onyx.merge that were queued // before the value was set. If Onyx.merge is currently reading the old value from storage, it will then not apply the changes. if (OnyxUtils.hasPendingMergeForKey(key)) { @@ -214,7 +221,7 @@ function set(key: TKey, value: OnyxSetInput, options OnyxUtils.logKeyChanged(OnyxUtils.METHOD.SET, key, value, hasChanged); // This approach prioritizes fast UI changes without waiting for data to be stored in device storage. - const updatePromise = OnyxUtils.broadcastUpdate(key, valueWithoutNestedNullValues, hasChanged); + const updatePromise = OnyxUtils.broadcastUpdate(key, valueWithoutNestedNullValues, hasChanged, isFromUpdate); // If the value has not changed or the key got removed, calling Storage.setItem() would be redundant and a waste of performance, so return early instead. if (!hasChanged) { @@ -222,7 +229,7 @@ function set(key: TKey, value: OnyxSetInput, options } return Storage.setItem(key, valueWithoutNestedNullValues) - .catch((error) => OnyxUtils.evictStorageAndRetry(error, set, key, valueWithoutNestedNullValues)) + .catch((error) => OnyxUtils.evictStorageAndRetry(error, setInternal, key, valueWithoutNestedNullValues, undefined, isFromUpdate)) .then(() => { OnyxUtils.sendActionToDevTools(OnyxUtils.METHOD.SET, key, valueWithoutNestedNullValues); return updatePromise; @@ -237,6 +244,13 @@ function set(key: TKey, value: OnyxSetInput, options * @param data object keyed by ONYXKEYS and the values to set */ function multiSet(data: OnyxMultiSetInput): Promise { + return multiSetInternal(data); +} +/** + * @param isFromUpdate - Whether this call originates from Onyx.update() + * When isFromUpdate = true (called from Onyx.update()), useOnyx hook subscribers are batched to escape excessive re-renders in components + */ +function multiSetInternal(data: OnyxMultiSetInput, isFromUpdate = false): Promise { let newData = data; const skippableCollectionMemberIDs = OnyxUtils.getSkippableCollectionMemberIDs(); @@ -269,11 +283,11 @@ function multiSet(data: OnyxMultiSetInput): Promise { // Update cache and optimistically inform subscribers on the next tick cache.set(key, value); - return OnyxUtils.scheduleSubscriberUpdate(key, value, prevValue); + return OnyxUtils.scheduleSubscriberUpdate(key, value, prevValue, undefined, isFromUpdate); }); return Storage.multiSet(keyValuePairsToSet) - .catch((error) => OnyxUtils.evictStorageAndRetry(error, multiSet, newData)) + .catch((error) => OnyxUtils.evictStorageAndRetry(error, multiSetInternal, newData, isFromUpdate)) .then(() => { OnyxUtils.sendActionToDevTools(OnyxUtils.METHOD.MULTI_SET, undefined, newData); return Promise.all(updatePromises); @@ -298,6 +312,13 @@ function multiSet(data: OnyxMultiSetInput): Promise { * Onyx.merge(ONYXKEYS.POLICY, {name: 'My Workspace'}); // -> {id: 1, name: 'My Workspace'} */ function merge(key: TKey, changes: OnyxMergeInput): Promise { + return mergeInternal(key, changes); +} +/** + * @param isFromUpdate - Whether this call originates from Onyx.update() + * When isFromUpdate = true (called from Onyx.update()), useOnyx hook subscribers are batched to escape excessive re-renders in components + */ +function mergeInternal(key: TKey, changes: OnyxMergeInput, isFromUpdate = false): Promise { const skippableCollectionMemberIDs = OnyxUtils.getSkippableCollectionMemberIDs(); if (skippableCollectionMemberIDs.size) { try { @@ -360,7 +381,7 @@ function merge(key: TKey, changes: OnyxMergeInput): return Promise.resolve(); } - return OnyxMerge.applyMerge(key, existingValue, validChanges).then(({mergedValue, updatePromise}) => { + return OnyxMerge.applyMerge(key, existingValue, validChanges, isFromUpdate).then(({mergedValue, updatePromise}) => { OnyxUtils.sendActionToDevTools(OnyxUtils.METHOD.MERGE, key, changes, mergedValue); return updatePromise; }); @@ -387,7 +408,14 @@ function merge(key: TKey, changes: OnyxMergeInput): * @param collection Object collection keyed by individual collection member keys and values */ function mergeCollection(collectionKey: TKey, collection: OnyxMergeCollectionInput): Promise { - return OnyxUtils.mergeCollectionWithPatches(collectionKey, collection); + return mergeCollectionInternal(collectionKey, collection); +} +/** + * @param isFromUpdate - Whether this call originates from Onyx.update() + * When isFromUpdate = true (called from Onyx.update()), useOnyx hook subscribers are batched to escape excessive re-renders in components + */ +function mergeCollectionInternal(collectionKey: TKey, collection: OnyxMergeCollectionInput, isFromUpdate = false): Promise { + return OnyxUtils.mergeCollectionWithPatches(collectionKey, collection, undefined, isFromUpdate); } /** @@ -476,10 +504,10 @@ function clear(keysToPreserve: OnyxKey[] = []): Promise { // Notify the subscribers for each key/value group so they can receive the new values Object.entries(keyValuesToResetIndividually).forEach(([key, value]) => { - updatePromises.push(OnyxUtils.scheduleSubscriberUpdate(key, value, cache.get(key, false))); + updatePromises.push(OnyxUtils.scheduleSubscriberUpdate(key, value, cache.get(key, false), undefined, false)); }); Object.entries(keyValuesToResetAsCollection).forEach(([key, value]) => { - updatePromises.push(OnyxUtils.scheduleNotifyCollectionSubscribers(key, value)); + updatePromises.push(OnyxUtils.scheduleNotifyCollectionSubscribers(key, value, undefined, false)); }); const defaultKeyValuePairs = Object.entries( @@ -570,7 +598,7 @@ function update(data: OnyxUpdate[]): Promise { collectionKeys.forEach((collectionKey) => enqueueMergeOperation(collectionKey, mergedCollection[collectionKey])); } }, - [OnyxUtils.METHOD.SET_COLLECTION]: (k, v) => promises.push(() => setCollection(k, v as Collection)), + [OnyxUtils.METHOD.SET_COLLECTION]: (k, v) => promises.push(() => setCollectionInternal(k, v as Collection, true)), [OnyxUtils.METHOD.MULTI_SET]: (k, v) => Object.entries(v as Partial).forEach(([entryKey, entryValue]) => enqueueSetOperation(entryKey, entryValue)), [OnyxUtils.METHOD.CLEAR]: () => { clearPromise = clear(); @@ -625,27 +653,28 @@ function update(data: OnyxUpdate[]): Promise { collectionKey, batchedCollectionUpdates.merge as Collection, batchedCollectionUpdates.mergeReplaceNullPatches, + true, ), ); } if (!utils.isEmptyObject(batchedCollectionUpdates.set)) { - promises.push(() => multiSet(batchedCollectionUpdates.set)); + promises.push(() => multiSetInternal(batchedCollectionUpdates.set, true)); } }); Object.entries(updateQueue).forEach(([key, operations]) => { if (operations[0] === null) { const batchedChanges = OnyxUtils.mergeChanges(operations).result; - promises.push(() => set(key, batchedChanges)); + promises.push(() => setInternal(key, batchedChanges, undefined, true)); return; } operations.forEach((operation) => { - promises.push(() => merge(key, operation)); + promises.push(() => mergeInternal(key, operation, true)); }); }); - const snapshotPromises = OnyxUtils.updateSnapshots(data, merge); + const snapshotPromises = OnyxUtils.updateSnapshots(data, (key, changes) => mergeInternal(key, changes, true)); // We need to run the snapshot updates before the other updates so the snapshot data can be updated before the loading state in the snapshot const finalPromises = snapshotPromises.concat(promises); @@ -667,6 +696,13 @@ function update(data: OnyxUpdate[]): Promise { * @param collection Object collection keyed by individual collection member keys and values */ function setCollection(collectionKey: TKey, collection: OnyxMergeCollectionInput): Promise { + return setCollectionInternal(collectionKey, collection); +} +/** + * @param isFromUpdate - Whether this call originates from Onyx.update() + * When isFromUpdate = true (called from Onyx.update()), useOnyx hook subscribers are batched to escape excessive re-renders in components + */ +function setCollectionInternal(collectionKey: TKey, collection: OnyxMergeCollectionInput, isFromUpdate = false): Promise { let resultCollection: OnyxInputKeyValueMapping = collection; let resultCollectionKeys = Object.keys(resultCollection); @@ -714,10 +750,10 @@ function setCollection(collectionKey: TKey keyValuePairs.forEach(([key, value]) => cache.set(key, value)); - const updatePromise = OnyxUtils.scheduleNotifyCollectionSubscribers(collectionKey, mutableCollection, previousCollection); + const updatePromise = OnyxUtils.scheduleNotifyCollectionSubscribers(collectionKey, mutableCollection, previousCollection, isFromUpdate); return Storage.multiSet(keyValuePairs) - .catch((error) => OnyxUtils.evictStorageAndRetry(error, setCollection, collectionKey, collection)) + .catch((error) => OnyxUtils.evictStorageAndRetry(error, setCollectionInternal, collectionKey, collection, isFromUpdate)) .then(() => { OnyxUtils.sendActionToDevTools(OnyxUtils.METHOD.SET_COLLECTION, undefined, mutableCollection); return updatePromise; diff --git a/lib/OnyxMerge/index.native.ts b/lib/OnyxMerge/index.native.ts index b796dfde4..03ea36a2a 100644 --- a/lib/OnyxMerge/index.native.ts +++ b/lib/OnyxMerge/index.native.ts @@ -8,6 +8,7 @@ const applyMerge: ApplyMerge = { // If any of the changes is null, we need to discard the existing value. const baseValue = validChanges.includes(null as TChange) ? undefined : existingValue; @@ -26,7 +27,7 @@ const applyMerge: ApplyMerge = , hasChanged); + const updatePromise = OnyxUtils.broadcastUpdate(key, mergedValue as OnyxValue, hasChanged, isFromUpdate); // If the value has not changed, calling Storage.setItem() would be redundant and a waste of performance, so return early instead. if (!hasChanged) { diff --git a/lib/OnyxMerge/index.ts b/lib/OnyxMerge/index.ts index 2a648499b..e34dd1095 100644 --- a/lib/OnyxMerge/index.ts +++ b/lib/OnyxMerge/index.ts @@ -8,6 +8,7 @@ const applyMerge: ApplyMerge = { const {result: mergedValue} = OnyxUtils.mergeChanges(validChanges, existingValue); @@ -18,7 +19,7 @@ const applyMerge: ApplyMerge = , hasChanged); + const updatePromise = OnyxUtils.broadcastUpdate(key, mergedValue as OnyxValue, hasChanged, isFromUpdate); // If the value has not changed, calling Storage.setItem() would be redundant and a waste of performance, so return early instead. if (!hasChanged) { diff --git a/lib/OnyxMerge/types.ts b/lib/OnyxMerge/types.ts index c59b7892a..ab246dbab 100644 --- a/lib/OnyxMerge/types.ts +++ b/lib/OnyxMerge/types.ts @@ -9,6 +9,7 @@ type ApplyMerge = | und key: TKey, existingValue: TValue, validChanges: TChange[], + isFromUpdate?: boolean, ) => Promise>; export type {ApplyMerge, ApplyMergeResult}; diff --git a/lib/OnyxUtils.ts b/lib/OnyxUtils.ts index c777d30ac..63a001fbd 100644 --- a/lib/OnyxUtils.ts +++ b/lib/OnyxUtils.ts @@ -622,6 +622,7 @@ function keysChanged( partialPreviousCollection: OnyxCollection | undefined, notifyConnectSubscribers = true, notifyWithOnyxSubscribers = true, + notifyUseOnyxHookSubscribers = true, ): void { // We prepare the "cached collection" which is the entire collection + the new partial data that // was merged in via mergeCollection(). @@ -657,7 +658,8 @@ function keysChanged( // Regular Onyx.connect() subscriber found. if (typeof subscriber.callback === 'function') { - if (!notifyConnectSubscribers) { + // Check if it's a useOnyx or a regular Onyx.connect() subscriber + if ((subscriber.isUseOnyxSubscriber && !notifyUseOnyxHookSubscribers) || (!subscriber.isUseOnyxSubscriber && !notifyConnectSubscribers)) { continue; } @@ -816,6 +818,7 @@ function keyChanged( canUpdateSubscriber: (subscriber?: Mapping) => boolean = () => true, notifyConnectSubscribers = true, notifyWithOnyxSubscribers = true, + notifyUseOnyxHookSubscribers = true, ): void { // Add or remove this key from the recentlyAccessedKeys lists if (value !== null) { @@ -857,7 +860,8 @@ function keyChanged( // Subscriber is a regular call to connect() and provided a callback if (typeof subscriber.callback === 'function') { - if (!notifyConnectSubscribers) { + // Check if it's a useOnyx or a regular Onyx.connect() subscriber + if ((subscriber.isUseOnyxSubscriber && !notifyUseOnyxHookSubscribers) || (!subscriber.isUseOnyxSubscriber && !notifyConnectSubscribers)) { continue; } if (lastConnectionCallbackData.has(subscriber.subscriptionID) && lastConnectionCallbackData.get(subscriber.subscriptionID) === value) { @@ -1077,9 +1081,10 @@ function scheduleSubscriberUpdate( value: OnyxValue, previousValue: OnyxValue, canUpdateSubscriber: (subscriber?: Mapping) => boolean = () => true, + isFromUpdate = false, ): Promise { - const promise = Promise.resolve().then(() => keyChanged(key, value, previousValue, canUpdateSubscriber, true, false)); - batchUpdates(() => keyChanged(key, value, previousValue, canUpdateSubscriber, false, true)); + const promise = Promise.resolve().then(() => keyChanged(key, value, previousValue, canUpdateSubscriber, true, false, !isFromUpdate)); + batchUpdates(() => keyChanged(key, value, previousValue, canUpdateSubscriber, false, true, isFromUpdate)); return Promise.all([maybeFlushBatchUpdates(), promise]).then(() => undefined); } @@ -1092,9 +1097,10 @@ function scheduleNotifyCollectionSubscribers( key: TKey, value: OnyxCollection, previousValue?: OnyxCollection, + isFromUpdate = false, ): Promise { - const promise = Promise.resolve().then(() => keysChanged(key, value, previousValue, true, false)); - batchUpdates(() => keysChanged(key, value, previousValue, false, true)); + const promise = Promise.resolve().then(() => keysChanged(key, value, previousValue, true, false, !isFromUpdate)); + batchUpdates(() => keysChanged(key, value, previousValue, false, true, isFromUpdate)); return Promise.all([maybeFlushBatchUpdates(), promise]).then(() => undefined); } @@ -1156,7 +1162,7 @@ function evictStorageAndRetry(key: TKey, value: OnyxValue, hasChanged?: boolean): Promise { +function broadcastUpdate(key: TKey, value: OnyxValue, hasChanged?: boolean, isFromUpdate = false): Promise { const prevValue = cache.get(key, false) as OnyxValue; // Update subscribers if the cached value has changed, or when the subscriber specifically requires @@ -1167,7 +1173,7 @@ function broadcastUpdate(key: TKey, value: OnyxValue cache.addToAccessedKeys(key); } - return scheduleSubscriberUpdate(key, value, prevValue, (subscriber) => hasChanged || subscriber?.initWithStoredValues === false).then(() => undefined); + return scheduleSubscriberUpdate(key, value, prevValue, (subscriber) => hasChanged || subscriber?.initWithStoredValues === false, isFromUpdate).then(() => undefined); } function hasPendingMergeForKey(key: OnyxKey): boolean { @@ -1514,6 +1520,7 @@ function mergeCollectionWithPatches( collectionKey: TKey, collection: OnyxMergeCollectionInput, mergeReplaceNullPatches?: MultiMergeReplaceNullPatches, + isFromUpdate = false, ): Promise { if (!isValidNonEmptyCollectionForMerge(collection)) { Logger.logInfo('mergeCollection() called with invalid or empty value. Skipping this update.'); @@ -1614,7 +1621,7 @@ function mergeCollectionWithPatches( // and update all subscribers const promiseUpdate = previousCollectionPromise.then((previousCollection) => { cache.merge(finalMergedCollection); - return scheduleNotifyCollectionSubscribers(collectionKey, finalMergedCollection, previousCollection); + return scheduleNotifyCollectionSubscribers(collectionKey, finalMergedCollection, previousCollection, isFromUpdate); }); return Promise.all(promises) diff --git a/lib/types.ts b/lib/types.ts index 07b6ddd3c..e9b9ae6c5 100644 --- a/lib/types.ts +++ b/lib/types.ts @@ -273,6 +273,9 @@ type BaseConnectOptions = { * with the same connect configurations. */ reuseConnection?: boolean; + + /** Indicates whether this subscriber is created from the useOnyx hook. */ + isUseOnyxSubscriber?: boolean; }; /** Represents the callback function used in `Onyx.connect()` method with a regular key. */ diff --git a/lib/useOnyx.ts b/lib/useOnyx.ts index 3bf238ead..462a61899 100644 --- a/lib/useOnyx.ts +++ b/lib/useOnyx.ts @@ -359,6 +359,7 @@ function useOnyx>( initWithStoredValues: options?.initWithStoredValues, waitForCollectionCallback: OnyxUtils.isCollectionKey(key) as true, reuseConnection: options?.reuseConnection, + isUseOnyxSubscriber: true, }); checkEvictableKey(); diff --git a/tests/unit/useOnyxTest.ts b/tests/unit/useOnyxTest.tsx similarity index 95% rename from tests/unit/useOnyxTest.ts rename to tests/unit/useOnyxTest.tsx index afaa6d66f..881208f59 100644 --- a/tests/unit/useOnyxTest.ts +++ b/tests/unit/useOnyxTest.tsx @@ -1,5 +1,7 @@ -import {act, renderHook} from '@testing-library/react-native'; -import type {OnyxCollection, OnyxEntry} from '../../lib'; +import {act, renderHook, render} from '@testing-library/react-native'; +import React from 'react'; +import {configure as configureRNTL, resetToDefaults as resetRNTLToDefaults} from '@testing-library/react-native/build/config'; +import type {OnyxCollection, OnyxEntry, OnyxKey} from '../../lib'; import Onyx, {useOnyx} from '../../lib'; import OnyxCache from '../../lib/OnyxCache'; import StorageMock from '../../lib/storage'; @@ -1153,6 +1155,73 @@ describe('useOnyx', () => { }); }); + describe('batching', () => { + // Temporarily disable concurrent rendering to isolate and test batching behavior independently of React's concurrent features + beforeAll(() => { + configureRNTL({ + concurrentRoot: false, + }); + }); + afterAll(() => { + resetRNTLToDefaults(); + }); + + function TestUseOnyxComponent({onRender, onyxKey1, onyxKey2}: {onRender: jest.Mock; onyxKey1: OnyxKey; onyxKey2: OnyxKey}) { + const [value1] = useOnyx(onyxKey1); + const [value2] = useOnyx(onyxKey2); + + React.useEffect(() => { + onRender({value1, value2}); + }); + + return null; + } + + it('re-renders once when two subscribed keys are updated in one Onyx.update', async () => { + const onRender = jest.fn(); + const testKeyValues = { + 1: {id: 1, value: '1'}, + 2: {id: 2, value: '2'}, + }; + const testKey2Value = {id: 10, value: '10'}; + + render( + , + ); + + await act(async () => waitForPromisesToResolve()); + onRender.mockClear(); + + await act(async () => { + Onyx.update([ + { + onyxMethod: Onyx.METHOD.MERGE_COLLECTION, + key: ONYXKEYS.COLLECTION.TEST_KEY, + value: { + [`${ONYXKEYS.COLLECTION.TEST_KEY}1`]: testKeyValues[1], + [`${ONYXKEYS.COLLECTION.TEST_KEY}2`]: testKeyValues[2], + }, + }, + { + onyxMethod: Onyx.METHOD.MERGE, + key: `${ONYXKEYS.COLLECTION.TEST_KEY_2}1`, + value: testKey2Value, + }, + ]); + }); + + await act(async () => waitForPromisesToResolve()); + + // We expect only one re-render after Onyx.update updates are applied + expect(onRender).toHaveBeenCalledTimes(1); + expect(onRender).toHaveBeenLastCalledWith({value1: testKeyValues[1], value2: testKey2Value}); + }); + }); + // This test suite must be the last one to avoid problems when running the other tests here. describe('canEvict', () => { const error = (key: string) => `canEvict can't be used on key '${key}'. This key must explicitly be flagged as safe for removal by adding it to Onyx.init({evictableKeys: []}).`;