-
Notifications
You must be signed in to change notification settings - Fork 83
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
Conversation
…react native async storage
* 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> |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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) => { |
There was a problem hiding this comment.
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()); |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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) => { |
There was a problem hiding this comment.
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
} | ||
|
||
static setItem(key: string, value: string, callback?: (error?: Error) => void): Promise<void> { | ||
return new Promise(resolve => resolve()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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" |
There was a problem hiding this comment.
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> |
There was a problem hiding this comment.
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)
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) | ||
}) |
There was a problem hiding this comment.
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:
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) { |
There was a problem hiding this comment.
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) => |
There was a problem hiding this comment.
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 { | ||
|
||
/** |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this 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) |
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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(() => {}) |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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
.
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