-
Notifications
You must be signed in to change notification settings - Fork 13
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
Use localForage #16
base: master
Are you sure you want to change the base?
Use localForage #16
Conversation
lgtm 👍 I'm gonna wait for @aweary to jump on this as well. |
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.
Thanks for the PR! Just a few changes so we're using the localForage
API correctly 👍
@@ -20,9 +22,8 @@ export default class CacheWriter { | |||
constructor(options: CacheWriterOptions = {}) { | |||
this.cacheKey = options.cacheKey || DEFAULT_CACHE_KEY | |||
try { | |||
let localCache = localStorage.getItem(this.cacheKey); | |||
let localCache = localForage.getItem(this.cacheKey); |
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.
localForage
is async, so it's not a drop in replacement for localStorage
. Lets go ahead and use their Promise API.
localForage.getItem(this.cacheKey)
.then(localCache => {
if (!localCache) {
this.cache = new CacheRecordStore();
} else {
this.cache = CacheRecordStore.fromJSON(localCache)
}
})
.catch(err => this.cache = new CacheRecordStore())
@@ -33,7 +34,7 @@ export default class CacheWriter { | |||
} | |||
|
|||
clearStorage() { | |||
localStorage.removeItem(this.cacheKey); | |||
localForage.removeItem(this.cacheKey); |
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 here, lets use the Promise API to reset this.cache
after this key has been removed.
@@ -53,8 +54,7 @@ export default class CacheWriter { | |||
record[field] = value; | |||
this.cache.records[dataId] = record; | |||
try { | |||
const serialized = JSON.stringify(this.cache); | |||
localStorage.setItem(this.cacheKey, serialized); | |||
localForage.setItem(this.cacheKey, this.cache); |
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 can move this out of the try
/ catch
since its async
@@ -20,9 +22,8 @@ export default class CacheWriter { | |||
constructor(options: CacheWriterOptions = {}) { | |||
this.cacheKey = options.cacheKey || DEFAULT_CACHE_KEY |
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.
You need to call localforage.config
before using it, so lets let users pass in their own config for this, and default to an empty object.
localForage.config(options.localForage || {});
Thanks for the feedback @aweary, pushed a commit addressing your points 👍 let me know if it looks good |
Hey @aweary any update on this? |
Addresses #1 and #15 by using
localForage
instead oflocalStorage
.Also updated the
relay-starter-kit
example to latest and the usage of the cache manager to reflect the exposedinjectCacheManager
method on theRelayEnvironment
(cf. facebook/relay#1320)