Fix concurrent writes in the update operation

.gitignore

@@ -11,6 +11,7 @@ dist/
 
 # IDEs
 .idea
+.vscode
 
 # Decrypted private key we do not want to commit
 .github/OSBotify-private-key.asc

lib/Onyx.js

@@ -1392,6 +1392,42 @@ function mergeCollection(collectionKey, collection) {
         });
 }
 
+/**
+ * Internal recursive function to execute the functions in the correct order
+ *
+ * @param {Array} data An array of objects with shape {onyxMethod: oneOf('set', 'merge', 'mergeCollection', 'multiSet', 'clear'), key: string, value: *}
+ * @returns {Promise<Void>}
+ */
+function innerUpdate(data) {
+    if (data.length === 0) {
+        return Promise.resolve();
+    }
+
+    const {onyxMethod, key, value} = data.shift();
+    let promise = Promise.resolve();
+    switch (onyxMethod) {
+        case METHOD.SET:
+            promise = set(key, value);
+            break;
+        case METHOD.MERGE:
+            promise = merge(key, value);
+            break;
+        case METHOD.MERGE_COLLECTION:
+            promise = mergeCollection(key, value);
+            break;
+        case METHOD.MULTI_SET:
+            promise = multiSet(value);
+            break;
+        case METHOD.CLEAR:
+            promise = clear();
+            break;
+        default:
+            break;
+    }
+
+    return promise.then(() => innerUpdate(data));
+}
+
 /**
  * Insert API responses and lifecycle data into Onyx
  *
@@ -1400,8 +1436,9 @@ function mergeCollection(collectionKey, collection) {
  */
 function update(data) {
     // First, validate the Onyx object is in the format we expect
+    const validMethods = [METHOD.CLEAR, METHOD.SET, METHOD.MERGE, METHOD.MERGE_COLLECTION, METHOD.MULTI_SET];
     _.each(data, ({onyxMethod, key, value}) => {
-        if (!_.contains([METHOD.CLEAR, METHOD.SET, METHOD.MERGE, METHOD.MERGE_COLLECTION, METHOD.MULTI_SET], onyxMethod)) {
+        if (!_.contains(validMethods, onyxMethod)) {
             throw new Error(`Invalid onyxMethod ${onyxMethod} in Onyx update.`);
         }
         if (onyxMethod === METHOD.MULTI_SET) {
@@ -1414,32 +1451,10 @@ function update(data) {
         }
     });
 
-    const promises = [];
-    let clearPromise = Promise.resolve();
-
-    _.each(data, ({onyxMethod, key, value}) => {
-        switch (onyxMethod) {
-            case METHOD.SET:
-                promises.push(() => set(key, value));
-                break;
-            case METHOD.MERGE:
-                promises.push(() => merge(key, value));
-                break;
-            case METHOD.MERGE_COLLECTION:
-                promises.push(() => mergeCollection(key, value));
-                break;
-            case METHOD.MULTI_SET:
-                promises.push(() => multiSet(value));
-                break;
-            case METHOD.CLEAR:
-                clearPromise = clear();
-                break;
-            default:
-                break;
-        }
-    });
+    // Put clear operation on top
+    data.sort(a => (a.onyxMethod === METHOD.CLEAR ? -1 : 1));
 
-    return clearPromise.then(() => Promise.all(_.map(promises, p => p())));
+    return innerUpdate(data);
 }
 
 /**

tests/unit/onyxTest.js

@@ -1,6 +1,7 @@
 import _ from 'underscore';
 import Onyx from '../../lib';
 import waitForPromisesToResolve from '../utils/waitForPromisesToResolve';
+import Storage from '../../lib/storage';
 
 const ONYX_KEYS = {
     TEST_KEY: 'test',
@@ -899,9 +900,9 @@ describe('Onyx', () => {
             {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);
+                expect(collectionCallback).toHaveBeenNthCalledWith(2, {[itemKey]: {a: 'a'}});
+                expect(testCallback).toHaveBeenNthCalledWith(2, 'taco', ONYX_KEYS.TEST_KEY);
+                expect(otherTestCallback).toHaveBeenNthCalledWith(2, 'pizza', ONYX_KEYS.OTHER_TEST);
                 Onyx.disconnect(connectionIDs);
             });
     });
@@ -960,4 +961,68 @@ describe('Onyx', () => {
                 });
             });
     });
+
+    it('should persist data in the correct order', () => {
+        const key = `${ONYX_KEYS.TEST_KEY}123`;
+        const callback = jest.fn();
+        connectionID = Onyx.connect({
+            key,
+            initWithStoredValues: false,
+            callback,
+        });
+
+        return waitForPromisesToResolve()
+            .then(() => Onyx.update([
+                {
+                    onyxMethod: 'set',
+                    key,
+                    value: 'one',
+                },
+                {
+                    onyxMethod: 'merge',
+                    key,
+                    value: 'two',
+                },
+                {
+                    onyxMethod: 'set',
+                    key,
+                    value: 'three',
+                },
+            ]))
+            .then(() => Storage.getItem(key))
+            .then((value) => {
+                expect(callback).toHaveBeenNthCalledWith(1, 'one', key);
+                expect(callback).toHaveBeenNthCalledWith(2, 'two', key);
+                expect(callback).toHaveBeenNthCalledWith(3, 'three', key);
+                expect(value).toBe('three');
+            });
+    });
+
+    it('should persist data in the correct order', () => {
+        const key = `${ONYX_KEYS.TEST_KEY}123`;
+        const callback = jest.fn();
+        connectionID = Onyx.connect({
+            key,
+            initWithStoredValues: false,
+            callback,
+        });
+
+        return waitForPromisesToResolve()
+            .then(() => Onyx.update([
+                {
+                    onyxMethod: 'set',
+                    key,
+                    value: 'one',
+                },
+                {
+                    onyxMethod: 'clear',
+                },
+
+            ]))
+            .then(() => Storage.getItem(key))
+            .then((value) => {
+                expect(callback).toHaveBeenNthCalledWith(1, 'one', key);
+                expect(value).toBe('one');
+            });
+    });
 });