-
Notifications
You must be signed in to change notification settings - Fork 95
Implement RAM-only key support to replace initWithStoredValues #725
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
Changes from all commits
aeb9457
d89ec4f
d91820a
ab3dd88
d4e975e
4889a47
17ecd78
3ff81f7
df07112
3296a6f
30fd1f3
ddfc694
a492c41
aa5f13b
091bf1b
3b25980
61b21fb
2edcd51
2824d57
c07c7a2
eea4af1
ab32044
f62fd74
b69c214
2b78c25
0c09ac1
0f12047
1fde270
bead3d3
0848187
94ece5a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
|
mountiny marked this conversation as resolved.
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -470,6 +470,35 @@ function isCollectionMember(key: OnyxKey): boolean { | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * Checks if a given key is a RAM-only key, RAM-only collection key, or a RAM-only collection member | ||
| * | ||
| * For example: | ||
| * | ||
| * For the following Onyx setup | ||
| * | ||
| * ramOnlyKeys: ["ramOnlyKey", "ramOnlyCollection_"] | ||
| * | ||
| * - `isRamOnlyKey("ramOnlyKey")` would return true | ||
| * - `isRamOnlyKey("ramOnlyCollection_")` would return true | ||
| * - `isRamOnlyKey("ramOnlyCollection_1")` would return true | ||
| * - `isRamOnlyKey("someOtherKey")` would return false | ||
| * | ||
| * @param key - The key to check | ||
| * @returns true if key is a RAM-only key, RAM-only collection key, or a RAM-only collection member | ||
| */ | ||
| function isRamOnlyKey(key: OnyxKey): boolean { | ||
| try { | ||
| const collectionKey = getCollectionKey(key); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not a fan of using Problems with the current pattern:
Its proposed solution is to change the signature of
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like this suggestion any concerns with that? @JKobrynski @fabioh8010 ?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I dont see immediate concerns and I agree with the comment, but I would make it return
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed. Do you think we could do that in a follow-up issue? That change would also affect some existing logic so it might be safer to do it separately.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should do it in a separate PR. I agree with the change and I have commented about this in past PRs as well.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @JKobrynski Mind creating an issue for this and link it here?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I created an issue to remove the |
||
| // If collectionKey exists for a given key, check if it's a RAM-only key | ||
| return cache.isRamOnlyKey(collectionKey); | ||
| } catch { | ||
|
mountiny marked this conversation as resolved.
|
||
| // If getCollectionKey throws, the key is not a collection member | ||
| } | ||
|
|
||
| return cache.isRamOnlyKey(key); | ||
| } | ||
|
JKobrynski marked this conversation as resolved.
|
||
|
|
||
| /** | ||
| * Splits a collection member key into the collection key part and the ID part. | ||
| * @param key - The collection member key to split. | ||
|
|
@@ -869,6 +898,11 @@ function scheduleNotifyCollectionSubscribers<TKey extends OnyxKey>( | |
| function remove<TKey extends OnyxKey>(key: TKey, isProcessingCollectionUpdate?: boolean): Promise<void> { | ||
| cache.drop(key); | ||
| scheduleSubscriberUpdate(key, undefined as OnyxValue<TKey>, undefined, isProcessingCollectionUpdate); | ||
|
|
||
| if (isRamOnlyKey(key)) { | ||
| return Promise.resolve(); | ||
| } | ||
|
|
||
| return Storage.removeItem(key).then(() => undefined); | ||
| } | ||
|
|
||
|
|
@@ -1344,6 +1378,12 @@ function setWithRetry<TKey extends OnyxKey>({key, value, options}: SetParams<TKe | |
| return updatePromise; | ||
| } | ||
|
|
||
| // If a key is a RAM-only key or a member of RAM-only collection, we skip the step that modifies the storage | ||
| if (isRamOnlyKey(key)) { | ||
| OnyxUtils.sendActionToDevTools(OnyxUtils.METHOD.SET, key, valueWithoutNestedNullValues); | ||
| return updatePromise; | ||
| } | ||
|
|
||
| return Storage.setItem(key, valueWithoutNestedNullValues) | ||
| .catch((error) => OnyxUtils.retryOperation(error, setWithRetry, {key, value: valueWithoutNestedNullValues, options}, retryAttempt)) | ||
| .then(() => { | ||
|
|
@@ -1394,7 +1434,13 @@ function multiSetWithRetry(data: OnyxMultiSetInput, retryAttempt?: number): Prom | |
| return OnyxUtils.scheduleSubscriberUpdate(key, value); | ||
| }); | ||
|
|
||
| return Storage.multiSet(keyValuePairsToSet) | ||
| const keyValuePairsToStore = keyValuePairsToSet.filter((keyValuePair) => { | ||
| const [key] = keyValuePair; | ||
| // Filter out the RAM-only key value pairs, as they should not be saved to storage | ||
| return !isRamOnlyKey(key); | ||
| }); | ||
|
|
||
| return Storage.multiSet(keyValuePairsToStore) | ||
| .catch((error) => OnyxUtils.retryOperation(error, multiSetWithRetry, newData, retryAttempt)) | ||
| .then(() => { | ||
| OnyxUtils.sendActionToDevTools(OnyxUtils.METHOD.MULTI_SET, undefined, newData); | ||
|
|
@@ -1463,6 +1509,12 @@ function setCollectionWithRetry<TKey extends CollectionKeyBase>({collectionKey, | |
|
|
||
| const updatePromise = OnyxUtils.scheduleNotifyCollectionSubscribers(collectionKey, mutableCollection, previousCollection); | ||
|
|
||
| // RAM-only keys are not supposed to be saved to storage | ||
| if (isRamOnlyKey(collectionKey)) { | ||
|
JKobrynski marked this conversation as resolved.
|
||
| OnyxUtils.sendActionToDevTools(OnyxUtils.METHOD.SET_COLLECTION, undefined, mutableCollection); | ||
| return updatePromise; | ||
| } | ||
|
|
||
| return Storage.multiSet(keyValuePairs) | ||
| .catch((error) => OnyxUtils.retryOperation(error, setCollectionWithRetry, {collectionKey, collection}, retryAttempt)) | ||
| .then(() => { | ||
|
|
@@ -1573,11 +1625,13 @@ function mergeCollectionWithPatches<TKey extends CollectionKeyBase>( | |
|
|
||
| // New keys will be added via multiSet while existing keys will be updated using multiMerge | ||
| // This is because setting a key that doesn't exist yet with multiMerge will throw errors | ||
| if (keyValuePairsForExistingCollection.length > 0) { | ||
| // We can skip this step for RAM-only keys as they should never be saved to storage | ||
| if (!isRamOnlyKey(collectionKey) && keyValuePairsForExistingCollection.length > 0) { | ||
| promises.push(Storage.multiMerge(keyValuePairsForExistingCollection)); | ||
| } | ||
|
|
||
| if (keyValuePairsForNewCollection.length > 0) { | ||
| // We can skip this step for RAM-only keys as they should never be saved to storage | ||
| if (!isRamOnlyKey(collectionKey) && keyValuePairsForNewCollection.length > 0) { | ||
|
JKobrynski marked this conversation as resolved.
|
||
| promises.push(Storage.multiSet(keyValuePairsForNewCollection)); | ||
| } | ||
|
|
||
|
|
@@ -1656,6 +1710,11 @@ function partialSetCollection<TKey extends CollectionKeyBase>({collectionKey, co | |
|
|
||
| const updatePromise = scheduleNotifyCollectionSubscribers(collectionKey, mutableCollection, previousCollection); | ||
|
|
||
| if (isRamOnlyKey(collectionKey)) { | ||
| sendActionToDevTools(METHOD.SET_COLLECTION, undefined, mutableCollection); | ||
| return updatePromise; | ||
| } | ||
|
|
||
| return Storage.multiSet(keyValuePairs) | ||
| .catch((error) => retryOperation(error, partialSetCollection, {collectionKey, collection}, retryAttempt)) | ||
| .then(() => { | ||
|
|
@@ -1741,6 +1800,7 @@ const OnyxUtils = { | |
| setWithRetry, | ||
| multiSetWithRetry, | ||
| setCollectionWithRetry, | ||
| isRamOnlyKey, | ||
| }; | ||
|
|
||
| GlobalSettings.addGlobalSettingsChangeListener(({enablePerformanceMetrics}) => { | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.