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: issues in Onyx bump due to removal of null values in cache #558

Merged
merged 24 commits into from
Jun 7, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
4e1cb68
fix: check if kez exists rather than undefined
chrispader Jun 4, 2024
138eca1
fix: only add undefined property to collection returned by "getCached…
chrispader Jun 4, 2024
da593f1
fix: improve types and add undefined handling
chrispader Jun 4, 2024
b680541
fix: onyx connect callback might return undefined
chrispader Jun 4, 2024
540480f
fix: input types
chrispader Jun 4, 2024
b0d2505
fix: export types
chrispader Jun 4, 2024
d44df56
fix: input type
chrispader Jun 4, 2024
cdf0c78
fix: check for equality
chrispader Jun 4, 2024
c8dd375
fix: removing items won't return undefined
chrispader Jun 5, 2024
1095fa4
check for undefined values in fastMerge
chrispader Jun 5, 2024
da1bd7e
add tests regarding undefined handling
chrispader Jun 5, 2024
ae64bfc
add test for mergeCollection
chrispader Jun 5, 2024
8292acb
fix: callbacks always return undefined instead of null
chrispader Jun 5, 2024
81291e7
fix: tests after null -> undefined changes
chrispader Jun 5, 2024
790e2b7
remove custom UseOnyxValue type
chrispader Jun 5, 2024
b386809
update comments
chrispader Jun 5, 2024
b1315b3
add a OnyxCollectionInput
chrispader Jun 5, 2024
08c2bfb
fix: OnyxInput type
chrispader Jun 5, 2024
4e893aa
update mergeCollection input type
chrispader Jun 5, 2024
4b1dccc
remove unused ts-expect-error pragmas
chrispader Jun 5, 2024
7b72e60
rename type
chrispader Jun 5, 2024
5fc63a9
Revert "update mergeCollection input type"
chrispader Jun 6, 2024
2cabca8
add back @ts-expect-error
chrispader Jun 6, 2024
767ff9c
Merge branch 'main' into @chrispader/fix-onyx-bump-fixes
chrispader Jun 6, 2024
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
45 changes: 19 additions & 26 deletions lib/Onyx.ts
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
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
Loading