-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Custom cache provider #1017
Custom cache provider #1017
Conversation
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit e8c21fa:
|
9f68e6d
to
5395cd2
Compare
Might it be better to use the Cache API? it persists the data to apps installed locally on mobile devices, and it works inside WebWorkers https://developer.mozilla.org/en-US/docs/Web/API/Cache https://caniuse.com/?search=cache also, what about customizing the "get" in addition to the "set" ? usage examples in readme would be helpful to adopt this |
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.
Generally LGTM! 👍
Since this is breaking and deprecates Cache
, we need to first prepare a PR to add @deprecates
to Cache
for 0.x versions.
Secondly since we don't need to expose Cache
anymore, I think we shouldn't wrap the provider with a class. We can just use a couple of lightweight functions:
function get(provider, key) { return provider.get(key) }
function set(provider, key, value) { return provider.set(key, value) }
And we don't need those internal listeners too. This change will be useful when we pass the provider down via context, and we don't maintain the cache
singleton itself inside SWR.
5834b59 latest change: limit cache API usage inside swr, only using set/get/delete, and they're all sync APIs. for global |
I refactored the API a little bit: const { cache, mutate } = createCache(provider) But the implementation is still not ideal:
I need to spend some more time thinking about it. I also moved web presets out of the default config object, because these can not be options. But I'm not very sure about that decision, let me know what you think! |
While the main goal of this PR is to better cover testing scenarios, there're still a couple of to-dos left (no blocking this PR to be merged, just writing them down):
|
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.
@shuding new changes genterally LGTM! you might also want to fix those lint warnings.
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.
🎉🎉🎉
Changes
This PR is a early phase implementation for #728 . Add
cache
property as a cache provider to SWR contextThis is the early support of #16 , user can have a way to access the cache state and changes ( #630 ), but won't effect swr internal cache. For example, you can use any store like localStorage / RN AsyncStorage, swr internal will still use a sync cache to guarantee the correct state updating sequence.
In the long term, we can consider to deprecate the exposed cache
Example