Skip to content

Reject nullable value from cache #2

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.

Sign up for GitHub

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

Open
nicolashemonic wants to merge 1 commit into
base: master
Choose a base branch
from
+6 −3
Open

Reject nullable value from cache #2

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,13 @@ const CacheStore = {
return AsyncStorage.getItem(exprKey).then((expiry) => {
if (expiry && currentTime() >= parseInt(expiry, 10)){
AsyncStorage.multiRemove([exprKey, theKey]);
return new Promise.reject(null);
return Promise.reject(null);
}
return AsyncStorage.getItem(theKey).then((item) => {
return Promise.resolve(JSON.parse(item));
if (item) {
return Promise.resolve(JSON.parse(item));
}
return Promise.reject(null);
Copy link
Owner

@cheeaun cheeaun Sep 8, 2018

Choose a reason for hiding this comment

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

Hmm, still don't quite get this part. If the item value is null, it's still a valid value. I think Promise.reject should only occur when AsyncStorage has issues/problems retrieving the value, as how I understand it from the doc https://facebook.github.io/react-native/docs/asyncstorage

Copy link
Author

@nicolashemonic nicolashemonic Sep 8, 2018

Choose a reason for hiding this comment

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

I agree. I have submit this correction regarding the logic in your code.

According the AsyncStorage documentation Promise is rejected only when a technical error occurred. Your implementation of Cache do not follow this rule because you reject the Promise when the cached value has expired but it's not an error.

I think, you should reject Promise only when a technical error occurred not when the cache has expired. I expect from get to resolve the cached value or null if expired and reject an error message when an error occurred. It's also true for others method like isExpired which reject the Promise when the value is not expired. I expect from isExpired to resolve true or false and reject an error message when an error occurred.

Copy link
Owner

@cheeaun cheeaun Sep 10, 2018

Choose a reason for hiding this comment

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

Oh! 😳 You're right. The isExpired method that rejects Promise if false, is actually a mistake I made 😆 (realised it few days after releasing this project). I was planning to fix it but never get to it (lazy) as I'm not sure if anyone else is using this RN module yet.

I'm planning to change how isExpired method works but will need to bump up a major version.

So for this PR, I think we'll stick with the correct way first. And when I have the time, will change isExpired method later.

});
});
},
Expand All @@ -48,7 +51,7 @@ const CacheStore = {
const exprKey = CACHE_EXPIRATION_PREFIX + key;
return AsyncStorage.getItem(exprKey).then((expiry) => {
var expired = expiry && currentTime() >= parseInt(expiry, 10);
return expired ? Promise.resolve() : new Promise.reject(null);
return expired ? Promise.resolve() : Promise.reject(null);
});
},

Expand Down