Skip to content
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

Add configurable cache storage adapter #17

Closed
wants to merge 3 commits into from

Conversation

wschurman
Copy link

Similar to #16, though allows user configuration of the storage adapter (still uses localStorage by default). Should be a straightforward rebase with #16 should we merge that below or on top of this.

Making this configurable allows for use in environments without localStorage.

react-native example:

import {
  AsyncStorage,
} from 'react-native';

export default class AsyncStorageCacheStorageAdapter {
  getItem(key: string, callback: (error: any, value: any) => void): void {
    AsyncStorage.getItem(key).then((val) => {
      callback(null, val);
    });
  }

  setItem(key: string, data: string): void {
    AsyncStorage.setItem(key, data);
  }

  removeItem(key: string): void {
    AsyncStorage.removeItem(key);
  }
}

Additional changes:

Definitely open for comments and additional suggestions on the feature.

@brad-decker
Copy link
Contributor

Hey @wschurman - thanks for the PR. I'm gonna review this but i'll need to consult with @aweary who has a great deal more experience with this repo.

@aweary aweary self-assigned this Jan 28, 2017
@wschurman
Copy link
Author

Upon further inspection and use in our app, it looks like having these async writes can cause race conditions when writing to a single storage key. I'm going to do a bit more investigation into distributing the records into their own cache keys or implement a reader-writer lock on the cache to keep it consistent.

I think we can close this in the meantime. I put up another PR if the solution is general enough for inclusion in this repo.

@wschurman wschurman closed this Jan 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants