Merge pull request #493 from callstack-internal/audit/app-startup/avo…

…id-creating-new-array-in-getAllKeys

[Audit][Implementation] - avoid re-creating array each time getAllKeys is called

lib/Onyx.js

@@ -167,12 +167,12 @@ function get(key) {
 /**
  * Returns current key names stored in persisted storage
  * @private
- * @returns {Promise<string[]>}
+ * @returns {Promise<Set<Key>>}
  */
 function getAllKeys() {
     // When we've already read stored keys, resolve right away
     const storedKeys = cache.getAllKeys();
-    if (storedKeys.length > 0) {
+    if (storedKeys.size > 0) {
         return Promise.resolve(storedKeys);
     }
 
@@ -186,7 +186,8 @@ function getAllKeys() {
     // Otherwise retrieve the keys from storage and capture a promise to aid concurrent usages
     const promise = Storage.getAllKeys().then((keys) => {
         cache.setAllKeys(keys);
-        return keys;
+        // return the updated set of keys
+        return cache.getAllKeys();
     });
 
     return cache.captureTask(taskName, promise);
@@ -268,10 +269,18 @@ function tryGetCachedValue(key, mapping = {}) {
 
         // It is possible we haven't loaded all keys yet so we do not know if the
         // collection actually exists.
-        if (allCacheKeys.length === 0) {
+        if (allCacheKeys.size === 0) {
             return;
         }
-        const matchingKeys = _.filter(allCacheKeys, (k) => k.startsWith(key));
+
+        const matchingKeys = [];
+        allCacheKeys.forEach((k) => {
+            if (!k.startsWith(key)) {
+                return;
+            }
+            matchingKeys.push(k);
+        });
+
         const values = _.reduce(
             matchingKeys,
             (finalObject, matchedKey) => {
@@ -374,7 +383,7 @@ function addToEvictionBlockList(key, connectionID) {
 function addAllSafeEvictionKeysToRecentlyAccessedList() {
     return getAllKeys().then((keys) => {
         _.each(evictionAllowList, (safeEvictionKey) => {
-            _.each(keys, (key) => {
+            keys.forEach((key) => {
                 if (!isKeyMatch(safeEvictionKey, key)) {
                     return;
                 }
@@ -390,8 +399,13 @@ function addAllSafeEvictionKeysToRecentlyAccessedList() {
  * @returns {Object}
  */
 function getCachedCollection(collectionKey) {
-    const collectionMemberKeys = _.filter(cache.getAllKeys(), (storedKey) => isCollectionMemberKey(collectionKey, storedKey));
-
+    const collectionMemberKeys = [];
+    cache.getAllKeys().forEach((storedKey) => {
+        if (!isCollectionMemberKey(collectionKey, storedKey)) {
+            return;
+        }
+        collectionMemberKeys.push(storedKey);
+    });
     return _.reduce(
         collectionMemberKeys,
         (prev, curr) => {
@@ -915,7 +929,14 @@ function connect(mapping) {
             // We search all the keys in storage to see if any are a "match" for the subscriber we are connecting so that we
             // can send data back to the subscriber. Note that multiple keys can match as a subscriber could either be
             // subscribed to a "collection key" or a single key.
-            const matchingKeys = _.filter(keys, (key) => isKeyMatch(mapping.key, key));
+
+            const matchingKeys = [];
+            keys.forEach((key) => {
+                if (!isKeyMatch(mapping.key, key)) {
+                    return;
+                }
+                matchingKeys.push(key);
+            });
 
             // If the key being connected to does not exist we initialize the value with null. For subscribers that connected
             // directly via connect() they will simply get a null value sent to them without any information about which key matched
@@ -1395,7 +1416,7 @@ function clear(keysToPreserve = []) {
         //      status, or activeClients need to remain in Onyx even when signed out)
         // 2. Any keys with a default state (because they need to remain in Onyx as their default, and setting them
         //      to null would cause unknown behavior)
-        _.each(keys, (key) => {
+        keys.forEach((key) => {
             const isKeyToPreserve = _.contains(keysToPreserve, key);
             const isDefaultKey = _.has(defaultKeyStates, key);
 
@@ -1502,7 +1523,7 @@ function mergeCollection(collectionKey, collection) {
                 return true;
             })
             .keys()
-            .partition((key) => persistedKeys.includes(key))
+            .partition((key) => persistedKeys.has(key))
             .value();
 
         const existingKeyCollection = _.pick(collection, existingKeys);

lib/OnyxCache.ts

@@ -54,8 +54,8 @@ class OnyxCache {
     }
 
     /** Get all the storage keys */
-    getAllKeys(): Key[] {
-        return Array.from(this.storageKeys);
+    getAllKeys(): Set<Key> {
+        return this.storageKeys;
     }
 
     /**

tests/unit/onyxCacheTest.js

@@ -26,7 +26,7 @@ describe('Onyx', () => {
                 const allKeys = cache.getAllKeys();
 
                 // Then the result should be empty
-                expect(allKeys).toEqual([]);
+                expect(allKeys).toEqual(new Set());
             });
 
             it('Should keep storage keys', () => {
@@ -37,7 +37,7 @@ describe('Onyx', () => {
 
                 // Then the keys should be stored in cache
                 const allKeys = cache.getAllKeys();
-                expect(allKeys).toEqual(['mockKey', 'mockKey2', 'mockKey3']);
+                expect(allKeys).toEqual(new Set(['mockKey', 'mockKey2', 'mockKey3']));
             });
 
             it('Should keep storage keys even when no values are provided', () => {
@@ -48,7 +48,7 @@ describe('Onyx', () => {
 
                 // Then the keys should be stored in cache
                 const allKeys = cache.getAllKeys();
-                expect(allKeys).toEqual(['mockKey', 'mockKey2', 'mockKey3']);
+                expect(allKeys).toEqual(new Set(['mockKey', 'mockKey2', 'mockKey3']));
             });
 
             it('Should not store duplicate keys', () => {
@@ -62,7 +62,7 @@ describe('Onyx', () => {
 
                 // Then getAllKeys should not include a duplicate value
                 const allKeys = cache.getAllKeys();
-                expect(allKeys).toEqual(['mockKey', 'mockKey2', 'mockKey3']);
+                expect(allKeys).toEqual(new Set(['mockKey', 'mockKey2', 'mockKey3']));
             });
         });
 
@@ -121,7 +121,7 @@ describe('Onyx', () => {
                 expect(cache.hasCacheForKey('mockKey')).toBe(false);
 
                 // Then but a key should be available
-                expect(cache.getAllKeys()).toEqual(expect.arrayContaining(['mockKey']));
+                expect(cache.getAllKeys()).toEqual(new Set(['mockKey']));
             });
 
             it('Should not make duplicate keys', () => {
@@ -135,7 +135,7 @@ describe('Onyx', () => {
 
                 // Then getAllKeys should not include a duplicate value
                 const allKeys = cache.getAllKeys();
-                expect(allKeys).toEqual(['mockKey', 'mockKey2']);
+                expect(allKeys).toEqual(new Set(['mockKey', 'mockKey2']));
             });
         });
 
@@ -158,7 +158,7 @@ describe('Onyx', () => {
                 cache.set('mockKey', {value: 'mockValue'});
 
                 // Then but a key should be available
-                expect(cache.getAllKeys()).toEqual(expect.arrayContaining(['mockKey']));
+                expect(cache.getAllKeys()).toEqual(new Set(['mockKey']));
             });
 
             it('Should overwrite existing cache items for the Given key', () => {
@@ -186,7 +186,7 @@ describe('Onyx', () => {
                 // Then a value should not be available in cache
                 expect(cache.hasCacheForKey('mockKey')).toBe(false);
                 expect(cache.getValue('mockKey')).not.toBeDefined();
-                expect(cache.getAllKeys('mockKey').includes('mockKey')).toBe(false);
+                expect(cache.getAllKeys('mockKey').has('mockKey')).toBe(false);
             });
         });
 
@@ -336,7 +336,7 @@ describe('Onyx', () => {
                 });
 
                 // Then getAllStorage keys should return updated storage keys
-                expect(cache.getAllKeys()).toEqual(['mockKey', 'mockKey2', 'mockKey3', 'mockKey4']);
+                expect(cache.getAllKeys()).toEqual(new Set(['mockKey', 'mockKey2', 'mockKey3', 'mockKey4']));
             });
 
             it('Should throw if called with anything that is not an object', () => {

tests/unit/onyxTest.js

@@ -40,18 +40,18 @@ describe('Onyx', () => {
         Onyx.set(ONYX_KEYS.OTHER_TEST, 42)
             .then(() => Onyx.getAllKeys())
             .then((keys) => {
-                expect(keys.includes(ONYX_KEYS.OTHER_TEST)).toBe(true);
+                expect(keys.has(ONYX_KEYS.OTHER_TEST)).toBe(true);
                 return Onyx.set(ONYX_KEYS.OTHER_TEST, null);
             })
             .then(() => {
                 // Checks if cache value is removed.
-                expect(cache.getAllKeys().length).toBe(0);
+                expect(cache.getAllKeys().size).toBe(0);
 
                 // When cache keys length is 0, we fetch the keys from storage.
                 return Onyx.getAllKeys();
             })
             .then((keys) => {
-                expect(keys.includes(ONYX_KEYS.OTHER_TEST)).toBe(false);
+                expect(keys.has(ONYX_KEYS.OTHER_TEST)).toBe(false);
             }));
 
     it('should set a simple key', () => {