feat: Key Value cache implementation for React Native #426

zashraf1985 · 2020-03-12T20:36:18Z

Summary

This is a Key Value Pair cache implementation for react native. This will be used when developing a Separate Datafile Manager for React native which will support datafile caching.

Test plan

Added Unit Tests

…react native async storage

coveralls · 2020-03-12T20:47:53Z

Coverage remained the same at 97.222% when pulling 0ad00f6 on zeeshan/datafile-manager-key-val-cache into 1e00509 on master.

mjc1283 · 2020-03-13T16:03:10Z

packages/datafile-manager/src/persistentKeyValueCache.ts

+   * 2. Object if value found was stored as a JSON Object.
+   * 3. null if the key does not exist in the cache.
+   */
+  get(key: string): Promise<string | Object | null>


Why does it need to support storing both strings and objects?

I was trying to make it a bit generic so that someone can use it to store string values too if required. But i can change it to object only for now.

mjc1283 · 2020-03-13T16:05:26Z

packages/datafile-manager/src/reactNativeAsyncStorageCache.ts

+export default class ReactNativeAsyncStorageCache implements PersistentKeyValueCache {
+  // If value is a valid JSON string, it converts it to object before returning
+  get(key: string): Promise<Object | string | null> {
+    return new Promise((resolve, reject) => {


I don't think we need to construct a new promise. We can use the one returned from AsyncStorage.getItem.

mjc1283 · 2020-03-13T16:14:57Z

packages/datafile-manager/src/reactNativeAsyncStorageCache.ts

+        let finalVal = val;
+        if (val) {
+          try {
+            finalVal = JSON.parse(val.valueOf());


Do we need to parse the value here? Datafile manager already does this.

mjc1283 · 2020-03-13T16:15:55Z

packages/datafile-manager/src/reactNativeAsyncStorageCache.ts

+          try {
+            finalVal = JSON.parse(val.valueOf());
+          } catch (ex) {
+            // couldnt parse, its a string


I am wary of catching and ignoring exceptions. Almost always, we should be logging an error.

mjc1283 · 2020-03-13T16:16:30Z

packages/datafile-manager/src/reactNativeAsyncStorageCache.ts

+
+  // If value is an Object, it stringifies it before storing.
+  set(key: string, val: string | Object): Promise<void> {
+    return new Promise((resolve, reject) => {


Same comment about constructing a promise

mjc1283 · 2020-03-18T22:18:00Z

packages/datafile-manager/__mocks__/@react-native-community/async-storage.ts

+  }
+
+  static setItem(key: string, value: string, callback?: (error?: Error) => void): Promise<void> {
+    return new Promise(resolve => resolve())


Suggested change

return new Promise(resolve => resolve())

return Promise.resolve();

mjc1283 · 2020-03-18T22:20:09Z

packages/datafile-manager/package.json

@@ -38,6 +38,9 @@
    "@optimizely/js-sdk-logging": "^0.1.0",
    "@optimizely/js-sdk-utils": "^0.1.0"
  },
+  "peerDependencies": {
+    "@react-native-community/async-storage": "^1.8.1"


Should we do something very general like "1.8.x"? Depending on what features of this we use, can we even get away with "1.x.x"?

mjc1283 · 2020-03-18T22:26:04Z

packages/datafile-manager/src/persistentKeyValueCache.ts

+   * 1. Object if value found was stored as a JSON Object.
+   * 2. null if the key does not exist in the cache.
+   */
+  get(key: string): Promise<Object | null>


It's not recommended to use Object as a type in TypeScript (see: https://www.typescriptlang.org/docs/handbook/declaration-files/do-s-and-don-ts.html#general-types)

mjc1283 · 2020-03-18T22:34:15Z

packages/datafile-manager/src/reactNativeAsyncStorageCache.ts

+    return new Promise((resolve, reject) => {
+      AsyncStorage.getItem(key).then((val: string | null) => {
+        if (val) {
+          try {
+            resolve(JSON.parse(val))
+          } catch (ex) {
+            logger.error('Error Parsing Object from cache - %s', ex)
+            reject(ex)
+          }
+        } else {
+          resolve(null)
+        }
+      }).catch(reject)
+    })


I still don't think constructing promise is necessary, better to chain off the getItem promise. I think this behaves the same:

Suggested change

return new Promise((resolve, reject) => {

AsyncStorage.getItem(key).then((val: string | null) => {

if (val) {

try {

resolve(JSON.parse(val))

} catch (ex) {

logger.error('Error Parsing Object from cache - %s', ex)

reject(ex)

}

} else {

resolve(null)

}

}).catch(reject)

})

return AsyncStorage.getItem(key).then((val: string | null) => {

if (val) {

return JSON.parse(val);

}

return null;

}).catch((ex) => {

logger.error('Error Parsing Object from cache - %s', ex);

throw ex;

});

Usually it is best to chain off existing promises, instead of creating them.

mjc1283 · 2020-03-18T22:35:38Z

packages/datafile-manager/__test__/reactNativeAsyncStorageCache.spec.ts

+  })
+
+  describe('get', function() { 
+    it('should return correct object when item is found in cache', function(done) {


Instead of using a done argument, I believe you can return a promise from your test method.

mjc1283 · 2020-03-18T22:44:06Z

packages/datafile-manager/src/reactNativeAsyncStorageCache.ts

+  }
+
+  contains(key: string): Promise<Boolean> {
+    return new Promise((resolve, reject) =>


Same comment about constructing a promise.

mjc1283 · 2020-03-18T22:45:02Z

packages/datafile-manager/src/persistentKeyValueCache.ts

+ */
+export default interface PersistentKeyValueCache {
+
+  /**


For all methods in this interface, the docs need more detail about the returned promise, what value it is fulfilled with, and what causes the promise to reject.

I just added more details to each method.

mjc1283

LGTM! Nice work! Just one suggestion about the get method.

mjc1283 · 2020-03-20T16:05:54Z

packages/datafile-manager/src/reactNativeAsyncStorageCache.ts

+      return AsyncStorage.getItem(key)
+        .then((val: string | null) => val ? JSON.parse(val) : null)
+    } catch (ex) {
+      logger.error('Error Parsing Object from cache - %s', ex)


We won't reach this line if there's an error thrown by JSON.parse on line 29. If you want to log an error upon JSON parse error, the try/catch block should be located inside the callback passed to then:

AsyncStorage.getItem(key).then((val: string | null) => { if (!val) { return null; } try { return JSON.parse(val); } catch (ex) { logger.error('Error ...'); throw ex; } });

mjc1283 · 2020-03-20T16:11:48Z

.travis.yml

    - <<: *packagetest
      before_install: cd packages/event-processor
    - <<: *packagetest
      before_install: cd packages/logging
    - <<: *packagetest
-      before_install: cd packages/utils
+      before_script: npm install "@react-native-community/async-storage"


Is this necessary? The tests should not actually call into AsyncStorage - it should be mocked.

Yes but typescript compiles first and that needs this dependency. It eventually uses mock

mjc1283 · 2020-03-20T16:17:26Z

packages/datafile-manager/__test__/reactNativeAsyncStorageCache.spec.ts

+    })
+
+    it('should reject promise error if string has an incorrect JSON format', function() {
+      return cacheInstance.get('keyWithInvalidJsonObject').catch(() => {})


I think this test will pass even if the promise does not reject. We need to do something in the catch callback, like set a flag to true, then check it after. Same goes for the other test that begins with should reject...

Made changes accordingly. Chained a then after the catch which checks for a value that is only available if it has been chained through catch.

zashraf1985 added 2 commits March 12, 2020 12:25

Added an interface for key val cache and added an implementation for …

2eef25c

…react native async storage

Added promise rejection handling

9cfda12

zashraf1985 requested a review from a team as a code owner March 12, 2020 20:36

optimizely-codereview-bot assigned zashraf1985 Mar 12, 2020

mjc1283 reviewed Mar 13, 2020

View reviewed changes

zashraf1985 added 4 commits March 17, 2020 23:34

Made some simplifications

7aca2e2

made changes to some comments

8d36c5b

Added unit tests

1b97abe

changed a value

310f181

zashraf1985 removed their assignment Mar 18, 2020

optimizely-codereview-bot requested a review from mjc1283 March 18, 2020 22:04

mjc1283 suggested changes Mar 18, 2020

View reviewed changes

optimizely-codereview-bot assigned zashraf1985 Mar 18, 2020

zashraf1985 added 3 commits March 19, 2020 15:46

Incorporated review changes

a452023

Incorporate some more feedback

8a8687f

Change async storage peer dependency version to the minimum that works

d91afff

zashraf1985 removed their assignment Mar 20, 2020

optimizely-codereview-bot requested a review from mjc1283 March 20, 2020 04:12

mjc1283 approved these changes Mar 20, 2020

View reviewed changes

Incorporated more review feedback.

0ad00f6

mjc1283 merged commit 08deb5c into master Mar 20, 2020

mjc1283 deleted the zeeshan/datafile-manager-key-val-cache branch March 20, 2020 18:46

	return new Promise(resolve => resolve())
	return Promise.resolve();

feat: Key Value cache implementation for React Native #426

feat: Key Value cache implementation for React Native #426

Uh oh!

Conversation

zashraf1985 commented Mar 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Uh oh!

coveralls commented Mar 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjc1283 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zashraf1985 commented Mar 12, 2020 •

edited

Loading

coveralls commented Mar 12, 2020 •

edited

Loading