Fix: nullish keys not removed/merged in IDB-Keyval (web)

lib/DevTools.js

@@ -1,6 +1,12 @@
-import {connectViaExtension} from 'remotedev';
 import _ from 'underscore';
 
+const {connectViaExtension} = process.env.NODE_ENV === 'production' ? require('remotedev') : {
+    connectViaExtension: () => ({
+        init: () => {},
+        send: () => {},
+    }),
+};
+
 class DevTools {
     /**
      * @callback onStateChange
@@ -42,7 +48,7 @@ class DevTools {
      */
     clearState(keysToBeRemoved = []) {
         const pairs = _.map(keysToBeRemoved, key => [key, undefined]);
-        this.registerAction('CLEAR', undefined, _.object((pairs)));
+        this.registerAction('CLEAR', undefined, _.object(pairs));
     }
 }
 

lib/Onyx.js

@@ -8,6 +8,7 @@ import createDeferredTask from './createDeferredTask';
 import fastMerge from './fastMerge';
 import * as PerformanceUtils from './metrics/PerformanceUtils';
 import Storage from './storage';
+import Utils from './utils';
 import DevTools from './DevTools';
 
 // Method constants
@@ -935,24 +936,6 @@ function hasPendingMergeForKey(key) {
     return Boolean(mergeQueue[key]);
 }
 
-/**
- * We generally want to remove top-level nullish values from objects written to disk and cache, because it decreases the amount of data stored in memory and on disk.
- * On native, when merging an existing value with new changes, SQLite will use  JSON_PATCH, which removes top-level nullish values.
- * To be consistent with the behaviour for merge, we'll also want to remove nullish values for "set" operations.
- * On web, IndexedDB will keep the top-level keys along with a null value and this uses up storage and memory.
- * This method will ensure that keys for null values are removed before an object is written to disk and cache so that all platforms are storing the data in the same efficient way.
- * @private
- * @param {*} value
- * @returns {*}
- */
-function removeNullObjectValues(value) {
-    if (_.isArray(value) || !_.isObject(value)) {
-        return value;
-    }
-
-    return _.omit(value, objectValue => _.isNull(objectValue));
-}
-
 /**
  * Write a value to our store with the given key
  *
@@ -970,7 +953,7 @@ function set(key, value) {
         Logger.logAlert(`Onyx.set() called after Onyx.merge() for key: ${key}. It is recommended to use set() or merge() not both.`);
     }
 
-    const valueWithNullRemoved = removeNullObjectValues(value);
+    const valueWithNullRemoved = Utils.removeNullObjectValues(value);
 
     const hasChanged = cache.hasValueChanged(key, valueWithNullRemoved);
 
@@ -1097,14 +1080,14 @@ function merge(key, changes) {
                 delete mergeQueue[key];
 
                 // After that we merge the batched changes with the existing value
-                const modifiedData = removeNullObjectValues(applyMerge(existingValue, [batchedChanges]));
+                const modifiedData = Utils.removeNullObjectValues(applyMerge(existingValue, [batchedChanges]));
 
                 // On native platforms we use SQLite which utilises JSON_PATCH to merge changes.
                 // JSON_PATCH generally removes top-level nullish values from the stored object.
                 // When there is no existing value though, SQLite will just insert the changes as a new value and thus the top-level nullish values won't be removed.
                 // Therefore we need to remove nullish values from the `batchedChanges` which are sent to the SQLite, if no existing value is present.
                 if (!existingValue) {
-                    batchedChanges = removeNullObjectValues(batchedChanges);
+                    batchedChanges = Utils.removeNullObjectValues(batchedChanges);
                 }
 
                 const hasChanged = cache.hasValueChanged(key, modifiedData);

lib/fastMerge.js

@@ -56,8 +56,8 @@ function mergeObject(target, source) {
 function fastMerge(target, source) {
     // lodash adds a small overhead so we don't use it here
     // eslint-disable-next-line rulesdir/prefer-underscore-method
-    const array = Array.isArray(source);
-    if (array) {
+    const isArray = Array.isArray(source);
+    if (isArray || source == null) {
         return source;
     }
     return mergeObject(target, source);

lib/storage/providers/IDBKeyVal.js

@@ -12,6 +12,7 @@ import {
 } from 'idb-keyval';
 import _ from 'underscore';
 import fastMerge from '../../fastMerge';
+import Utils from '../../utils';
 
 // We don't want to initialize the store while the JS bundle loads as idb-keyval will try to use global.indexedDB
 // which might not be available in certain environments that load the bundle (e.g. electron main process).
@@ -54,9 +55,17 @@ const provider = {
 
         return getValues.then((values) => {
             const upsertMany = _.map(pairs, ([key, value], index) => {
+                // If the value is null, we want to delete the key from storage,
+                // to be consistent with the native implementation with SQLite.
+                // SQLite by default removes keys from storage that are set to nullish values
+                if (_.isUndefined(value) || _.isNull(value)) {
+                    return promisifyRequest(store.delete(key));
+                }
+
                 const prev = values[index];
                 const newValue = _.isObject(prev) ? fastMerge(prev, value) : value;
-                return promisifyRequest(store.put(newValue, key));
+
+                return promisifyRequest(store.put(Utils.removeNullObjectValues(newValue), key));
             });
             return Promise.all(upsertMany);
         });

lib/utils.js

@@ -0,0 +1,21 @@
+import _ from 'underscore';
+
+/**
+ * We generally want to remove top-level nullish values from objects written to disk and cache, because it decreases the amount of data stored in memory and on disk.
+ * On native, when merging an existing value with new changes, SQLite will use  JSON_PATCH, which removes top-level nullish values.
+ * To be consistent with the behaviour for merge, we'll also want to remove nullish values for "set" operations.
+ * On web, IndexedDB will keep the top-level keys along with a null value and this uses up storage and memory.
+ * This method will ensure that keys for null values are removed before an object is written to disk and cache so that all platforms are storing the data in the same efficient way.
+ * @private
+ * @param {*} value
+ * @returns {*}
+ */
+function removeNullObjectValues(value) {
+    if (_.isArray(value) || !_.isObject(value)) {
+        return value;
+    }
+
+    return _.omit(value, objectValue => _.isNull(objectValue));
+}
+
+export default {removeNullObjectValues};

package.json

@@ -1,6 +1,6 @@
 {
   "name": "react-native-onyx",
-  "version": "1.0.74",
+  "version": "1.0.75",
   "author": "Expensify, Inc.",
   "homepage": "https://expensify.com",
   "description": "State management for React Native",