Formed in 2009, the Archive Team (not to be confused with the archive.org Archive-It Team) is a rogue archivist collective dedicated to saving copies of rapidly dying or deleted websites for the sake of history and digital heritage. The group is 100% composed of volunteers and interested parties, and has expanded into a large amount of related projects for saving online and digital history.
Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upfeat: Key Value cache implementation for React Native #426
Conversation
coveralls
commented
Mar 12, 2020
•
| * 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> |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
zashraf1985
Mar 13, 2020
Author
Contributor
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) => { |
This comment has been minimized.
This comment has been minimized.
mjc1283
Mar 13, 2020
Contributor
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()); |
This comment has been minimized.
This comment has been minimized.
mjc1283
Mar 13, 2020
Contributor
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 |
This comment has been minimized.
This comment has been minimized.
mjc1283
Mar 13, 2020
Contributor
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) => { |
This comment has been minimized.
This comment has been minimized.
| } | ||
|
|
||
| static setItem(key: string, value: string, callback?: (error?: Error) => void): Promise<void> { | ||
| return new Promise(resolve => resolve()) |
This comment has been minimized.
This comment has been minimized.
mjc1283
Mar 18, 2020
Contributor
| 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" |
This comment has been minimized.
This comment has been minimized.
mjc1283
Mar 18, 2020
Contributor
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> |
This comment has been minimized.
This comment has been minimized.
mjc1283
Mar 18, 2020
Contributor
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) | ||
| }) |
This comment has been minimized.
This comment has been minimized.
mjc1283
Mar 18, 2020
Contributor
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) { |
This comment has been minimized.
This comment has been minimized.
mjc1283
Mar 18, 2020
Contributor
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) => |
This comment has been minimized.
This comment has been minimized.
| */ | ||
| export default interface PersistentKeyValueCache { | ||
|
|
||
| /** |
This comment has been minimized.
This comment has been minimized.
mjc1283
Mar 18, 2020
Contributor
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.
This comment has been minimized.
This comment has been minimized.
|
LGTM! Nice work! Just one suggestion about the |
| return AsyncStorage.getItem(key) | ||
| .then((val: string | null) => val ? JSON.parse(val) : null) | ||
| } catch (ex) { | ||
| logger.error('Error Parsing Object from cache - %s', ex) |
This comment has been minimized.
This comment has been minimized.
mjc1283
Mar 20, 2020
Contributor
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" |
This comment has been minimized.
This comment has been minimized.
mjc1283
Mar 20, 2020
Contributor
Is this necessary? The tests should not actually call into AsyncStorage - it should be mocked.
This comment has been minimized.
This comment has been minimized.
zashraf1985
Mar 20, 2020
Author
Contributor
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(() => {}) |
This comment has been minimized.
This comment has been minimized.
mjc1283
Mar 20, 2020
Contributor
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...
This comment has been minimized.
This comment has been minimized.
zashraf1985
Mar 20, 2020
Author
Contributor
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 commentedMar 12, 2020
•
edited
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