Skip to content

Commit

Permalink
Merge pull request #437 from margelo/@chrispader/fix-onyx-update-order
Browse files Browse the repository at this point in the history 
Fix: Onyx.update order
  • Loading branch information
@neil-marcellini
neil-marcellini authored Dec 22, 2023
2 parents 24c551b + b89d046 commit 3d2706f
Show file tree
Hide file tree
Showing 3 changed files with 72 additions and 43 deletions.
80 changes: 38 additions & 42 deletions lib/Onyx.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1021,17 +1021,18 @@ function evictStorageAndRetry(error, onyxMethod, ...args) {
*
* @param {String} key
* @param {*} value
* @param {Boolean} hasChanged
* @param {String} method
* @param {Boolean} hasChanged
* @param {Boolean} wasRemoved
* @returns {Promise}
*/
function broadcastUpdate(key, value, hasChanged, method) {
function broadcastUpdate(key, value, method, hasChanged, wasRemoved = false) {
// Logging properties only since values could be sensitive things we don't want to log
Logger.logInfo(`${method}() called for key: ${key}${_.isObject(value) ? ` properties: ${_.keys(value).join(',')}` : ''}`);

// Update subscribers if the cached value has changed, or when the subscriber specifically requires
// all updates regardless of value changes (indicated by initWithStoredValues set to false).
if (hasChanged) {
if (hasChanged && !wasRemoved) {
cache.set(key, value);
} else {
cache.addToAccessedKeys(key);
Expand All @@ -1053,18 +1054,18 @@ function hasPendingMergeForKey(key) {
* Otherwise removes all nested null values in objects and returns the object
* @param {String} key
* @param {Mixed} value
* @returns {Mixed} `null` if the key got removed completely, otherwise the value without null values
* @returns {Mixed} The value without null values and a boolean "wasRemoved", which indicates if the key got removed completely
*/
function removeNullValues(key, value) {
if (_.isNull(value)) {
remove(key);
return null;
return {value, wasRemoved: true};
}

// We can remove all null values in an object by merging it with itself
// utils.fastMerge recursively goes through the object and removes all null values
// Passing two identical objects as source and target to fastMerge will not change it, but only remove the null values
return utils.removeNestedNullValues(value);
return {value: utils.removeNestedNullValues(value), wasRemoved: false};
}

/**
Expand All @@ -1085,41 +1086,48 @@ function set(key, value) {
return Promise.resolve();
}

const valueWithoutNull = removeNullValues(key, value);

if (valueWithoutNull === null) {
return Promise.resolve();
}
// If the value is null, we remove the key from storage
const {value: valueAfterRemoving, wasRemoved} = removeNullValues(key, value);

if (hasPendingMergeForKey(key)) {
Logger.logAlert(`Onyx.set() called after Onyx.merge() for key: ${key}. It is recommended to use set() or merge() not both.`);
delete mergeQueue[key]
}

const hasChanged = cache.hasValueChanged(key, valueWithoutNull);
const hasChanged = cache.hasValueChanged(key, valueAfterRemoving);

// This approach prioritizes fast UI changes without waiting for data to be stored in device storage.
const updatePromise = broadcastUpdate(key, valueWithoutNull, hasChanged, 'set');
const updatePromise = broadcastUpdate(key, valueAfterRemoving, 'set', hasChanged, wasRemoved);

// If the value has not changed, calling Storage.setItem() would be redundant and a waste of performance, so return early instead.
if (!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 || wasRemoved) {
return updatePromise;
}

return Storage.setItem(key, valueWithoutNull)
.catch((error) => evictStorageAndRetry(error, set, key, valueWithoutNull))
return Storage.setItem(key, valueAfterRemoving)
.catch((error) => evictStorageAndRetry(error, set, key, valueAfterRemoving))
.then(() => updatePromise);
}

/**
* Storage expects array like: [["@MyApp_user", value_1], ["@MyApp_key", value_2]]
* This method transforms an object like {'@MyApp_user': myUserValue, '@MyApp_key': myKeyValue}
* to an array of key-value pairs in the above format
* to an array of key-value pairs in the above format and removes key-value pairs that are being set to null
* @private
* @param {Record} data
* @return {Array} an array of key - value pairs <[key, value]>
*/
function prepareKeyValuePairsForStorage(data) {
return _.map(data, (value, key) => [key, value]);
const keyValuePairs = [];

_.forEach(data, (value, key) => {
const {value: valueAfterRemoving, wasRemoved} = removeNullValues(key, value);

if (wasRemoved) return

keyValuePairs.push([key, valueAfterRemoving])
});

return keyValuePairs
}

/**
Expand All @@ -1142,25 +1150,13 @@ function multiSet(data) {

const keyValuePairs = prepareKeyValuePairsForStorage(data);

const updatePromises = _.map(data, (val, key) => {
const updatePromises = _.map(keyValuePairs, ([key, value]) => {
// Update cache and optimistically inform subscribers on the next tick
cache.set(key, val);
return scheduleSubscriberUpdate(key, val);
cache.set(key, value);
return scheduleSubscriberUpdate(key, value);
});

const keyValuePairsWithoutNull = _.filter(
_.map(keyValuePairs, ([key, value]) => {
const valueWithoutNull = removeNullValues(key, value);

if (valueWithoutNull === null) {
return;
}
return [key, valueWithoutNull];
}),
Boolean,
);

return Storage.multiSet(keyValuePairsWithoutNull)
return Storage.multiSet(keyValuePairs)
.catch((error) => evictStorageAndRetry(error, multiSet, data))
.then(() => Promise.all(updatePromises));
}
Expand Down Expand Up @@ -1236,6 +1232,9 @@ function merge(key, changes) {
mergeQueue[key] = [changes];

mergeQueuePromise[key] = get(key).then((existingValue) => {
// Calls to Onyx.set after a merge will terminate the current merge process and clear the merge queue
if (mergeQueue[key] == null) return

try {
// We first only merge the changes, so we can provide these to the native implementation (SQLite uses only delta changes in "JSON_PATCH" to merge)
// We don't want to remove null values from the "batchedChanges", because SQLite uses them to remove keys from storage natively.
Expand All @@ -1250,10 +1249,7 @@ function merge(key, changes) {
delete mergeQueuePromise[key];

// If the batched changes equal null, we want to remove the key from storage, to reduce storage size
if (_.isNull(batchedChanges)) {
remove(key);
return;
}
const {wasRemoved} = removeNullValues(key, batchedChanges)

// After that we merge the batched changes with the existing value
// We can remove null values from the "modifiedData", because "null" implicates that the user wants to remove a value from storage.
Expand All @@ -1271,10 +1267,10 @@ function merge(key, changes) {
const hasChanged = cache.hasValueChanged(key, modifiedData);

// This approach prioritizes fast UI changes without waiting for data to be stored in device storage.
const updatePromise = broadcastUpdate(key, modifiedData, hasChanged, 'merge');
const updatePromise = broadcastUpdate(key, modifiedData, 'merge', hasChanged, wasRemoved);

// If the value has not changed, calling Storage.setItem() would be redundant and a waste of performance, so return early instead.
if (!hasChanged || isClearing) {
if (!hasChanged || isClearing || wasRemoved) {
return updatePromise;
}

Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
],
"react-native": "native.js",
"main": "native.js",
"browser": "web.js",
"browser": "lib/index.js",
"types": "lib/index.d.ts",
"scripts": {
"lint": "eslint .",
Expand Down
33 changes: 33 additions & 0 deletions tests/unit/onyxTest.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1011,4 +1011,37 @@ describe('Onyx', () => {
expect(testKeyValue).toEqual(2);
});
});

it('should apply updates in order with Onyx.update', () => {
let testKeyValue;

connectionID = Onyx.connect({
key: ONYX_KEYS.TEST_KEY,
initWithStoredValues: false,
callback: (value) => {
testKeyValue = value;
},
});

return Onyx.set(ONYX_KEYS.TEST_KEY, {})
.then(() => {
expect(testKeyValue).toEqual({});
Onyx.update([
{
onyxMethod: 'merge',
key: ONYX_KEYS.TEST_KEY,
value: {test1: 'test1'},
},
{
onyxMethod: 'set',
key: ONYX_KEYS.TEST_KEY,
value: null,
},
]);
return waitForPromisesToResolve();
})
.then(() => {
expect(testKeyValue).toEqual(null);
})
});
});

0 comments on commit 3d2706f

Please sign in to comment.