From 09c529e90f2b6574772598a103a26f781cac780f Mon Sep 17 00:00:00 2001 From: Viktoryia Kliushun Date: Wed, 10 Sep 2025 08:56:04 +0300 Subject: [PATCH] Revert "Update batching to be applied to useOnyx if they come from `Onyx.update`" --- lib/Onyx.ts | 66 ++++------------- lib/OnyxMerge/index.native.ts | 3 +- lib/OnyxMerge/index.ts | 3 +- lib/OnyxMerge/types.ts | 1 - lib/OnyxUtils.ts | 25 +++---- lib/types.ts | 3 - lib/useOnyx.ts | 1 - .../unit/{useOnyxTest.tsx => useOnyxTest.ts} | 73 +------------------ 8 files changed, 28 insertions(+), 147 deletions(-) rename tests/unit/{useOnyxTest.tsx => useOnyxTest.ts} (95%) diff --git a/lib/Onyx.ts b/lib/Onyx.ts index a826bf44c..5b9065c7e 100644 --- a/lib/Onyx.ts +++ b/lib/Onyx.ts @@ -162,13 +162,6 @@ 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)) { @@ -221,7 +214,7 @@ function setInternal(key: TKey, value: OnyxSetInput, 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, isFromUpdate); + const updatePromise = OnyxUtils.broadcastUpdate(key, valueWithoutNestedNullValues, hasChanged); // 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) { @@ -229,7 +222,7 @@ function setInternal(key: TKey, value: OnyxSetInput, } return Storage.setItem(key, valueWithoutNestedNullValues) - .catch((error) => OnyxUtils.evictStorageAndRetry(error, setInternal, key, valueWithoutNestedNullValues, undefined, isFromUpdate)) + .catch((error) => OnyxUtils.evictStorageAndRetry(error, set, key, valueWithoutNestedNullValues)) .then(() => { OnyxUtils.sendActionToDevTools(OnyxUtils.METHOD.SET, key, valueWithoutNestedNullValues); return updatePromise; @@ -244,13 +237,6 @@ function setInternal(key: TKey, value: OnyxSetInput, * @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(); @@ -283,11 +269,11 @@ function multiSetInternal(data: OnyxMultiSetInput, isFromUpdate = false): Promis // Update cache and optimistically inform subscribers on the next tick cache.set(key, value); - return OnyxUtils.scheduleSubscriberUpdate(key, value, prevValue, undefined, isFromUpdate); + return OnyxUtils.scheduleSubscriberUpdate(key, value, prevValue); }); return Storage.multiSet(keyValuePairsToSet) - .catch((error) => OnyxUtils.evictStorageAndRetry(error, multiSetInternal, newData, isFromUpdate)) + .catch((error) => OnyxUtils.evictStorageAndRetry(error, multiSet, newData)) .then(() => { OnyxUtils.sendActionToDevTools(OnyxUtils.METHOD.MULTI_SET, undefined, newData); return Promise.all(updatePromises); @@ -312,13 +298,6 @@ function multiSetInternal(data: OnyxMultiSetInput, isFromUpdate = false): Promis * 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 { @@ -381,7 +360,7 @@ function mergeInternal(key: TKey, changes: OnyxMergeInput< return Promise.resolve(); } - return OnyxMerge.applyMerge(key, existingValue, validChanges, isFromUpdate).then(({mergedValue, updatePromise}) => { + return OnyxMerge.applyMerge(key, existingValue, validChanges).then(({mergedValue, updatePromise}) => { OnyxUtils.sendActionToDevTools(OnyxUtils.METHOD.MERGE, key, changes, mergedValue); return updatePromise; }); @@ -408,14 +387,7 @@ function mergeInternal(key: TKey, changes: OnyxMergeInput< * @param collection Object collection keyed by individual collection member keys and values */ function mergeCollection(collectionKey: TKey, collection: OnyxMergeCollectionInput): Promise { - 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); + return OnyxUtils.mergeCollectionWithPatches(collectionKey, collection); } /** @@ -504,10 +476,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), undefined, false)); + updatePromises.push(OnyxUtils.scheduleSubscriberUpdate(key, value, cache.get(key, false))); }); Object.entries(keyValuesToResetAsCollection).forEach(([key, value]) => { - updatePromises.push(OnyxUtils.scheduleNotifyCollectionSubscribers(key, value, undefined, false)); + updatePromises.push(OnyxUtils.scheduleNotifyCollectionSubscribers(key, value)); }); const defaultKeyValuePairs = Object.entries( @@ -598,7 +570,7 @@ function update(data: OnyxUpdate[]): Promise { collectionKeys.forEach((collectionKey) => enqueueMergeOperation(collectionKey, mergedCollection[collectionKey])); } }, - [OnyxUtils.METHOD.SET_COLLECTION]: (k, v) => promises.push(() => setCollectionInternal(k, v as Collection, true)), + [OnyxUtils.METHOD.SET_COLLECTION]: (k, v) => promises.push(() => setCollection(k, v as Collection)), [OnyxUtils.METHOD.MULTI_SET]: (k, v) => Object.entries(v as Partial).forEach(([entryKey, entryValue]) => enqueueSetOperation(entryKey, entryValue)), [OnyxUtils.METHOD.CLEAR]: () => { clearPromise = clear(); @@ -653,28 +625,27 @@ function update(data: OnyxUpdate[]): Promise { collectionKey, batchedCollectionUpdates.merge as Collection, batchedCollectionUpdates.mergeReplaceNullPatches, - true, ), ); } if (!utils.isEmptyObject(batchedCollectionUpdates.set)) { - promises.push(() => multiSetInternal(batchedCollectionUpdates.set, true)); + promises.push(() => multiSet(batchedCollectionUpdates.set)); } }); Object.entries(updateQueue).forEach(([key, operations]) => { if (operations[0] === null) { const batchedChanges = OnyxUtils.mergeChanges(operations).result; - promises.push(() => setInternal(key, batchedChanges, undefined, true)); + promises.push(() => set(key, batchedChanges)); return; } operations.forEach((operation) => { - promises.push(() => mergeInternal(key, operation, true)); + promises.push(() => merge(key, operation)); }); }); - const snapshotPromises = OnyxUtils.updateSnapshots(data, (key, changes) => mergeInternal(key, changes, true)); + const snapshotPromises = OnyxUtils.updateSnapshots(data, merge); // 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); @@ -696,13 +667,6 @@ 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); @@ -750,10 +714,10 @@ function setCollectionInternal(collectionK keyValuePairs.forEach(([key, value]) => cache.set(key, value)); - const updatePromise = OnyxUtils.scheduleNotifyCollectionSubscribers(collectionKey, mutableCollection, previousCollection, isFromUpdate); + const updatePromise = OnyxUtils.scheduleNotifyCollectionSubscribers(collectionKey, mutableCollection, previousCollection); return Storage.multiSet(keyValuePairs) - .catch((error) => OnyxUtils.evictStorageAndRetry(error, setCollectionInternal, collectionKey, collection, isFromUpdate)) + .catch((error) => OnyxUtils.evictStorageAndRetry(error, setCollection, collectionKey, collection)) .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 03ea36a2a..b796dfde4 100644 --- a/lib/OnyxMerge/index.native.ts +++ b/lib/OnyxMerge/index.native.ts @@ -8,7 +8,6 @@ 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; @@ -27,7 +26,7 @@ const applyMerge: ApplyMerge = , hasChanged, isFromUpdate); + const updatePromise = OnyxUtils.broadcastUpdate(key, mergedValue as OnyxValue, hasChanged); // 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 e34dd1095..2a648499b 100644 --- a/lib/OnyxMerge/index.ts +++ b/lib/OnyxMerge/index.ts @@ -8,7 +8,6 @@ const applyMerge: ApplyMerge = { const {result: mergedValue} = OnyxUtils.mergeChanges(validChanges, existingValue); @@ -19,7 +18,7 @@ const applyMerge: ApplyMerge = , hasChanged, isFromUpdate); + const updatePromise = OnyxUtils.broadcastUpdate(key, mergedValue as OnyxValue, hasChanged); // 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 ab246dbab..c59b7892a 100644 --- a/lib/OnyxMerge/types.ts +++ b/lib/OnyxMerge/types.ts @@ -9,7 +9,6 @@ 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 04b7e2fa6..a288b1a19 100644 --- a/lib/OnyxUtils.ts +++ b/lib/OnyxUtils.ts @@ -611,7 +611,6 @@ 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(). @@ -647,8 +646,7 @@ function keysChanged( // Regular Onyx.connect() subscriber found. if (typeof subscriber.callback === 'function') { - // Check if it's a useOnyx or a regular Onyx.connect() subscriber - if ((subscriber.isUseOnyxSubscriber && !notifyUseOnyxHookSubscribers) || (!subscriber.isUseOnyxSubscriber && !notifyConnectSubscribers)) { + if (!notifyConnectSubscribers) { continue; } @@ -807,7 +805,6 @@ 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) { @@ -849,8 +846,7 @@ function keyChanged( // Subscriber is a regular call to connect() and provided a callback if (typeof subscriber.callback === 'function') { - // Check if it's a useOnyx or a regular Onyx.connect() subscriber - if ((subscriber.isUseOnyxSubscriber && !notifyUseOnyxHookSubscribers) || (!subscriber.isUseOnyxSubscriber && !notifyConnectSubscribers)) { + if (!notifyConnectSubscribers) { continue; } if (lastConnectionCallbackData.has(subscriber.subscriptionID) && lastConnectionCallbackData.get(subscriber.subscriptionID) === value) { @@ -1070,10 +1066,9 @@ 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, !isFromUpdate)); - batchUpdates(() => keyChanged(key, value, previousValue, canUpdateSubscriber, false, true, isFromUpdate)); + 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); } @@ -1086,10 +1081,9 @@ function scheduleNotifyCollectionSubscribers( key: TKey, value: OnyxCollection, previousValue?: OnyxCollection, - isFromUpdate = false, ): Promise { - const promise = Promise.resolve().then(() => keysChanged(key, value, previousValue, true, false, !isFromUpdate)); - batchUpdates(() => keysChanged(key, value, previousValue, false, true, isFromUpdate)); + 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); } @@ -1151,7 +1145,7 @@ function evictStorageAndRetry(key: TKey, value: OnyxValue, hasChanged?: boolean, isFromUpdate = false): Promise { +function broadcastUpdate(key: TKey, value: OnyxValue, hasChanged?: boolean): Promise { const prevValue = cache.get(key, false) as OnyxValue; // Update subscribers if the cached value has changed, or when the subscriber specifically requires @@ -1162,7 +1156,7 @@ function broadcastUpdate(key: TKey, value: OnyxValue cache.addToAccessedKeys(key); } - return scheduleSubscriberUpdate(key, value, prevValue, (subscriber) => hasChanged || subscriber?.initWithStoredValues === false, isFromUpdate).then(() => undefined); + return scheduleSubscriberUpdate(key, value, prevValue, (subscriber) => hasChanged || subscriber?.initWithStoredValues === false).then(() => undefined); } function hasPendingMergeForKey(key: OnyxKey): boolean { @@ -1509,7 +1503,6 @@ 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.'); @@ -1610,7 +1603,7 @@ function mergeCollectionWithPatches( // and update all subscribers const promiseUpdate = previousCollectionPromise.then((previousCollection) => { cache.merge(finalMergedCollection); - return scheduleNotifyCollectionSubscribers(collectionKey, finalMergedCollection, previousCollection, isFromUpdate); + return scheduleNotifyCollectionSubscribers(collectionKey, finalMergedCollection, previousCollection); }); return Promise.all(promises) diff --git a/lib/types.ts b/lib/types.ts index e9b9ae6c5..07b6ddd3c 100644 --- a/lib/types.ts +++ b/lib/types.ts @@ -273,9 +273,6 @@ 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 462a61899..3bf238ead 100644 --- a/lib/useOnyx.ts +++ b/lib/useOnyx.ts @@ -359,7 +359,6 @@ function useOnyx>( initWithStoredValues: options?.initWithStoredValues, waitForCollectionCallback: OnyxUtils.isCollectionKey(key) as true, reuseConnection: options?.reuseConnection, - isUseOnyxSubscriber: true, }); checkEvictableKey(); diff --git a/tests/unit/useOnyxTest.tsx b/tests/unit/useOnyxTest.ts similarity index 95% rename from tests/unit/useOnyxTest.tsx rename to tests/unit/useOnyxTest.ts index 881208f59..afaa6d66f 100644 --- a/tests/unit/useOnyxTest.tsx +++ b/tests/unit/useOnyxTest.ts @@ -1,7 +1,5 @@ -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 {act, renderHook} from '@testing-library/react-native'; +import type {OnyxCollection, OnyxEntry} from '../../lib'; import Onyx, {useOnyx} from '../../lib'; import OnyxCache from '../../lib/OnyxCache'; import StorageMock from '../../lib/storage'; @@ -1155,73 +1153,6 @@ 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: []}).`;