From 28bd489d28ab468dc6c0ea29cbb49bb313dd5284 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A1bio=20Henriques?= Date: Wed, 4 Dec 2024 19:08:50 +0000 Subject: [PATCH 01/11] Implement skippable collection member IDs logic in Onyx methods --- lib/Onyx.ts | 106 ++++++++++++++++++++++++++++++++++++++++------- lib/OnyxUtils.ts | 34 ++++++++++++++- lib/types.ts | 2 + 3 files changed, 124 insertions(+), 18 deletions(-) diff --git a/lib/Onyx.ts b/lib/Onyx.ts index 01ac9fcd..bd426a9b 100644 --- a/lib/Onyx.ts +++ b/lib/Onyx.ts @@ -41,9 +41,12 @@ function init({ maxCachedKeysCount = 1000, shouldSyncMultipleInstances = Boolean(global.localStorage), debugSetState = false, + skippableCollectionMemberIDs = [], }: InitOptions): void { Storage.init(); + OnyxUtils.setSkippableCollectionMemberIDs(skippableCollectionMemberIDs); + if (shouldSyncMultipleInstances) { Storage.keepInstancesSync?.((key, value) => { const prevValue = cache.get(key, false) as OnyxValue; @@ -120,6 +123,15 @@ function disconnect(connection: Connection): void { * @param value value to store */ function set(key: TKey, value: OnyxSetInput): Promise { + try { + const [, collectionMemberID] = OnyxUtils.splitCollectionMemberKey(key); + if (OnyxUtils.getSkippableCollectionMemberIDs().includes(collectionMemberID)) { + return Promise.resolve(); + } + } catch (e) { + // Key is not a collection one or something went wrong during split, so we proceed with the function's logic. + } + // When we use Onyx.set to set a key we want to clear the current delta changes from Onyx.merge that were queued // before the value was set. If Onyx.merge is currently reading the old value from storage, it will then not apply the changes. if (OnyxUtils.hasPendingMergeForKey(key)) { @@ -188,7 +200,26 @@ function set(key: TKey, value: OnyxSetInput): Promis * @param data object keyed by ONYXKEYS and the values to set */ function multiSet(data: OnyxMultiSetInput): Promise { - const keyValuePairsToSet = OnyxUtils.prepareKeyValuePairsForStorage(data, true); + let newData = data; + + const skippableCollectionMemberIDs = OnyxUtils.getSkippableCollectionMemberIDs(); + newData = Object.keys(newData).reduce((result: OnyxMultiSetInput, key) => { + try { + const [, collectionMemberID] = OnyxUtils.splitCollectionMemberKey(key); + if (!skippableCollectionMemberIDs.includes(collectionMemberID)) { + // eslint-disable-next-line no-param-reassign + result[key] = newData[key]; + } + } catch { + // Key is not a collection one or something went wrong during split, so we assign the data to result anyway. + // eslint-disable-next-line no-param-reassign + result[key] = newData[key]; + } + + return result; + }, {}); + + const keyValuePairsToSet = OnyxUtils.prepareKeyValuePairsForStorage(newData, true); const updatePromises = keyValuePairsToSet.map(([key, value]) => { const prevValue = cache.get(key, false); @@ -199,9 +230,9 @@ function multiSet(data: OnyxMultiSetInput): Promise { }); return Storage.multiSet(keyValuePairsToSet) - .catch((error) => OnyxUtils.evictStorageAndRetry(error, multiSet, data)) + .catch((error) => OnyxUtils.evictStorageAndRetry(error, multiSet, newData)) .then(() => { - OnyxUtils.sendActionToDevTools(OnyxUtils.METHOD.MULTI_SET, undefined, data); + OnyxUtils.sendActionToDevTools(OnyxUtils.METHOD.MULTI_SET, undefined, newData); return Promise.all(updatePromises); }) .then(() => undefined); @@ -224,6 +255,15 @@ function multiSet(data: OnyxMultiSetInput): Promise { * Onyx.merge(ONYXKEYS.POLICY, {name: 'My Workspace'}); // -> {id: 1, name: 'My Workspace'} */ function merge(key: TKey, changes: OnyxMergeInput): Promise { + try { + const [, collectionMemberID] = OnyxUtils.splitCollectionMemberKey(key); + if (OnyxUtils.getSkippableCollectionMemberIDs().includes(collectionMemberID)) { + return Promise.resolve(); + } + } catch (e) { + // Key is not a collection one or something went wrong during split, so we proceed with the function's logic. + } + const mergeQueue = OnyxUtils.getMergeQueue(); const mergeQueuePromise = OnyxUtils.getMergeQueuePromise(); @@ -338,19 +378,36 @@ function mergeCollection(collectionKey: TK return Promise.resolve(); } - const mergedCollection: OnyxInputKeyValueMapping = collection; + let resultCollection: OnyxInputKeyValueMapping = collection; // Confirm all the collection keys belong to the same parent - const mergedCollectionKeys = Object.keys(mergedCollection); + const mergedCollectionKeys = Object.keys(resultCollection); if (!OnyxUtils.doAllCollectionItemsBelongToSameParent(collectionKey, mergedCollectionKeys)) { return Promise.resolve(); } + const skippableCollectionMemberIDs = OnyxUtils.getSkippableCollectionMemberIDs(); + resultCollection = Object.keys(resultCollection).reduce((result: OnyxInputKeyValueMapping, key) => { + try { + const [, collectionMemberID] = OnyxUtils.splitCollectionMemberKey(key, collectionKey); + if (!skippableCollectionMemberIDs.includes(collectionMemberID)) { + // eslint-disable-next-line no-param-reassign + result[key] = resultCollection[key]; + } + } catch { + // Something went wrong during split, so we assign the data to result anyway. + // eslint-disable-next-line no-param-reassign + result[key] = resultCollection[key]; + } + + return result; + }, {}); + return OnyxUtils.getAllKeys() .then((persistedKeys) => { // Split to keys that exist in storage and keys that don't const keys = mergedCollectionKeys.filter((key) => { - if (mergedCollection[key] === null) { + if (resultCollection[key] === null) { OnyxUtils.remove(key); return false; } @@ -362,13 +419,13 @@ function mergeCollection(collectionKey: TK const cachedCollectionForExistingKeys = OnyxUtils.getCachedCollection(collectionKey, existingKeys); const existingKeyCollection = existingKeys.reduce((obj: OnyxInputKeyValueMapping, key) => { - const {isCompatible, existingValueType, newValueType} = utils.checkCompatibilityWithExistingValue(mergedCollection[key], cachedCollectionForExistingKeys[key]); + const {isCompatible, existingValueType, newValueType} = utils.checkCompatibilityWithExistingValue(resultCollection[key], cachedCollectionForExistingKeys[key]); if (!isCompatible) { Logger.logAlert(logMessages.incompatibleUpdateAlert(key, 'mergeCollection', existingValueType, newValueType)); return obj; } // eslint-disable-next-line no-param-reassign - obj[key] = mergedCollection[key]; + obj[key] = resultCollection[key]; return obj; }, {}) as Record>; @@ -377,7 +434,7 @@ function mergeCollection(collectionKey: TK if (persistedKeys.has(key)) { return; } - newCollection[key] = mergedCollection[key]; + newCollection[key] = resultCollection[key]; }); // When (multi-)merging the values with the existing values in storage, @@ -416,9 +473,9 @@ function mergeCollection(collectionKey: TK }); return Promise.all(promises) - .catch((error) => OnyxUtils.evictStorageAndRetry(error, mergeCollection, collectionKey, mergedCollection)) + .catch((error) => OnyxUtils.evictStorageAndRetry(error, mergeCollection, collectionKey, resultCollection)) .then(() => { - OnyxUtils.sendActionToDevTools(OnyxUtils.METHOD.MERGE_COLLECTION, undefined, mergedCollection); + OnyxUtils.sendActionToDevTools(OnyxUtils.METHOD.MERGE_COLLECTION, undefined, resultCollection); return promiseUpdate; }); }) @@ -722,8 +779,6 @@ function update(data: OnyxUpdate[]): Promise { .then(() => undefined); } -type BaseCollection = Record; - /** * Sets a collection by replacing all existing collection members with new values. * Any existing collection members not included in the new data will be removed. @@ -737,16 +792,35 @@ type BaseCollection = Record; * @param collectionKey e.g. `ONYXKEYS.COLLECTION.REPORT` * @param collection Object collection keyed by individual collection member keys and values */ -function setCollection(collectionKey: TKey, collection: OnyxMergeCollectionInput): Promise { - const newCollectionKeys = Object.keys(collection); +function setCollection(collectionKey: TKey, collection: OnyxMergeCollectionInput): Promise { + let resultCollection: OnyxInputKeyValueMapping = collection; + // Confirm all the collection keys belong to the same parent + const newCollectionKeys = Object.keys(resultCollection); if (!OnyxUtils.doAllCollectionItemsBelongToSameParent(collectionKey, newCollectionKeys)) { Logger.logAlert(`setCollection called with keys that do not belong to the same parent ${collectionKey}. Skipping this update.`); return Promise.resolve(); } + const skippableCollectionMemberIDs = OnyxUtils.getSkippableCollectionMemberIDs(); + resultCollection = Object.keys(resultCollection).reduce((result: OnyxInputKeyValueMapping, key) => { + try { + const [, collectionMemberID] = OnyxUtils.splitCollectionMemberKey(key, collectionKey); + if (!skippableCollectionMemberIDs.includes(collectionMemberID)) { + // eslint-disable-next-line no-param-reassign + result[key] = resultCollection[key]; + } + } catch { + // Something went wrong during split, so we assign the data to result anyway. + // eslint-disable-next-line no-param-reassign + result[key] = resultCollection[key]; + } + + return result; + }, {}); + return OnyxUtils.getAllKeys().then((persistedKeys) => { - const mutableCollection: BaseCollection = {...collection}; + const mutableCollection: OnyxInputKeyValueMapping = {...resultCollection}; persistedKeys.forEach((key) => { if (!key.startsWith(collectionKey)) { diff --git a/lib/OnyxUtils.ts b/lib/OnyxUtils.ts index ec966aca..9d85d197 100644 --- a/lib/OnyxUtils.ts +++ b/lib/OnyxUtils.ts @@ -86,6 +86,8 @@ let lastSubscriptionID = 0; // Connections can be made before `Onyx.init`. They would wait for this task before resolving const deferredInitTask = createDeferredTask(); +let skippableCollectionMemberIDs: string[] = []; + function getSnapshotKey(): OnyxKey | null { return snapshotKey; } @@ -125,6 +127,20 @@ function getEvictionBlocklist(): Record { return evictionBlocklist; } +/** + * Getter - TODO + */ +function getSkippableCollectionMemberIDs(): string[] { + return skippableCollectionMemberIDs; +} + +/** + * Setter - TODO + */ +function setSkippableCollectionMemberIDs(ids: string[]): void { + skippableCollectionMemberIDs = ids; +} + /** * Sets the initial values for the Onyx store * @@ -417,11 +433,23 @@ function isCollectionMemberKey(collect /** * Splits a collection member key into the collection key part and the ID part. * @param key - The collection member key to split. + * @param collectionKey - The collection key of the `key` param that can be passed in advance to optimize the function. * @returns A tuple where the first element is the collection part and the second element is the ID part, * or throws an Error if the key is not a collection one. */ -function splitCollectionMemberKey(key: TKey): [CollectionKeyType, string] { - const collectionKey = getCollectionKey(key); +function splitCollectionMemberKey( + key: TKey, + collectionKey?: string, +): [CollectionKeyType, string] { + if (collectionKey && !isCollectionMemberKey(collectionKey, key, collectionKey.length)) { + throw new Error(`Invalid '${collectionKey}' collection key provided, it isn't compatible with '${key}' key.`); + } + + if (!collectionKey) { + // eslint-disable-next-line no-param-reassign + collectionKey = getCollectionKey(key); + } + return [collectionKey as CollectionKeyType, key.slice(collectionKey.length)]; } @@ -1416,6 +1444,8 @@ const OnyxUtils = { subscribeToKey, unsubscribeFromKey, getEvictionBlocklist, + getSkippableCollectionMemberIDs, + setSkippableCollectionMemberIDs, }; export type {OnyxMethod}; diff --git a/lib/types.ts b/lib/types.ts index ea939a9b..ed5cbd57 100644 --- a/lib/types.ts +++ b/lib/types.ts @@ -468,6 +468,8 @@ type InitOptions = { /** Enables debugging setState() calls to connected components */ debugSetState?: boolean; + + skippableCollectionMemberIDs?: string[]; }; // eslint-disable-next-line @typescript-eslint/no-explicit-any From 0912da354b54538f8a1b69830757e3087c20aa80 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A1bio=20Henriques?= Date: Wed, 4 Dec 2024 19:19:41 +0000 Subject: [PATCH 02/11] Performance optimizations --- lib/Onyx.ts | 104 ++++++++++++++++++++++++++--------------------- lib/OnyxUtils.ts | 6 +-- 2 files changed, 61 insertions(+), 49 deletions(-) diff --git a/lib/Onyx.ts b/lib/Onyx.ts index bd426a9b..cd155712 100644 --- a/lib/Onyx.ts +++ b/lib/Onyx.ts @@ -45,7 +45,7 @@ function init({ }: InitOptions): void { Storage.init(); - OnyxUtils.setSkippableCollectionMemberIDs(skippableCollectionMemberIDs); + OnyxUtils.setSkippableCollectionMemberIDs(new Set(skippableCollectionMemberIDs)); if (shouldSyncMultipleInstances) { Storage.keepInstancesSync?.((key, value) => { @@ -123,13 +123,16 @@ function disconnect(connection: Connection): void { * @param value value to store */ function set(key: TKey, value: OnyxSetInput): Promise { - try { - const [, collectionMemberID] = OnyxUtils.splitCollectionMemberKey(key); - if (OnyxUtils.getSkippableCollectionMemberIDs().includes(collectionMemberID)) { - return Promise.resolve(); + const skippableCollectionMemberIDs = OnyxUtils.getSkippableCollectionMemberIDs(); + if (skippableCollectionMemberIDs.size) { + try { + const [, collectionMemberID] = OnyxUtils.splitCollectionMemberKey(key); + if (skippableCollectionMemberIDs.has(collectionMemberID)) { + return Promise.resolve(); + } + } catch (e) { + // Key is not a collection one or something went wrong during split, so we proceed with the function's logic. } - } catch (e) { - // Key is not a collection one or something went wrong during split, so we proceed with the function's logic. } // When we use Onyx.set to set a key we want to clear the current delta changes from Onyx.merge that were queued @@ -203,21 +206,23 @@ function multiSet(data: OnyxMultiSetInput): Promise { let newData = data; const skippableCollectionMemberIDs = OnyxUtils.getSkippableCollectionMemberIDs(); - newData = Object.keys(newData).reduce((result: OnyxMultiSetInput, key) => { - try { - const [, collectionMemberID] = OnyxUtils.splitCollectionMemberKey(key); - if (!skippableCollectionMemberIDs.includes(collectionMemberID)) { + if (skippableCollectionMemberIDs.size) { + newData = Object.keys(newData).reduce((result: OnyxMultiSetInput, key) => { + try { + const [, collectionMemberID] = OnyxUtils.splitCollectionMemberKey(key); + if (!skippableCollectionMemberIDs.has(collectionMemberID)) { + // eslint-disable-next-line no-param-reassign + result[key] = newData[key]; + } + } catch { + // Key is not a collection one or something went wrong during split, so we assign the data to result anyway. // eslint-disable-next-line no-param-reassign result[key] = newData[key]; } - } catch { - // Key is not a collection one or something went wrong during split, so we assign the data to result anyway. - // eslint-disable-next-line no-param-reassign - result[key] = newData[key]; - } - return result; - }, {}); + return result; + }, {}); + } const keyValuePairsToSet = OnyxUtils.prepareKeyValuePairsForStorage(newData, true); @@ -255,13 +260,16 @@ function multiSet(data: OnyxMultiSetInput): Promise { * Onyx.merge(ONYXKEYS.POLICY, {name: 'My Workspace'}); // -> {id: 1, name: 'My Workspace'} */ function merge(key: TKey, changes: OnyxMergeInput): Promise { - try { - const [, collectionMemberID] = OnyxUtils.splitCollectionMemberKey(key); - if (OnyxUtils.getSkippableCollectionMemberIDs().includes(collectionMemberID)) { - return Promise.resolve(); + const skippableCollectionMemberIDs = OnyxUtils.getSkippableCollectionMemberIDs(); + if (skippableCollectionMemberIDs.size) { + try { + const [, collectionMemberID] = OnyxUtils.splitCollectionMemberKey(key); + if (skippableCollectionMemberIDs.has(collectionMemberID)) { + return Promise.resolve(); + } + } catch (e) { + // Key is not a collection one or something went wrong during split, so we proceed with the function's logic. } - } catch (e) { - // Key is not a collection one or something went wrong during split, so we proceed with the function's logic. } const mergeQueue = OnyxUtils.getMergeQueue(); @@ -387,21 +395,23 @@ function mergeCollection(collectionKey: TK } const skippableCollectionMemberIDs = OnyxUtils.getSkippableCollectionMemberIDs(); - resultCollection = Object.keys(resultCollection).reduce((result: OnyxInputKeyValueMapping, key) => { - try { - const [, collectionMemberID] = OnyxUtils.splitCollectionMemberKey(key, collectionKey); - if (!skippableCollectionMemberIDs.includes(collectionMemberID)) { + if (skippableCollectionMemberIDs.size) { + resultCollection = Object.keys(resultCollection).reduce((result: OnyxInputKeyValueMapping, key) => { + try { + const [, collectionMemberID] = OnyxUtils.splitCollectionMemberKey(key, collectionKey); + if (!skippableCollectionMemberIDs.has(collectionMemberID)) { + // eslint-disable-next-line no-param-reassign + result[key] = resultCollection[key]; + } + } catch { + // Something went wrong during split, so we assign the data to result anyway. // eslint-disable-next-line no-param-reassign result[key] = resultCollection[key]; } - } catch { - // Something went wrong during split, so we assign the data to result anyway. - // eslint-disable-next-line no-param-reassign - result[key] = resultCollection[key]; - } - return result; - }, {}); + return result; + }, {}); + } return OnyxUtils.getAllKeys() .then((persistedKeys) => { @@ -803,21 +813,23 @@ function setCollection(collectionKey: TKey } const skippableCollectionMemberIDs = OnyxUtils.getSkippableCollectionMemberIDs(); - resultCollection = Object.keys(resultCollection).reduce((result: OnyxInputKeyValueMapping, key) => { - try { - const [, collectionMemberID] = OnyxUtils.splitCollectionMemberKey(key, collectionKey); - if (!skippableCollectionMemberIDs.includes(collectionMemberID)) { + if (skippableCollectionMemberIDs.size) { + resultCollection = Object.keys(resultCollection).reduce((result: OnyxInputKeyValueMapping, key) => { + try { + const [, collectionMemberID] = OnyxUtils.splitCollectionMemberKey(key, collectionKey); + if (!skippableCollectionMemberIDs.has(collectionMemberID)) { + // eslint-disable-next-line no-param-reassign + result[key] = resultCollection[key]; + } + } catch { + // Something went wrong during split, so we assign the data to result anyway. // eslint-disable-next-line no-param-reassign result[key] = resultCollection[key]; } - } catch { - // Something went wrong during split, so we assign the data to result anyway. - // eslint-disable-next-line no-param-reassign - result[key] = resultCollection[key]; - } - return result; - }, {}); + return result; + }, {}); + } return OnyxUtils.getAllKeys().then((persistedKeys) => { const mutableCollection: OnyxInputKeyValueMapping = {...resultCollection}; diff --git a/lib/OnyxUtils.ts b/lib/OnyxUtils.ts index 9d85d197..a780bcb8 100644 --- a/lib/OnyxUtils.ts +++ b/lib/OnyxUtils.ts @@ -86,7 +86,7 @@ let lastSubscriptionID = 0; // Connections can be made before `Onyx.init`. They would wait for this task before resolving const deferredInitTask = createDeferredTask(); -let skippableCollectionMemberIDs: string[] = []; +let skippableCollectionMemberIDs = new Set(); function getSnapshotKey(): OnyxKey | null { return snapshotKey; @@ -130,14 +130,14 @@ function getEvictionBlocklist(): Record { /** * Getter - TODO */ -function getSkippableCollectionMemberIDs(): string[] { +function getSkippableCollectionMemberIDs(): Set { return skippableCollectionMemberIDs; } /** * Setter - TODO */ -function setSkippableCollectionMemberIDs(ids: string[]): void { +function setSkippableCollectionMemberIDs(ids: Set): void { skippableCollectionMemberIDs = ids; } From 4c5a3b07c97f9265e934e9d476a3252dee7f1419 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A1bio=20Henriques?= Date: Mon, 9 Dec 2024 13:58:26 +0100 Subject: [PATCH 03/11] Add tests for splitCollectionMemberKey --- tests/unit/onyxUtilsTest.ts | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/tests/unit/onyxUtilsTest.ts b/tests/unit/onyxUtilsTest.ts index 305c7cbc..06e6be1d 100644 --- a/tests/unit/onyxUtilsTest.ts +++ b/tests/unit/onyxUtilsTest.ts @@ -45,6 +45,18 @@ describe('OnyxUtils', () => { OnyxUtils.splitCollectionMemberKey(''); }).toThrowError("Invalid '' key provided, only collection keys are allowed."); }); + + it('should allow passing the collection key beforehand for performance gains', () => { + const [collectionKey, id] = OnyxUtils.splitCollectionMemberKey(`${ONYXKEYS.COLLECTION.TEST_KEY}id1`, ONYXKEYS.COLLECTION.TEST_KEY); + expect(collectionKey).toEqual(ONYXKEYS.COLLECTION.TEST_KEY); + expect(id).toEqual('id1'); + }); + + it("should throw error if the passed collection key isn't compatible with the key", () => { + expect(() => { + OnyxUtils.splitCollectionMemberKey(`${ONYXKEYS.COLLECTION.TEST_KEY}id1`, ONYXKEYS.COLLECTION.TEST_LEVEL_KEY); + }).toThrowError("Invalid 'test_level_' collection key provided, it isn't compatible with 'test_id1' key."); + }); }); describe('getCollectionKey', () => { From 03c4afeaf648373e3decf3baaa9ea44e040cbfb4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A1bio=20Henriques?= Date: Mon, 9 Dec 2024 16:34:22 +0100 Subject: [PATCH 04/11] Start implementing tests for Onyx consumers --- lib/OnyxUtils.ts | 22 +++++++ tests/unit/onyxTest.ts | 113 ++++++++++++++++++++++++++++++++++++ tests/unit/useOnyxTest.ts | 45 ++++++++++++++ tests/unit/withOnyxTest.tsx | 26 +++++++++ 4 files changed, 206 insertions(+) diff --git a/lib/OnyxUtils.ts b/lib/OnyxUtils.ts index 4cda8260..80e1e915 100644 --- a/lib/OnyxUtils.ts +++ b/lib/OnyxUtils.ts @@ -258,6 +258,17 @@ function reduceCollectionWithSelector>(key: TKey): Promise { + if (skippableCollectionMemberIDs.size) { + try { + const [, collectionMemberID] = splitCollectionMemberKey(key); + if (skippableCollectionMemberIDs.has(collectionMemberID)) { + return Promise.resolve(undefined as TValue); + } + } catch (e) { + // Key is not a collection one or something went wrong during split, so we proceed with the function's logic. + } + } + // When we already have the value in cache - resolve right away if (cache.hasCacheForKey(key)) { return Promise.resolve(cache.get(key) as TValue); @@ -309,6 +320,17 @@ function multiGet(keys: CollectionKeyBase[]): Promise { + if (skippableCollectionMemberIDs.size) { + try { + const [, collectionMemberID] = OnyxUtils.splitCollectionMemberKey(key); + if (skippableCollectionMemberIDs.has(collectionMemberID)) { + return; + } + } catch (e) { + // Key is not a collection one or something went wrong during split, so we proceed with the function's logic. + } + } + const cacheValue = cache.get(key) as OnyxValue; if (cacheValue) { dataMap.set(key, cacheValue); diff --git a/tests/unit/onyxTest.ts b/tests/unit/onyxTest.ts index ed75c46c..be816c98 100644 --- a/tests/unit/onyxTest.ts +++ b/tests/unit/onyxTest.ts @@ -30,6 +30,7 @@ Onyx.init({ [ONYX_KEYS.OTHER_TEST]: 42, [ONYX_KEYS.KEY_WITH_UNDERSCORE]: 'default', }, + skippableCollectionMemberIDs: ['skippable-id'], }); describe('Onyx', () => { @@ -1906,4 +1907,116 @@ describe('Onyx', () => { }); }); }); + + describe('skippable collection member ids', () => { + it('should skip the collection member id value when using Onyx.set()', async () => { + let testKeyValue: unknown; + connection = Onyx.connect({ + key: ONYX_KEYS.COLLECTION.TEST_KEY, + initWithStoredValues: false, + waitForCollectionCallback: true, + callback: (value) => { + testKeyValue = value; + }, + }); + + await Onyx.set(`${ONYX_KEYS.COLLECTION.TEST_KEY}entry1`, {id: 'entry1_id', name: 'entry2_name'}); + await Onyx.set(`${ONYX_KEYS.COLLECTION.TEST_KEY}skippable-id`, {id: 'skippable-id_id', name: 'skippable-id_name'}); + + expect(testKeyValue).toEqual({ + [`${ONYX_KEYS.COLLECTION.TEST_KEY}entry1`]: {id: 'entry1_id', name: 'entry2_name'}, + }); + }); + + it('should skip the collection member id value when using Onyx.merge()', async () => { + let testKeyValue: unknown; + connection = Onyx.connect({ + key: ONYX_KEYS.COLLECTION.TEST_KEY, + initWithStoredValues: false, + waitForCollectionCallback: true, + callback: (value) => { + testKeyValue = value; + }, + }); + + await Onyx.merge(`${ONYX_KEYS.COLLECTION.TEST_KEY}entry1`, {id: 'entry1_id', name: 'entry2_name'}); + await Onyx.merge(`${ONYX_KEYS.COLLECTION.TEST_KEY}skippable-id`, {id: 'skippable-id_id', name: 'skippable-id_name'}); + + expect(testKeyValue).toEqual({ + [`${ONYX_KEYS.COLLECTION.TEST_KEY}entry1`]: {id: 'entry1_id', name: 'entry2_name'}, + }); + }); + + it('should skip the collection member id value when using Onyx.mergeCollection()', async () => { + let testKeyValue: unknown; + connection = Onyx.connect({ + key: ONYX_KEYS.COLLECTION.TEST_KEY, + initWithStoredValues: false, + waitForCollectionCallback: true, + callback: (value) => { + testKeyValue = value; + }, + }); + + await Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, { + [`${ONYX_KEYS.COLLECTION.TEST_KEY}entry1`]: {id: 'entry1_id', name: 'entry1_name'}, + [`${ONYX_KEYS.COLLECTION.TEST_KEY}entry2`]: {id: 'entry2_id', name: 'entry2_name'}, + [`${ONYX_KEYS.COLLECTION.TEST_KEY}skippable-id`]: {id: 'skippable-id_id', name: 'skippable-id_name'}, + } as GenericCollection); + + expect(testKeyValue).toEqual({ + [`${ONYX_KEYS.COLLECTION.TEST_KEY}entry1`]: {id: 'entry1_id', name: 'entry1_name'}, + [`${ONYX_KEYS.COLLECTION.TEST_KEY}entry2`]: {id: 'entry2_id', name: 'entry2_name'}, + [`${ONYX_KEYS.COLLECTION.TEST_KEY}skippable-id`]: undefined, + }); + }); + + it('should skip the collection member id value when using Onyx.setCollection()', async () => { + let testKeyValue: unknown; + connection = Onyx.connect({ + key: ONYX_KEYS.COLLECTION.TEST_KEY, + initWithStoredValues: false, + waitForCollectionCallback: true, + callback: (value) => { + testKeyValue = value; + }, + }); + + await Onyx.setCollection(ONYX_KEYS.COLLECTION.TEST_KEY, { + [`${ONYX_KEYS.COLLECTION.TEST_KEY}entry1`]: {id: 'entry1_id', name: 'entry1_name'}, + [`${ONYX_KEYS.COLLECTION.TEST_KEY}entry2`]: {id: 'entry2_id', name: 'entry2_name'}, + [`${ONYX_KEYS.COLLECTION.TEST_KEY}skippable-id`]: {id: 'skippable-id_id', name: 'skippable-id_name'}, + } as GenericCollection); + + expect(testKeyValue).toEqual({ + [`${ONYX_KEYS.COLLECTION.TEST_KEY}entry1`]: {id: 'entry1_id', name: 'entry1_name'}, + [`${ONYX_KEYS.COLLECTION.TEST_KEY}entry2`]: {id: 'entry2_id', name: 'entry2_name'}, + [`${ONYX_KEYS.COLLECTION.TEST_KEY}skippable-id`]: undefined, + }); + }); + + it('should skip the collection member id value when using Onyx.multiSet()', async () => { + let testKeyValue: unknown; + connection = Onyx.connect({ + key: ONYX_KEYS.COLLECTION.TEST_KEY, + initWithStoredValues: false, + waitForCollectionCallback: true, + callback: (value) => { + testKeyValue = value; + }, + }); + + await Onyx.multiSet({ + [`${ONYX_KEYS.COLLECTION.TEST_KEY}entry1`]: {id: 'entry1_id', name: 'entry1_name'}, + [`${ONYX_KEYS.COLLECTION.TEST_KEY}entry2`]: {id: 'entry2_id', name: 'entry2_name'}, + [`${ONYX_KEYS.COLLECTION.TEST_KEY}skippable-id`]: {id: 'skippable-id_id', name: 'skippable-id_name'}, + } as GenericCollection); + + expect(testKeyValue).toEqual({ + [`${ONYX_KEYS.COLLECTION.TEST_KEY}entry1`]: {id: 'entry1_id', name: 'entry1_name'}, + [`${ONYX_KEYS.COLLECTION.TEST_KEY}entry2`]: {id: 'entry2_id', name: 'entry2_name'}, + [`${ONYX_KEYS.COLLECTION.TEST_KEY}skippable-id`]: undefined, + }); + }); + }); }); diff --git a/tests/unit/useOnyxTest.ts b/tests/unit/useOnyxTest.ts index 2c874469..57da1bde 100644 --- a/tests/unit/useOnyxTest.ts +++ b/tests/unit/useOnyxTest.ts @@ -19,6 +19,7 @@ const ONYXKEYS = { Onyx.init({ keys: ONYXKEYS, safeEvictionKeys: [ONYXKEYS.COLLECTION.EVICTABLE_TEST_KEY], + skippableCollectionMemberIDs: ['skippable-id'], }); beforeEach(() => Onyx.clear()); @@ -659,6 +660,50 @@ describe('useOnyx', () => { }); }); + describe('skippable collection member ids', () => { + it('should 1', async () => { + Onyx.mergeCollection(ONYXKEYS.COLLECTION.TEST_KEY, { + [`${ONYXKEYS.COLLECTION.TEST_KEY}entry1`]: {id: 'entry1_id', name: 'entry1_name'}, + [`${ONYXKEYS.COLLECTION.TEST_KEY}entry2`]: {id: 'entry2_id', name: 'entry2_name'}, + [`${ONYXKEYS.COLLECTION.TEST_KEY}skippable-id`]: {id: 'skippable-id_id', name: 'skippable-id_name'}, + } as GenericCollection); + + const {result} = renderHook(() => useOnyx(ONYXKEYS.COLLECTION.TEST_KEY)); + + await act(async () => waitForPromisesToResolve()); + + expect(result.current[0]).toEqual({ + [`${ONYXKEYS.COLLECTION.TEST_KEY}entry1`]: {id: 'entry1_id', name: 'entry1_name'}, + [`${ONYXKEYS.COLLECTION.TEST_KEY}entry2`]: {id: 'entry2_id', name: 'entry2_name'}, + [`${ONYXKEYS.COLLECTION.TEST_KEY}skippable-id`]: undefined, + }); + expect(result.current[1].status).toEqual('loaded'); + + await act(async () => Onyx.merge(`${ONYXKEYS.COLLECTION.TEST_KEY}skippable-id`, 'something')); + + expect(result.current[0]).toEqual({ + [`${ONYXKEYS.COLLECTION.TEST_KEY}entry1`]: {id: 'entry1_id', name: 'entry1_name'}, + [`${ONYXKEYS.COLLECTION.TEST_KEY}entry2`]: {id: 'entry2_id', name: 'entry2_name'}, + [`${ONYXKEYS.COLLECTION.TEST_KEY}skippable-id`]: undefined, + }); + expect(result.current[1].status).toEqual('loaded'); + }); + + it('should 2', async () => { + // @ts-expect-error bypass + await StorageMock.setItem(`${ONYXKEYS.COLLECTION.TEST_KEY}skippable-id`, 'something'); + + const {result} = renderHook(() => useOnyx(ONYXKEYS.COLLECTION.TEST_KEY)); + + await act(async () => waitForPromisesToResolve()); + + expect(result.current[0]).toEqual({ + [`${ONYXKEYS.COLLECTION.TEST_KEY}skippable-id`]: undefined, + }); + expect(result.current[1].status).toEqual('loaded'); + }); + }); + // This test suite must be the last one to avoid problems when running the other tests here. describe('canEvict', () => { const error = (key: string) => `canEvict can't be used on key '${key}'. This key must explicitly be flagged as safe for removal by adding it to Onyx.init({safeEvictionKeys: []}).`; diff --git a/tests/unit/withOnyxTest.tsx b/tests/unit/withOnyxTest.tsx index b29e8f40..08c7cbb9 100644 --- a/tests/unit/withOnyxTest.tsx +++ b/tests/unit/withOnyxTest.tsx @@ -28,6 +28,7 @@ const ONYX_KEYS = { Onyx.init({ keys: ONYX_KEYS, + skippableCollectionMemberIDs: ['skippable-id'], }); beforeEach(() => Onyx.clear()); @@ -817,4 +818,29 @@ describe('withOnyxTest', () => { textComponent = renderResult.getByText('null'); expect(textComponent).not.toBeNull(); }); + + describe('skippable collection member ids', () => { + it('should 1', async () => { + // TODO: Finish + const TestComponentWithOnyx = withOnyx({ + text: { + key: ONYX_KEYS.COLLECTION.TEST_KEY, + }, + })(ViewWithCollections); + const onRender = jest.fn(); + const renderResult = render(); + + Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, { + [`${ONYX_KEYS.COLLECTION.TEST_KEY}entry1`]: {ID: 1}, + [`${ONYX_KEYS.COLLECTION.TEST_KEY}entry2`]: {ID: 2}, + [`${ONYX_KEYS.COLLECTION.TEST_KEY}skippable-id`]: {ID: 3}, + } as GenericCollection); + + return waitForPromisesToResolve().then(() => { + expect(renderResult.getByText('1')).not.toBeNull(); + expect(renderResult.getByText('2')).not.toBeNull(); + expect(renderResult.getByText('3')).toBeNull(); + }); + }); + }); }); From f34eb51c9404cc96070ca3d62a5fc98e97e8bb03 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A1bio=20Henriques?= Date: Sat, 14 Dec 2024 20:24:57 +0000 Subject: [PATCH 05/11] Improve logic and tests --- lib/Onyx.ts | 33 +++++------- lib/OnyxUtils.ts | 45 ++++++++-------- tests/components/ViewWithCollections.tsx | 2 +- tests/unit/useOnyxTest.ts | 29 +++++++---- tests/unit/withOnyxTest.tsx | 66 ++++++++++++++++++------ 5 files changed, 106 insertions(+), 69 deletions(-) diff --git a/lib/Onyx.ts b/lib/Onyx.ts index 5f8debcc..50ee7255 100644 --- a/lib/Onyx.ts +++ b/lib/Onyx.ts @@ -131,6 +131,12 @@ function disconnect(connection: Connection): void { * @param value value to store */ function set(key: TKey, value: OnyxSetInput): Promise { + // When we use Onyx.set to set a key we want to clear the current delta changes from Onyx.merge that were queued + // before the value was set. If Onyx.merge is currently reading the old value from storage, it will then not apply the changes. + if (OnyxUtils.hasPendingMergeForKey(key)) { + delete OnyxUtils.getMergeQueue()[key]; + } + const skippableCollectionMemberIDs = OnyxUtils.getSkippableCollectionMemberIDs(); if (skippableCollectionMemberIDs.size) { try { @@ -143,12 +149,6 @@ function set(key: TKey, value: OnyxSetInput): Promis } } - // When we use Onyx.set to set a key we want to clear the current delta changes from Onyx.merge that were queued - // before the value was set. If Onyx.merge is currently reading the old value from storage, it will then not apply the changes. - if (OnyxUtils.hasPendingMergeForKey(key)) { - delete OnyxUtils.getMergeQueue()[key]; - } - // Onyx.set will ignore `undefined` values as inputs, therefore we can return early. if (value === undefined) { return Promise.resolve(); @@ -218,10 +218,8 @@ function multiSet(data: OnyxMultiSetInput): Promise { newData = Object.keys(newData).reduce((result: OnyxMultiSetInput, key) => { try { const [, collectionMemberID] = OnyxUtils.splitCollectionMemberKey(key); - if (!skippableCollectionMemberIDs.has(collectionMemberID)) { - // eslint-disable-next-line no-param-reassign - result[key] = newData[key]; - } + // eslint-disable-next-line no-param-reassign + result[key] = !skippableCollectionMemberIDs.has(collectionMemberID) ? newData[key] : null; } catch { // Key is not a collection one or something went wrong during split, so we assign the data to result anyway. // eslint-disable-next-line no-param-reassign @@ -273,7 +271,8 @@ function merge(key: TKey, changes: OnyxMergeInput): try { const [, collectionMemberID] = OnyxUtils.splitCollectionMemberKey(key); if (skippableCollectionMemberIDs.has(collectionMemberID)) { - return Promise.resolve(); + // eslint-disable-next-line no-param-reassign + changes = undefined; } } catch (e) { // Key is not a collection one or something went wrong during split, so we proceed with the function's logic. @@ -407,10 +406,8 @@ function mergeCollection(collectionKey: TK resultCollection = Object.keys(resultCollection).reduce((result: OnyxInputKeyValueMapping, key) => { try { const [, collectionMemberID] = OnyxUtils.splitCollectionMemberKey(key, collectionKey); - if (!skippableCollectionMemberIDs.has(collectionMemberID)) { - // eslint-disable-next-line no-param-reassign - result[key] = resultCollection[key]; - } + // eslint-disable-next-line no-param-reassign + result[key] = !skippableCollectionMemberIDs.has(collectionMemberID) ? resultCollection[key] : null; } catch { // Something went wrong during split, so we assign the data to result anyway. // eslint-disable-next-line no-param-reassign @@ -825,10 +822,8 @@ function setCollection(collectionKey: TKey resultCollection = Object.keys(resultCollection).reduce((result: OnyxInputKeyValueMapping, key) => { try { const [, collectionMemberID] = OnyxUtils.splitCollectionMemberKey(key, collectionKey); - if (!skippableCollectionMemberIDs.has(collectionMemberID)) { - // eslint-disable-next-line no-param-reassign - result[key] = resultCollection[key]; - } + // eslint-disable-next-line no-param-reassign + result[key] = !skippableCollectionMemberIDs.has(collectionMemberID) ? resultCollection[key] : null; } catch { // Something went wrong during split, so we assign the data to result anyway. // eslint-disable-next-line no-param-reassign diff --git a/lib/OnyxUtils.ts b/lib/OnyxUtils.ts index 80e1e915..d13e95f0 100644 --- a/lib/OnyxUtils.ts +++ b/lib/OnyxUtils.ts @@ -258,17 +258,6 @@ function reduceCollectionWithSelector>(key: TKey): Promise { - if (skippableCollectionMemberIDs.size) { - try { - const [, collectionMemberID] = splitCollectionMemberKey(key); - if (skippableCollectionMemberIDs.has(collectionMemberID)) { - return Promise.resolve(undefined as TValue); - } - } catch (e) { - // Key is not a collection one or something went wrong during split, so we proceed with the function's logic. - } - } - // When we already have the value in cache - resolve right away if (cache.hasCacheForKey(key)) { return Promise.resolve(cache.get(key) as TValue); @@ -284,6 +273,18 @@ function get>(key: TKey): P // Otherwise retrieve the value from storage and capture a promise to aid concurrent usages const promise = Storage.getItem(key) .then((val) => { + if (skippableCollectionMemberIDs.size) { + try { + const [, collectionMemberID] = splitCollectionMemberKey(key); + if (skippableCollectionMemberIDs.has(collectionMemberID)) { + // eslint-disable-next-line no-param-reassign + val = undefined as OnyxValue; + } + } catch (e) { + // Key is not a collection one or something went wrong during split, so we proceed with the function's logic. + } + } + if (val === undefined) { cache.addNullishStorageKey(key); return undefined; @@ -320,17 +321,6 @@ function multiGet(keys: CollectionKeyBase[]): Promise { - if (skippableCollectionMemberIDs.size) { - try { - const [, collectionMemberID] = OnyxUtils.splitCollectionMemberKey(key); - if (skippableCollectionMemberIDs.has(collectionMemberID)) { - return; - } - } catch (e) { - // Key is not a collection one or something went wrong during split, so we proceed with the function's logic. - } - } - const cacheValue = cache.get(key) as OnyxValue; if (cacheValue) { dataMap.set(key, cacheValue); @@ -373,6 +363,17 @@ function multiGet(keys: CollectionKeyBase[]): Promise = {}; values.forEach(([key, value]) => { + if (skippableCollectionMemberIDs.size) { + try { + const [, collectionMemberID] = OnyxUtils.splitCollectionMemberKey(key); + if (skippableCollectionMemberIDs.has(collectionMemberID)) { + return; + } + } catch (e) { + // Key is not a collection one or something went wrong during split, so we proceed with the function's logic. + } + } + dataMap.set(key, value as OnyxValue); temp[key] = value as OnyxValue; }); diff --git a/tests/components/ViewWithCollections.tsx b/tests/components/ViewWithCollections.tsx index 799168c6..fd15f5e9 100644 --- a/tests/components/ViewWithCollections.tsx +++ b/tests/components/ViewWithCollections.tsx @@ -26,7 +26,7 @@ function ViewWithCollections( {Object.values(collections).map((collection, i) => ( // eslint-disable-next-line react/no-array-index-key - {collection.ID} + {collection?.ID} ))} ); diff --git a/tests/unit/useOnyxTest.ts b/tests/unit/useOnyxTest.ts index 57da1bde..c735f6a9 100644 --- a/tests/unit/useOnyxTest.ts +++ b/tests/unit/useOnyxTest.ts @@ -661,7 +661,7 @@ describe('useOnyx', () => { }); describe('skippable collection member ids', () => { - it('should 1', async () => { + it('should always return undefined entry when subscribing to a collection with skippable member ids', async () => { Onyx.mergeCollection(ONYXKEYS.COLLECTION.TEST_KEY, { [`${ONYXKEYS.COLLECTION.TEST_KEY}entry1`]: {id: 'entry1_id', name: 'entry1_name'}, [`${ONYXKEYS.COLLECTION.TEST_KEY}entry2`]: {id: 'entry2_id', name: 'entry2_name'}, @@ -679,27 +679,36 @@ describe('useOnyx', () => { }); expect(result.current[1].status).toEqual('loaded'); - await act(async () => Onyx.merge(`${ONYXKEYS.COLLECTION.TEST_KEY}skippable-id`, 'something')); + await act(async () => + Onyx.mergeCollection(ONYXKEYS.COLLECTION.TEST_KEY, { + [`${ONYXKEYS.COLLECTION.TEST_KEY}entry1`]: {id: 'entry1_id', name: 'entry1_name_changed'}, + [`${ONYXKEYS.COLLECTION.TEST_KEY}entry2`]: {id: 'entry2_id', name: 'entry2_name_changed'}, + [`${ONYXKEYS.COLLECTION.TEST_KEY}skippable-id`]: {id: 'skippable-id_id', name: 'skippable-id_name_changed'}, + } as GenericCollection), + ); expect(result.current[0]).toEqual({ - [`${ONYXKEYS.COLLECTION.TEST_KEY}entry1`]: {id: 'entry1_id', name: 'entry1_name'}, - [`${ONYXKEYS.COLLECTION.TEST_KEY}entry2`]: {id: 'entry2_id', name: 'entry2_name'}, + [`${ONYXKEYS.COLLECTION.TEST_KEY}entry1`]: {id: 'entry1_id', name: 'entry1_name_changed'}, + [`${ONYXKEYS.COLLECTION.TEST_KEY}entry2`]: {id: 'entry2_id', name: 'entry2_name_changed'}, [`${ONYXKEYS.COLLECTION.TEST_KEY}skippable-id`]: undefined, }); expect(result.current[1].status).toEqual('loaded'); }); - it('should 2', async () => { + it('should always return undefined when subscribing to a skippable collection member id', async () => { // @ts-expect-error bypass - await StorageMock.setItem(`${ONYXKEYS.COLLECTION.TEST_KEY}skippable-id`, 'something'); + await StorageMock.setItem(`${ONYXKEYS.COLLECTION.TEST_KEY}skippable-id`, 'skippable-id_value'); - const {result} = renderHook(() => useOnyx(ONYXKEYS.COLLECTION.TEST_KEY)); + const {result} = renderHook(() => useOnyx(`${ONYXKEYS.COLLECTION.TEST_KEY}skippable-id`)); await act(async () => waitForPromisesToResolve()); - expect(result.current[0]).toEqual({ - [`${ONYXKEYS.COLLECTION.TEST_KEY}skippable-id`]: undefined, - }); + expect(result.current[0]).toEqual(undefined); + expect(result.current[1].status).toEqual('loaded'); + + await act(async () => Onyx.merge(`${ONYXKEYS.COLLECTION.TEST_KEY}skippable-id`, 'skippable-id_value_changed')); + + expect(result.current[0]).toEqual(undefined); expect(result.current[1].status).toEqual('loaded'); }); }); diff --git a/tests/unit/withOnyxTest.tsx b/tests/unit/withOnyxTest.tsx index 08c7cbb9..a36eb30d 100644 --- a/tests/unit/withOnyxTest.tsx +++ b/tests/unit/withOnyxTest.tsx @@ -1,6 +1,6 @@ /* eslint-disable rulesdir/onyx-props-must-have-default */ import React from 'react'; -import {render} from '@testing-library/react-native'; +import {act, render} from '@testing-library/react-native'; import Onyx, {withOnyx} from '../../lib'; import type {ViewWithTextOnyxProps, ViewWithTextProps} from '../components/ViewWithText'; import ViewWithText from '../components/ViewWithText'; @@ -820,27 +820,59 @@ describe('withOnyxTest', () => { }); describe('skippable collection member ids', () => { - it('should 1', async () => { - // TODO: Finish - const TestComponentWithOnyx = withOnyx({ - text: { + it('should always return undefined entry when subscribing to a collection with skippable member ids', async () => { + await Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, { + [`${ONYX_KEYS.COLLECTION.TEST_KEY}entry1`]: {ID: 'entry1_id'}, + [`${ONYX_KEYS.COLLECTION.TEST_KEY}entry2`]: {ID: 'entry2_id'}, + [`${ONYX_KEYS.COLLECTION.TEST_KEY}skippable-id`]: {ID: 'skippable-id_id'}, + } as GenericCollection); + + const TestComponentWithOnyx = withOnyx({ + collections: { key: ONYX_KEYS.COLLECTION.TEST_KEY, }, })(ViewWithCollections); - const onRender = jest.fn(); - const renderResult = render(); + const renderResult = render(); - Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, { - [`${ONYX_KEYS.COLLECTION.TEST_KEY}entry1`]: {ID: 1}, - [`${ONYX_KEYS.COLLECTION.TEST_KEY}entry2`]: {ID: 2}, - [`${ONYX_KEYS.COLLECTION.TEST_KEY}skippable-id`]: {ID: 3}, - } as GenericCollection); + await waitForPromisesToResolve(); - return waitForPromisesToResolve().then(() => { - expect(renderResult.getByText('1')).not.toBeNull(); - expect(renderResult.getByText('2')).not.toBeNull(); - expect(renderResult.getByText('3')).toBeNull(); - }); + expect(renderResult.queryByText('entry1_id')).not.toBeNull(); + expect(renderResult.queryByText('entry2_id')).not.toBeNull(); + expect(renderResult.queryByText('entry3_id')).toBeNull(); + + await act(async () => + Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, { + [`${ONYX_KEYS.COLLECTION.TEST_KEY}entry1`]: {ID: 'entry1_id_changed'}, + [`${ONYX_KEYS.COLLECTION.TEST_KEY}entry2`]: {ID: 'entry2_id_changed'}, + [`${ONYX_KEYS.COLLECTION.TEST_KEY}skippable-id`]: {ID: 'skippable-id_id_changed'}, + } as GenericCollection), + ); + + expect(renderResult.queryByText('entry1_id_changed')).not.toBeNull(); + expect(renderResult.queryByText('entry2_id_changed')).not.toBeNull(); + expect(renderResult.queryByText('skippable-id_id_changed')).toBeNull(); + }); + + it('should always return undefined when subscribing to a skippable collection member id', async () => { + const TestComponentWithOnyx = withOnyx({ + text: { + key: `${ONYX_KEYS.COLLECTION.TEST_KEY}skippable-id`, + }, + })(ViewWithText); + + // @ts-expect-error bypass + await StorageMock.setItem(`${ONYX_KEYS.COLLECTION.TEST_KEY}skippable-id`, 'skippable-id_value'); + + const renderResult = render(); + + await waitForPromisesToResolve(); + await waitForPromisesToResolve(); + + expect(renderResult.queryByText('skippable-id_value')).toBeNull(); + + await act(async () => Onyx.merge(`${ONYX_KEYS.COLLECTION.TEST_KEY}skippable-id`, 'skippable-id_value_changed')); + + expect(renderResult.queryByText('skippable-id_value_changed')).toBeNull(); }); }); }); From 192568b582aba40cd6440beebedc7e4c79cdf0ca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A1bio=20Henriques?= Date: Sat, 14 Dec 2024 20:36:08 +0000 Subject: [PATCH 06/11] Minor fix --- tests/unit/useOnyxTest.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/unit/useOnyxTest.ts b/tests/unit/useOnyxTest.ts index c735f6a9..07bfc87f 100644 --- a/tests/unit/useOnyxTest.ts +++ b/tests/unit/useOnyxTest.ts @@ -703,12 +703,12 @@ describe('useOnyx', () => { await act(async () => waitForPromisesToResolve()); - expect(result.current[0]).toEqual(undefined); + expect(result.current[0]).toBeUndefined(); expect(result.current[1].status).toEqual('loaded'); await act(async () => Onyx.merge(`${ONYXKEYS.COLLECTION.TEST_KEY}skippable-id`, 'skippable-id_value_changed')); - expect(result.current[0]).toEqual(undefined); + expect(result.current[0]).toBeUndefined(); expect(result.current[1].status).toEqual('loaded'); }); }); From 74cea97a9f360eaa05163f0dbde496c43c724365 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A1bio=20Henriques?= Date: Sat, 14 Dec 2024 20:58:35 +0000 Subject: [PATCH 07/11] Add description to skippableCollectionMemberIDs option --- lib/types.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lib/types.ts b/lib/types.ts index 26a24862..b722fa41 100644 --- a/lib/types.ts +++ b/lib/types.ts @@ -475,6 +475,10 @@ type InitOptions = { */ enablePerformanceMetrics?: boolean; + /** + * Array of collection member IDs which updates will be ignored when using Onyx methods. + * Additionally, any subscribers from these keys to won't receive any data from Onyx. + */ skippableCollectionMemberIDs?: string[]; }; From d65b1ece19d7e67943cae7a9759f64034a422afe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A1bio=20Henriques?= Date: Sat, 14 Dec 2024 20:59:08 +0000 Subject: [PATCH 08/11] Update docs --- API-INTERNAL.md | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/API-INTERNAL.md b/API-INTERNAL.md index 33d5f4de..a66208fe 100644 --- a/API-INTERNAL.md +++ b/API-INTERNAL.md @@ -20,6 +20,12 @@
getEvictionBlocklist()

Getter - returns the eviction block list.

+
getSkippableCollectionMemberIDs()
+

Getter - TODO

+
+
setSkippableCollectionMemberIDs()
+

Setter - TODO

+
initStoreValues(keys, initialKeyStates, safeEvictionKeys)

Sets the initial values for the Onyx store

@@ -53,7 +59,7 @@ The resulting collection will only contain items that are returned by the select

Checks to see if the subscriber's supplied key is associated with a collection of keys.

-
splitCollectionMemberKey(key)
+
splitCollectionMemberKey(key, collectionKey)

Splits a collection member key into the collection key part and the ID part.

isKeyMatch()
@@ -187,6 +193,18 @@ Getter - returns the deffered init task. ## getEvictionBlocklist() Getter - returns the eviction block list. +**Kind**: global function + + +## getSkippableCollectionMemberIDs() +Getter - TODO + +**Kind**: global function + + +## setSkippableCollectionMemberIDs() +Setter - TODO + **Kind**: global function @@ -268,7 +286,7 @@ is associated with a collection of keys. **Kind**: global function -## splitCollectionMemberKey(key) ⇒ +## splitCollectionMemberKey(key, collectionKey) ⇒ Splits a collection member key into the collection key part and the ID part. **Kind**: global function @@ -278,6 +296,7 @@ or throws an Error if the key is not a collection one. | Param | Description | | --- | --- | | key | The collection member key to split. | +| collectionKey | The collection key of the `key` param that can be passed in advance to optimize the function. | From c235575f861eb1054a19c8c34168141d230bef91 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A1bio=20Henriques?= Date: Mon, 13 Jan 2025 18:27:47 +0000 Subject: [PATCH 09/11] Minor improvements --- lib/Onyx.ts | 2 +- tests/unit/onyxTest.ts | 3 --- tests/unit/useOnyxTest.ts | 2 -- 3 files changed, 1 insertion(+), 6 deletions(-) diff --git a/lib/Onyx.ts b/lib/Onyx.ts index 65edf5c3..4b6ead4d 100644 --- a/lib/Onyx.ts +++ b/lib/Onyx.ts @@ -819,7 +819,7 @@ function setCollection(collectionKey: TKey const skippableCollectionMemberIDs = OnyxUtils.getSkippableCollectionMemberIDs(); if (skippableCollectionMemberIDs.size) { - resultCollection = Object.keys(resultCollection).reduce((result: OnyxInputKeyValueMapping, key) => { + resultCollection = newCollectionKeys.reduce((result: OnyxInputKeyValueMapping, key) => { try { const [, collectionMemberID] = OnyxUtils.splitCollectionMemberKey(key, collectionKey); // eslint-disable-next-line no-param-reassign diff --git a/tests/unit/onyxTest.ts b/tests/unit/onyxTest.ts index be816c98..44400e71 100644 --- a/tests/unit/onyxTest.ts +++ b/tests/unit/onyxTest.ts @@ -1967,7 +1967,6 @@ describe('Onyx', () => { expect(testKeyValue).toEqual({ [`${ONYX_KEYS.COLLECTION.TEST_KEY}entry1`]: {id: 'entry1_id', name: 'entry1_name'}, [`${ONYX_KEYS.COLLECTION.TEST_KEY}entry2`]: {id: 'entry2_id', name: 'entry2_name'}, - [`${ONYX_KEYS.COLLECTION.TEST_KEY}skippable-id`]: undefined, }); }); @@ -1991,7 +1990,6 @@ describe('Onyx', () => { expect(testKeyValue).toEqual({ [`${ONYX_KEYS.COLLECTION.TEST_KEY}entry1`]: {id: 'entry1_id', name: 'entry1_name'}, [`${ONYX_KEYS.COLLECTION.TEST_KEY}entry2`]: {id: 'entry2_id', name: 'entry2_name'}, - [`${ONYX_KEYS.COLLECTION.TEST_KEY}skippable-id`]: undefined, }); }); @@ -2015,7 +2013,6 @@ describe('Onyx', () => { expect(testKeyValue).toEqual({ [`${ONYX_KEYS.COLLECTION.TEST_KEY}entry1`]: {id: 'entry1_id', name: 'entry1_name'}, [`${ONYX_KEYS.COLLECTION.TEST_KEY}entry2`]: {id: 'entry2_id', name: 'entry2_name'}, - [`${ONYX_KEYS.COLLECTION.TEST_KEY}skippable-id`]: undefined, }); }); }); diff --git a/tests/unit/useOnyxTest.ts b/tests/unit/useOnyxTest.ts index 07bfc87f..92a7ffb5 100644 --- a/tests/unit/useOnyxTest.ts +++ b/tests/unit/useOnyxTest.ts @@ -675,7 +675,6 @@ describe('useOnyx', () => { expect(result.current[0]).toEqual({ [`${ONYXKEYS.COLLECTION.TEST_KEY}entry1`]: {id: 'entry1_id', name: 'entry1_name'}, [`${ONYXKEYS.COLLECTION.TEST_KEY}entry2`]: {id: 'entry2_id', name: 'entry2_name'}, - [`${ONYXKEYS.COLLECTION.TEST_KEY}skippable-id`]: undefined, }); expect(result.current[1].status).toEqual('loaded'); @@ -690,7 +689,6 @@ describe('useOnyx', () => { expect(result.current[0]).toEqual({ [`${ONYXKEYS.COLLECTION.TEST_KEY}entry1`]: {id: 'entry1_id', name: 'entry1_name_changed'}, [`${ONYXKEYS.COLLECTION.TEST_KEY}entry2`]: {id: 'entry2_id', name: 'entry2_name_changed'}, - [`${ONYXKEYS.COLLECTION.TEST_KEY}skippable-id`]: undefined, }); expect(result.current[1].status).toEqual('loaded'); }); From 5cc1cd5fba3975e00711f2cf23f30523b0dae734 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A1bio=20Henriques?= Date: Thu, 16 Jan 2025 07:38:58 +0000 Subject: [PATCH 10/11] Resolve TODOs --- API-INTERNAL.md | 8 ++++---- lib/OnyxUtils.ts | 5 +++-- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/API-INTERNAL.md b/API-INTERNAL.md index a66208fe..0dc8b5d8 100644 --- a/API-INTERNAL.md +++ b/API-INTERNAL.md @@ -21,10 +21,10 @@

Getter - returns the eviction block list.

getSkippableCollectionMemberIDs()
-

Getter - TODO

+

Getter - returns the skippable collection member IDs.

setSkippableCollectionMemberIDs()
-

Setter - TODO

+

Setter - sets the skippable collection member IDs.

initStoreValues(keys, initialKeyStates, safeEvictionKeys)

Sets the initial values for the Onyx store

@@ -197,13 +197,13 @@ Getter - returns the eviction block list. ## getSkippableCollectionMemberIDs() -Getter - TODO +Getter - returns the skippable collection member IDs. **Kind**: global function ## setSkippableCollectionMemberIDs() -Setter - TODO +Setter - sets the skippable collection member IDs. **Kind**: global function diff --git a/lib/OnyxUtils.ts b/lib/OnyxUtils.ts index d8569546..9cb147e2 100644 --- a/lib/OnyxUtils.ts +++ b/lib/OnyxUtils.ts @@ -88,6 +88,7 @@ let lastSubscriptionID = 0; // Connections can be made before `Onyx.init`. They would wait for this task before resolving const deferredInitTask = createDeferredTask(); +// Holds a set of collection member IDs which updates will be ignored when using Onyx methods. let skippableCollectionMemberIDs = new Set(); function getSnapshotKey(): OnyxKey | null { @@ -130,14 +131,14 @@ function getEvictionBlocklist(): Record { } /** - * Getter - TODO + * Getter - returns the skippable collection member IDs. */ function getSkippableCollectionMemberIDs(): Set { return skippableCollectionMemberIDs; } /** - * Setter - TODO + * Setter - sets the skippable collection member IDs. */ function setSkippableCollectionMemberIDs(ids: Set): void { skippableCollectionMemberIDs = ids; From ec33615e5948b2796c5d8f60c5eff8d339ac2844 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A1bio=20Henriques?= Date: Thu, 16 Jan 2025 09:42:08 +0000 Subject: [PATCH 11/11] Some polishing --- lib/Onyx.ts | 32 ++++++++++++++++++++------------ lib/OnyxUtils.ts | 6 ++++-- 2 files changed, 24 insertions(+), 14 deletions(-) diff --git a/lib/Onyx.ts b/lib/Onyx.ts index 66419aef..baf7c068 100644 --- a/lib/Onyx.ts +++ b/lib/Onyx.ts @@ -142,10 +142,12 @@ function set(key: TKey, value: OnyxSetInput): Promis try { const [, collectionMemberID] = OnyxUtils.splitCollectionMemberKey(key); if (skippableCollectionMemberIDs.has(collectionMemberID)) { - return Promise.resolve(); + // The key is a skippable one, so we set the new value to null. + // eslint-disable-next-line no-param-reassign + value = null; } } catch (e) { - // Key is not a collection one or something went wrong during split, so we proceed with the function's logic. + // The key is not a collection one or something went wrong during split, so we proceed with the function's logic. } } @@ -218,10 +220,11 @@ function multiSet(data: OnyxMultiSetInput): Promise { newData = Object.keys(newData).reduce((result: OnyxMultiSetInput, key) => { try { const [, collectionMemberID] = OnyxUtils.splitCollectionMemberKey(key); + // If the collection member key is a skippable one we set its value to null. // eslint-disable-next-line no-param-reassign result[key] = !skippableCollectionMemberIDs.has(collectionMemberID) ? newData[key] : null; } catch { - // Key is not a collection one or something went wrong during split, so we assign the data to result anyway. + // The key is not a collection one or something went wrong during split, so we assign the data to result anyway. // eslint-disable-next-line no-param-reassign result[key] = newData[key]; } @@ -271,11 +274,12 @@ function merge(key: TKey, changes: OnyxMergeInput): try { const [, collectionMemberID] = OnyxUtils.splitCollectionMemberKey(key); if (skippableCollectionMemberIDs.has(collectionMemberID)) { + // The key is a skippable one, so we set the new changes to undefined. // eslint-disable-next-line no-param-reassign changes = undefined; } } catch (e) { - // Key is not a collection one or something went wrong during split, so we proceed with the function's logic. + // The key is not a collection one or something went wrong during split, so we proceed with the function's logic. } } @@ -394,18 +398,19 @@ function mergeCollection(collectionKey: TK } let resultCollection: OnyxInputKeyValueMapping = collection; + let resultCollectionKeys = Object.keys(resultCollection); // Confirm all the collection keys belong to the same parent - const mergedCollectionKeys = Object.keys(resultCollection); - if (!OnyxUtils.doAllCollectionItemsBelongToSameParent(collectionKey, mergedCollectionKeys)) { + if (!OnyxUtils.doAllCollectionItemsBelongToSameParent(collectionKey, resultCollectionKeys)) { return Promise.resolve(); } const skippableCollectionMemberIDs = OnyxUtils.getSkippableCollectionMemberIDs(); if (skippableCollectionMemberIDs.size) { - resultCollection = Object.keys(resultCollection).reduce((result: OnyxInputKeyValueMapping, key) => { + resultCollection = resultCollectionKeys.reduce((result: OnyxInputKeyValueMapping, key) => { try { const [, collectionMemberID] = OnyxUtils.splitCollectionMemberKey(key, collectionKey); + // If the collection member key is a skippable one we set its value to null. // eslint-disable-next-line no-param-reassign result[key] = !skippableCollectionMemberIDs.has(collectionMemberID) ? resultCollection[key] : null; } catch { @@ -417,11 +422,12 @@ function mergeCollection(collectionKey: TK return result; }, {}); } + resultCollectionKeys = Object.keys(resultCollection); return OnyxUtils.getAllKeys() .then((persistedKeys) => { // Split to keys that exist in storage and keys that don't - const keys = mergedCollectionKeys.filter((key) => { + const keys = resultCollectionKeys.filter((key) => { if (resultCollection[key] === null) { OnyxUtils.remove(key); return false; @@ -814,19 +820,20 @@ function update(data: OnyxUpdate[]): Promise { */ function setCollection(collectionKey: TKey, collection: OnyxMergeCollectionInput): Promise { let resultCollection: OnyxInputKeyValueMapping = collection; + let resultCollectionKeys = Object.keys(resultCollection); // Confirm all the collection keys belong to the same parent - const newCollectionKeys = Object.keys(resultCollection); - if (!OnyxUtils.doAllCollectionItemsBelongToSameParent(collectionKey, newCollectionKeys)) { + if (!OnyxUtils.doAllCollectionItemsBelongToSameParent(collectionKey, resultCollectionKeys)) { Logger.logAlert(`setCollection called with keys that do not belong to the same parent ${collectionKey}. Skipping this update.`); return Promise.resolve(); } const skippableCollectionMemberIDs = OnyxUtils.getSkippableCollectionMemberIDs(); if (skippableCollectionMemberIDs.size) { - resultCollection = newCollectionKeys.reduce((result: OnyxInputKeyValueMapping, key) => { + resultCollection = resultCollectionKeys.reduce((result: OnyxInputKeyValueMapping, key) => { try { const [, collectionMemberID] = OnyxUtils.splitCollectionMemberKey(key, collectionKey); + // If the collection member key is a skippable one we set its value to null. // eslint-disable-next-line no-param-reassign result[key] = !skippableCollectionMemberIDs.has(collectionMemberID) ? resultCollection[key] : null; } catch { @@ -838,6 +845,7 @@ function setCollection(collectionKey: TKey return result; }, {}); } + resultCollectionKeys = Object.keys(resultCollection); return OnyxUtils.getAllKeys().then((persistedKeys) => { const mutableCollection: OnyxInputKeyValueMapping = {...resultCollection}; @@ -846,7 +854,7 @@ function setCollection(collectionKey: TKey if (!key.startsWith(collectionKey)) { return; } - if (newCollectionKeys.includes(key)) { + if (resultCollectionKeys.includes(key)) { return; } diff --git a/lib/OnyxUtils.ts b/lib/OnyxUtils.ts index 9cb147e2..a3ef656a 100644 --- a/lib/OnyxUtils.ts +++ b/lib/OnyxUtils.ts @@ -278,11 +278,12 @@ function get>(key: TKey): P try { const [, collectionMemberID] = splitCollectionMemberKey(key); if (skippableCollectionMemberIDs.has(collectionMemberID)) { + // The key is a skippable one, so we set the value to undefined. // eslint-disable-next-line no-param-reassign val = undefined as OnyxValue; } } catch (e) { - // Key is not a collection one or something went wrong during split, so we proceed with the function's logic. + // The key is not a collection one or something went wrong during split, so we proceed with the function's logic. } } @@ -368,10 +369,11 @@ function multiGet(keys: CollectionKeyBase[]): Promise