Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Onyx.update order #437

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 38 additions & 42 deletions lib/Onyx.js
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) {
neil-marcellini marked this conversation as resolved.
Show resolved Hide resolved
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};
chrispader marked this conversation as resolved.
Show resolved Hide resolved
}

// 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) {
neil-marcellini marked this conversation as resolved.
Show resolved Hide resolved
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
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
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);
})
});
});
Loading