-
Notifications
You must be signed in to change notification settings - Fork 90
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
base: main
Are you sure you want to change the base?
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -40,6 +40,7 @@ function init({ | |
| enablePerformanceMetrics = false, | ||
| enableDevTools = true, | ||
| skippableCollectionMemberIDs = [], | ||
| ramOnlyKeys = [], | ||
|
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. Be sure to run the automated documentation script. I expected the docs to be updated with this parameter. It would also be great to give it some comments.
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. Thanks, I ran the script and pushed the changes. I added this comment in the
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. Yeah, that looks good. Thanks! |
||
| snapshotMergeKeys = [], | ||
| }: InitOptions): void { | ||
| if (enablePerformanceMetrics) { | ||
|
|
@@ -54,6 +55,8 @@ function init({ | |
| OnyxUtils.setSkippableCollectionMemberIDs(new Set(skippableCollectionMemberIDs)); | ||
| OnyxUtils.setSnapshotMergeKeys(new Set(snapshotMergeKeys)); | ||
|
|
||
| cache.setRamOnlyKeys(new Set<OnyxKey>(ramOnlyKeys)); | ||
|
|
||
| if (shouldSyncMultipleInstances) { | ||
| Storage.keepInstancesSync?.((key, value) => { | ||
| cache.set(key, value); | ||
|
|
@@ -378,9 +381,10 @@ function clear(keysToPreserve: OnyxKey[] = []): Promise<void> { | |
| updatePromises.push(OnyxUtils.scheduleNotifyCollectionSubscribers(key, value.newValues, value.oldValues)); | ||
| } | ||
|
|
||
| // Exclude RAM-only keys to prevent them from being saved to storage | ||
| const defaultKeyValuePairs = Object.entries( | ||
| Object.keys(defaultKeyStates) | ||
| .filter((key) => !keysToPreserve.includes(key)) | ||
| .filter((key) => !keysToPreserve.includes(key) && !OnyxUtils.isRamOnlyKey(key)) | ||
|
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. Why exclude the RAM only keys 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. Because
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. Ah, I see. Would you mind adding that as a code comment so that it's obvious to others?
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. Done! |
||
| .reduce((obj: KeyValueMapping, key) => { | ||
| // eslint-disable-next-line no-param-reassign | ||
mountiny marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| obj[key] = defaultKeyStates[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. Issue 4: O(n) filtering on every multiSet call The new filter in For large batch operations (e.g., syncing many reports), this compounds. While acceptable for now since RAM-only keys are expected to be rare, if performance becomes an issue in the future, consider:
Low priority - just flagging for awareness. Issue 5: Consider adding RAM-only check to When a RAM-only key is set to Consider adding a RAM-only check in 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);
}Low priority since the storage operation will just be a no-op.
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 added that additional condition to
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 think we can confirm Issue 4 with Reassure – we already have a test for
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. That's correct! However, the perf tests don't include RAM-only keys, we should probably add them right?
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. It would be great to add those tests
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. Tests added! |
| 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. |
||
| // If collectionKey exists for a given key, check if it's a RAM-only key | ||
| return cache.isRamOnlyKey(collectionKey); | ||
| } catch { | ||
|
Comment on lines
+490
to
+495
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.
The new Useful? React with 👍 / 👎.
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 think this makes sense to clarify @JKobrynski , either the docs should be updated or the individual keys should be updated
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'm not sure I understand this problem correctly, this function is tested and should work correctly for RAM-only keys, RAM-only collection keys, and RAM-only collection member keys, the tests pass for all cases listed above. I updated the comment so every supported key is listed and added some examples. Let me know if that's what you meant, if not I will change it, thanks!
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 agree with @JKobrynski , I don't understand the comment because tests are actually covering all possible scenarios. |
||
| // If getCollectionKey throws, the key is not a collection member | ||
| } | ||
|
|
||
| return cache.isRamOnlyKey(key); | ||
| } | ||
JKobrynski marked this conversation as resolved.
Show resolved
Hide 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.
Show resolved
Hide 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.
Show resolved
Hide 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.