Reject nullable value from cache #2
Conversation
nicolashemonic
commented
Sep 7, 2018
nicolashemonic
commented
- Get cache resolve promise only when value is not null
- Remove new keyword from Promise.reject usage
- Get cache resolve promise only when value is not null - Remove new keyword from Promise.reject usage
| if (item) { | ||
| return Promise.resolve(JSON.parse(item)); | ||
| } | ||
| return Promise.reject(null); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
refact(config): remove jest.config
2 participants
