Merge pull request #689 from callstack-internal/VickyStash/poc/remove-batching

Remove batching mechanism

Author: mountiny

API-INTERNAL.md

@@ -26,12 +26,6 @@
 <dt><a href="#initStoreValues">initStoreValues(keys, initialKeyStates, evictableKeys)</a></dt>
 <dd><p>Sets the initial values for the Onyx store</p>
 </dd>
-<dt><a href="#maybeFlushBatchUpdates">maybeFlushBatchUpdates()</a></dt>
-<dd><p>We are batching together onyx updates. This helps with use cases where we schedule onyx updates after each other.
-This happens for example in the Onyx.update function, where we process API responses that might contain a lot of
-update operations. Instead of calling the subscribers for each update operation, we batch them together which will
-cause react to schedule the updates at once instead of after each other. This is mainly a performance optimization.</p>
-</dd>
 <dt><a href="#reduceCollectionWithSelector">reduceCollectionWithSelector()</a></dt>
 <dd><p>Takes a collection of items (eg. {testKey_1:{a:&#39;a&#39;}, testKey_2:{b:&#39;b&#39;}})
 and runs it through a reducer function to return a subset of the data according to a selector.
@@ -61,6 +55,9 @@ to the values for those keys (correctly typed) such as <code>[OnyxCollection&lt;
 <dd><p>Checks to see if the subscriber&#39;s supplied key
 is associated with a collection of keys.</p>
 </dd>
+<dt><a href="#isCollectionMember">isCollectionMember(key)</a> ⇒</dt>
+<dd><p>Checks if a given key is a collection member key (not just a collection key).</p>
+</dd>
 <dt><a href="#splitCollectionMemberKey">splitCollectionMemberKey(key, collectionKey)</a> ⇒</dt>
 <dd><p>Splits a collection member key into the collection key part and the ID part.</p>
 </dd>
@@ -98,11 +95,14 @@ run out of storage the least recently accessed key can be removed.</p>
 <dt><a href="#getCollectionDataAndSendAsObject">getCollectionDataAndSendAsObject()</a></dt>
 <dd><p>Gets the data for a given an array of matching keys, combines them into an object, and sends the result back to the subscriber.</p>
 </dd>
+<dt><a href="#prepareSubscriberUpdate">prepareSubscriberUpdate(callback)</a></dt>
+<dd><p>Delays promise resolution until the next macrotask to prevent race condition if the key subscription is in progress.</p>
+</dd>
 <dt><a href="#scheduleSubscriberUpdate">scheduleSubscriberUpdate()</a></dt>
 <dd><p>Schedules an update that will be appended to the macro task queue (so it doesn&#39;t update the subscribers immediately).</p>
 </dd>
 <dt><a href="#scheduleNotifyCollectionSubscribers">scheduleNotifyCollectionSubscribers()</a></dt>
-<dd><p>This method is similar to notifySubscribersOnNextTick but it is built for working specifically with collections
+<dd><p>This method is similar to scheduleSubscriberUpdate but it is built for working specifically with collections
 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.</p>
 </dd>
@@ -230,15 +230,6 @@ Sets the initial values for the Onyx store
 | initialKeyStates | initial data to set when `init()` and `clear()` are called |
 | evictableKeys | This is an array of keys (individual or collection patterns) that when provided to Onyx are flagged as "safe" for removal. |
 
-<a name="maybeFlushBatchUpdates"></a>
-
-## maybeFlushBatchUpdates()
-We are batching together onyx updates. This helps with use cases where we schedule onyx updates after each other.
-This happens for example in the Onyx.update function, where we process API responses that might contain a lot of
-update operations. Instead of calling the subscribers for each update operation, we batch them together which will
-cause react to schedule the updates at once instead of after each other. This is mainly a performance optimization.
-
-**Kind**: global function  
 <a name="reduceCollectionWithSelector"></a>
 
 ## reduceCollectionWithSelector()
@@ -304,6 +295,18 @@ Checks to see if the subscriber's supplied key
 is associated with a collection of keys.
 
 **Kind**: global function  
+<a name="isCollectionMember"></a>
+
+## isCollectionMember(key) ⇒
+Checks if a given key is a collection member key (not just a collection key).
+
+**Kind**: global function  
+**Returns**: true if the key is a collection member, false otherwise  
+
+| Param | Description |
+| --- | --- |
+| key | The key to check |
+
 <a name="splitCollectionMemberKey"></a>
 
 ## splitCollectionMemberKey(key, collectionKey) ⇒
@@ -402,6 +405,17 @@ run out of storage the least recently accessed key can be removed.
 Gets the data for a given an array of matching keys, combines them into an object, and sends the result back to the subscriber.
 
 **Kind**: global function  
+<a name="prepareSubscriberUpdate"></a>
+
+## prepareSubscriberUpdate(callback)
+Delays promise resolution until the next macrotask to prevent race condition if the key subscription is in progress.
+
+**Kind**: global function  
+
+| Param | Description |
+| --- | --- |
+| callback | The keyChanged/keysChanged callback |
+
 <a name="scheduleSubscriberUpdate"></a>
 
 ## scheduleSubscriberUpdate()
@@ -415,7 +429,7 @@ scheduleSubscriberUpdate(key, value, subscriber => subscriber.initWithStoredValu
 <a name="scheduleNotifyCollectionSubscribers"></a>
 
 ## scheduleNotifyCollectionSubscribers()
-This method is similar to notifySubscribersOnNextTick but it is built for working specifically with collections
+This method is similar to scheduleSubscriberUpdate but it is built for working specifically with collections
 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.
 

API.md

@@ -177,7 +177,7 @@ applied in the order they were called. Note: `Onyx.set()` calls do not work this
 **Example**  
 ```js
 Onyx.merge(ONYXKEYS.EMPLOYEE_LIST, ['Joe']); // -> ['Joe']
-Onyx.merge(ONYXKEYS.EMPLOYEE_LIST, ['Jack']); // -> ['Jack']
+Onyx.merge(ONYXKEYS.EMPLOYEE_LIST, ['Jack']); // -> ['Joe', 'Jack']
 Onyx.merge(ONYXKEYS.POLICY, {id: 1}); // -> {id: 1}
 Onyx.merge(ONYXKEYS.POLICY, {name: 'My Workspace'}); // -> {id: 1, name: 'My Workspace'}
 ```

jestSetup.js

@@ -9,6 +9,3 @@ jest.mock('react-native-nitro-sqlite', () => ({
 }));
 
 jest.useRealTimers();
-
-const unstable_batchedUpdates_jest = require('react-test-renderer').unstable_batchedUpdates;
-require('./lib/batch.native').default = unstable_batchedUpdates_jest;

lib/Onyx.ts

@@ -62,7 +62,7 @@ function init({
             // Setting isProcessingCollectionUpdate=true prevents triggering collection callbacks for each individual update
             const isKeyCollectionMember = OnyxUtils.isCollectionMember(key);
 
-            OnyxUtils.keyChanged(key, value as OnyxValue<typeof key>, undefined, true, isKeyCollectionMember);
+            OnyxUtils.keyChanged(key, value as OnyxValue<typeof key>, undefined, isKeyCollectionMember);
         });
     }
 

lib/OnyxUtils.ts

@@ -7,7 +7,6 @@ import * as Logger from './Logger';
 import type Onyx from './Onyx';
 import cache, {TASK} from './OnyxCache';
 import * as Str from './Str';
-import unstable_batchedUpdates from './batch';
 import Storage from './storage';
 import type {
     CollectionKey,
@@ -76,6 +75,9 @@ type OnyxMethod = ValueOf<typeof METHOD>;
 let mergeQueue: Record<OnyxKey, Array<OnyxValue<OnyxKey>>> = {};
 let mergeQueuePromise: Record<OnyxKey, Promise<void>> = {};
 
+// Used to schedule subscriber update to the macro task queue
+let nextMacrotaskPromise: Promise<void> | null = null;
+
 // Holds a mapping of all the React components that want their state subscribed to a store key
 let callbackToStateMapping: Record<string, CallbackToStateMapping<OnyxKey>> = {};
 
@@ -88,9 +90,6 @@ let onyxKeyToSubscriptionIDs = new Map();
 // Optional user-provided key value states set when Onyx initializes or clears
 let defaultKeyStates: Record<OnyxKey, OnyxValue<OnyxKey>> = {};
 
-let batchUpdatesPromise: Promise<void> | null = null;
-let batchUpdatesQueue: Array<() => void> = [];
-
 // Used for comparison with a new update to avoid invoking the Onyx.connect callback with the same data.
 let lastConnectionCallbackData = new Map<number, OnyxValue<OnyxKey>>();
 
@@ -212,43 +211,6 @@ function sendActionToDevTools(
     DevTools.registerAction(utils.formatActionName(method, key), value, key ? {[key]: mergedValue || value} : (value as OnyxCollection<KeyValueMapping[OnyxKey]>));
 }
 
-/**
- * We are batching together onyx updates. This helps with use cases where we schedule onyx updates after each other.
- * This happens for example in the Onyx.update function, where we process API responses that might contain a lot of
- * update operations. Instead of calling the subscribers for each update operation, we batch them together which will
- * cause react to schedule the updates at once instead of after each other. This is mainly a performance optimization.
- */
-function maybeFlushBatchUpdates(): Promise<void> {
-    if (batchUpdatesPromise) {
-        return batchUpdatesPromise;
-    }
-
-    batchUpdatesPromise = new Promise((resolve) => {
-        /* We use (setTimeout, 0) here which should be called once native module calls are flushed (usually at the end of the frame)
-         * We may investigate if (setTimeout, 1) (which in React Native is equal to requestAnimationFrame) works even better
-         * then the batch will be flushed on next frame.
-         */
-        setTimeout(() => {
-            const updatesCopy = batchUpdatesQueue;
-            batchUpdatesQueue = [];
-            batchUpdatesPromise = null;
-            unstable_batchedUpdates(() => {
-                for (const applyUpdates of updatesCopy) {
-                    applyUpdates();
-                }
-            });
-
-            resolve();
-        }, 0);
-    });
-    return batchUpdatesPromise;
-}
-
-function batchUpdates(updates: () => void): Promise<void> {
-    batchUpdatesQueue.push(updates);
-    return maybeFlushBatchUpdates();
-}
-
 /**
  * Takes a collection of items (eg. {testKey_1:{a:'a'}, testKey_2:{b:'b'}})
  * and runs it through a reducer function to return a subset of the data according to a selector.
@@ -634,7 +596,6 @@ function keysChanged<TKey extends CollectionKeyBase>(
     collectionKey: TKey,
     partialCollection: OnyxCollection<KeyValueMapping[TKey]>,
     partialPreviousCollection: OnyxCollection<KeyValueMapping[TKey]> | undefined,
-    notifyConnectSubscribers = true,
 ): void {
     // We prepare the "cached collection" which is the entire collection + the new partial data that
     // was merged in via mergeCollection().
@@ -670,10 +631,6 @@ function keysChanged<TKey extends CollectionKeyBase>(
 
         // Regular Onyx.connect() subscriber found.
         if (typeof subscriber.callback === 'function') {
-            if (!notifyConnectSubscribers) {
-                continue;
-            }
-
             // If they are subscribed to the collection key and using waitForCollectionCallback then we'll
             // send the whole cached collection.
             if (isSubscribedToCollectionKey) {
@@ -723,7 +680,6 @@ function keyChanged<TKey extends OnyxKey>(
     key: TKey,
     value: OnyxValue<TKey>,
     canUpdateSubscriber: (subscriber?: CallbackToStateMapping<OnyxKey>) => boolean = () => true,
-    notifyConnectSubscribers = true,
     isProcessingCollectionUpdate = false,
 ): void {
     // Add or remove this key from the recentlyAccessedKeys lists
@@ -765,9 +721,6 @@ function keyChanged<TKey extends OnyxKey>(
 
         // Subscriber is a regular call to connect() and provided a callback
         if (typeof subscriber.callback === 'function') {
-            if (!notifyConnectSubscribers) {
-                continue;
-            }
             if (lastConnectionCallbackData.has(subscriber.subscriptionID) && lastConnectionCallbackData.get(subscriber.subscriptionID) === value) {
                 continue;
             }
@@ -850,6 +803,23 @@ function getCollectionDataAndSendAsObject<TKey extends OnyxKey>(matchingKeys: Co
     });
 }
 
+/**
+ * Delays promise resolution until the next macrotask to prevent race condition if the key subscription is in progress.
+ *
+ * @param callback The keyChanged/keysChanged callback
+ * */
+function prepareSubscriberUpdate(callback: () => void): Promise<void> {
+    if (!nextMacrotaskPromise) {
+        nextMacrotaskPromise = new Promise<void>((resolve) => {
+            setTimeout(() => {
+                nextMacrotaskPromise = null;
+                resolve();
+            }, 0);
+        });
+    }
+    return Promise.all([nextMacrotaskPromise, Promise.resolve().then(callback)]).then();
+}
+
 /**
  * Schedules an update that will be appended to the macro task queue (so it doesn't update the subscribers immediately).
  *
@@ -862,13 +832,11 @@ function scheduleSubscriberUpdate<TKey extends OnyxKey>(
     canUpdateSubscriber: (subscriber?: CallbackToStateMapping<OnyxKey>) => boolean = () => true,
     isProcessingCollectionUpdate = false,
 ): Promise<void> {
-    const promise = Promise.resolve().then(() => keyChanged(key, value, canUpdateSubscriber, true, isProcessingCollectionUpdate));
-    batchUpdates(() => keyChanged(key, value, canUpdateSubscriber, false, isProcessingCollectionUpdate));
-    return Promise.all([maybeFlushBatchUpdates(), promise]).then(() => undefined);
+    return prepareSubscriberUpdate(() => keyChanged(key, value, canUpdateSubscriber, isProcessingCollectionUpdate));
 }
 
 /**
- * This method is similar to notifySubscribersOnNextTick but it is built for working specifically with collections
+ * This method is similar to scheduleSubscriberUpdate but it is built for working specifically with collections
  * 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.
  */
@@ -877,9 +845,7 @@ function scheduleNotifyCollectionSubscribers<TKey extends OnyxKey>(
     value: OnyxCollection<KeyValueMapping[TKey]>,
     previousValue?: OnyxCollection<KeyValueMapping[TKey]>,
 ): Promise<void> {
-    const promise = Promise.resolve().then(() => keysChanged(key, value, previousValue, true));
-    batchUpdates(() => keysChanged(key, value, previousValue, false));
-    return Promise.all([maybeFlushBatchUpdates(), promise]).then(() => undefined);
+    return prepareSubscriberUpdate(() => keysChanged(key, value, previousValue));
 }
 
 /**
@@ -1692,7 +1658,6 @@ function clearOnyxUtilsInternals() {
     mergeQueuePromise = {};
     callbackToStateMapping = {};
     onyxKeyToSubscriptionIDs = new Map();
-    batchUpdatesQueue = [];
     lastConnectionCallbackData = new Map();
 }
 
@@ -1704,8 +1669,6 @@ const OnyxUtils = {
     getDeferredInitTask,
     initStoreValues,
     sendActionToDevTools,
-    maybeFlushBatchUpdates,
-    batchUpdates,
     get,
     getAllKeys,
     getCollectionKeys,
@@ -1763,10 +1726,6 @@ GlobalSettings.addGlobalSettingsChangeListener(({enablePerformanceMetrics}) => {
 
     // @ts-expect-error Reassign
     initStoreValues = decorateWithMetrics(initStoreValues, 'OnyxUtils.initStoreValues');
-    // @ts-expect-error Reassign
-    maybeFlushBatchUpdates = decorateWithMetrics(maybeFlushBatchUpdates, 'OnyxUtils.maybeFlushBatchUpdates');
-    // @ts-expect-error Reassign
-    batchUpdates = decorateWithMetrics(batchUpdates, 'OnyxUtils.batchUpdates');
     // @ts-expect-error Complex type signature
     get = decorateWithMetrics(get, 'OnyxUtils.get');
     // @ts-expect-error Reassign