Skip to content

feat: Key Value cache implementation for React Native #426

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

Merged
merged 10 commits into from
Mar 20, 2020

Conversation

zashraf1985
Copy link
Contributor

@zashraf1985 zashraf1985 commented Mar 12, 2020

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

@coveralls
Copy link

coveralls commented Mar 12, 2020

Coverage Status

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

* 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>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

let finalVal = val;
if (val) {
try {
finalVal = JSON.parse(val.valueOf());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment about constructing a promise

@zashraf1985 zashraf1985 removed their assignment Mar 18, 2020
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"?

* 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>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Comment on lines 27 to 40
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)
})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

})

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

contains(key: string): Promise<Boolean> {
return new Promise((resolve, reject) =>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment about constructing a promise.

*/
export default interface PersistentKeyValueCache {

/**
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just added more details to each method.

@zashraf1985 zashraf1985 removed their assignment Mar 20, 2020
Copy link
Contributor

@mjc1283 mjc1283 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
  }
});

- <<: *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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

})

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@mjc1283 mjc1283 merged commit 08deb5c into master Mar 20, 2020
@mjc1283 mjc1283 deleted the zeeshan/datafile-manager-key-val-cache branch March 20, 2020 18:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants