Skip to content
This repository has been archived by the owner on Nov 10, 2023. It is now read-only.

Allow async storage for cache provider #7

Merged
merged 4 commits into from
Jun 25, 2018

Conversation

jxom
Copy link
Owner

@jxom jxom commented Jun 25, 2018

This allows get and set functions in the cacheProvider to be async (return a promise).

Copy link
Collaborator

@daveols daveols left a comment

Choose a reason for hiding this comment

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

Nice use of the Component component!

You may have missed an edge case, see my comment. You should also update docs.

src/context.js Outdated
if (cacheProvider && cacheProvider.get) {
const cacheResponse = cacheProvider.get(cacheKey);
if (cacheResponse instanceof Promise) {
return cacheResponse.then(cachedData => setState({ cachedData, hasLoaded: true }));
Copy link
Collaborator

Choose a reason for hiding this comment

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

If the provided promise is rejected then setState never gets called here, leading hasLoaded to stay false and nothing gets rendered (deadlocked?).

Copy link
Owner Author

Choose a reason for hiding this comment

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

sad times

src/context.js Outdated
@@ -77,7 +80,10 @@ class LoadsConsumer extends React.Component<ConsumerProps> {
if (cacheProvider && cacheProvider.get) {
const cacheResponse = cacheProvider.get(cacheKey);
if (cacheResponse instanceof Promise) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This needs to be more generic to detect 3rd party promise implementations

@jxom jxom merged commit a9e976b into master Jun 25, 2018
@jxom jxom deleted the enhancement/allow-async-cache-provider branch June 25, 2018 07:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants