Skip to content

Commit

Permalink
Merge pull request #558 from margelo/@chrispader/fix-onyx-bump-fixes
Browse files Browse the repository at this point in the history 
Fix: issues in Onyx bump due to removal of null values in cache
  • Loading branch information
@mountiny
mountiny authored Jun 7, 2024
2 parents d5f9c75 + 767ff9c commit 91544e9
Show file tree
Hide file tree
Showing 10 changed files with 352 additions and 210 deletions.
45 changes: 19 additions & 26 deletions lib/Onyx.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import type {
InitOptions,
KeyValueMapping,
Mapping,
NullableKeyValueMapping,
OnyxInputKeyValueMapping,
OnyxCollection,
OnyxKey,
OnyxMergeCollectionInput,
Expand All @@ -22,6 +22,7 @@ import type {
OnyxSetInput,
OnyxUpdate,
OnyxValue,
OnyxInput,
} from './types';
import OnyxUtils from './OnyxUtils';
import logMessages from './logMessages';
Expand Down Expand Up @@ -216,10 +217,14 @@ function set<TKey extends OnyxKey>(key: TKey, value: OnyxSetInput<TKey>): Promis
delete OnyxUtils.getMergeQueue()[key];
}

const existingValue = cache.get(key, false);
// Onyx.set will ignore `undefined` values as inputs, therefore we can return early.
if (value === undefined) {
return Promise.resolve();
}

const existingValue = cache.get(key, false);
// If the existing value as well as the new value are null, we can return early.
if (value === null && existingValue === null) {
if (existingValue === undefined && value === null) {
return Promise.resolve();
}

Expand Down Expand Up @@ -274,33 +279,21 @@ function set<TKey extends OnyxKey>(key: TKey, value: OnyxSetInput<TKey>): Promis
* @param data object keyed by ONYXKEYS and the values to set
*/
function multiSet(data: OnyxMultiSetInput): Promise<void> {
const allKeyValuePairs = OnyxUtils.prepareKeyValuePairsForStorage(data, true);

// When a key is set to null, we need to remove the remove the key from storage using "OnyxUtils.remove".
// Therefore, we filter the key value pairs to exclude null values and remove those keys explicitly.
const removePromises: Array<Promise<void>> = [];
const keyValuePairsToUpdate = allKeyValuePairs.filter(([key, value]) => {
if (value === null) {
removePromises.push(OnyxUtils.remove(key));
return false;
}

return true;
});
const keyValuePairsToSet = OnyxUtils.prepareKeyValuePairsForStorage(data, true);

const updatePromises = keyValuePairsToUpdate.map(([key, value]) => {
const updatePromises = keyValuePairsToSet.map(([key, value]) => {
const prevValue = cache.get(key, false);

// Update cache and optimistically inform subscribers on the next tick
cache.set(key, value);
return OnyxUtils.scheduleSubscriberUpdate(key, value, prevValue);
});

return Storage.multiSet(allKeyValuePairs)
return Storage.multiSet(keyValuePairsToSet)
.catch((error) => OnyxUtils.evictStorageAndRetry(error, multiSet, data))
.then(() => {
OnyxUtils.sendActionToDevTools(OnyxUtils.METHOD.MULTI_SET, undefined, data);
return Promise.all([removePromises, updatePromises]);
return Promise.all(updatePromises);
})
.then(() => undefined);
}
Expand Down Expand Up @@ -354,7 +347,7 @@ function merge<TKey extends OnyxKey>(key: TKey, changes: OnyxMergeInput<TKey>):
Logger.logAlert(logMessages.incompatibleUpdateAlert(key, 'merge', existingValueType, newValueType));
}
return isCompatible;
});
}) as Array<OnyxInput<TKey>>;

if (!validChanges.length) {
return Promise.resolve();
Expand Down Expand Up @@ -435,7 +428,7 @@ function mergeCollection<TKey extends CollectionKeyBase, TMap>(collectionKey: TK
Logger.logInfo('mergeCollection() called with invalid or empty value. Skipping this update.');
return Promise.resolve();
}
const mergedCollection: NullableKeyValueMapping = collection;
const mergedCollection: OnyxInputKeyValueMapping = collection;

// Confirm all the collection keys belong to the same parent
let hasCollectionKeyCheckFailed = false;
Expand Down Expand Up @@ -474,7 +467,7 @@ function mergeCollection<TKey extends CollectionKeyBase, TMap>(collectionKey: TK

const newKeys = keys.filter((key) => !persistedKeys.has(key));

const existingKeyCollection = existingKeys.reduce((obj: NullableKeyValueMapping, key) => {
const existingKeyCollection = existingKeys.reduce((obj: OnyxInputKeyValueMapping, key) => {
const {isCompatible, existingValueType, newValueType} = utils.checkCompatibilityWithExistingValue(mergedCollection[key], cachedCollectionForExistingKeys[key]);
if (!isCompatible) {
Logger.logAlert(logMessages.incompatibleUpdateAlert(key, 'mergeCollection', existingValueType, newValueType));
Expand All @@ -483,13 +476,13 @@ function mergeCollection<TKey extends CollectionKeyBase, TMap>(collectionKey: TK
// eslint-disable-next-line no-param-reassign
obj[key] = mergedCollection[key];
return obj;
}, {});
}, {}) as Record<OnyxKey, OnyxInput<TKey>>;

const newCollection = newKeys.reduce((obj: NullableKeyValueMapping, key) => {
const newCollection = newKeys.reduce((obj: OnyxInputKeyValueMapping, key) => {
// eslint-disable-next-line no-param-reassign
obj[key] = mergedCollection[key];
return obj;
}, {});
}, {}) as Record<OnyxKey, OnyxInput<TKey>>;

// When (multi-)merging the values with the existing values in storage,
// we don't want to remove nested null values from the data that we pass to the storage layer,
Expand Down Expand Up @@ -582,7 +575,7 @@ function clear(keysToPreserve: OnyxKey[] = []): Promise<void> {
// since collection key subscribers need to be updated differently
if (!isKeyToPreserve) {
const oldValue = cache.get(key);
const newValue = defaultKeyStates[key] ?? null;
const newValue = defaultKeyStates[key] ?? undefined;
if (newValue !== oldValue) {
cache.set(key, newValue);
const collectionKey = key.substring(0, key.indexOf('_') + 1);
Expand Down
8 changes: 7 additions & 1 deletion lib/OnyxCache.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ class OnyxCache {
'hasCacheForKey',
'addKey',
'addNullishStorageKey',
'hasNullishStorageKey',
'clearNullishStorageKeys',
'set',
'drop',
Expand Down Expand Up @@ -89,14 +90,19 @@ class OnyxCache {
this.nullishStorageKeys.add(key);
}

/** Used to set keys that are null/undefined in storage without adding null to the storage map */
hasNullishStorageKey(key: OnyxKey): boolean {
return this.nullishStorageKeys.has(key);
}

/** Used to clear keys that are null/undefined in cache */
clearNullishStorageKeys(): void {
this.nullishStorageKeys = new Set();
}

/** Check whether cache has data for the given key */
hasCacheForKey(key: OnyxKey): boolean {
return this.storageMap[key] !== undefined || this.nullishStorageKeys.has(key);
return this.storageMap[key] !== undefined || this.hasNullishStorageKey(key);
}

/**
Expand Down
Loading

0 comments on commit 91544e9

Please sign in to comment.