Skip to content

[RFC] Async Storage v2 #113

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

Closed
krizzu opened this issue May 28, 2019 · 36 comments
Closed

[RFC] Async Storage v2 #113

krizzu opened this issue May 28, 2019 · 36 comments
Assignees
Labels
enhancement New feature or request

Comments

@krizzu
Copy link
Member

krizzu commented May 28, 2019

Async Storage v2

Motivation

A few months back, due to the lean core effort, Async Storage was moved to a standalone repo, making it more open for bug fixes and feature implementations. By adding semantic-versioning, we've made sure that releases happen more often, keeping the library in a good shape.

One of the discussed topics among the community was adding support for out of the tree platforms, so in a case where you have multi-platform project (say, Web app and RN app, sharing components and/or business logic), you don't have to use two different libraries for persisting data.

In a new major release, I'd like to introduce a concept of replaceable storage backends, making Async Storage more open not only to other platforms but also to other storage implementations.

Main changes

Current Async Storage is strictly coupled with its storage implementation (SQLite for Android and serialized dictionary/files for iOS). The replaceable storage feature would give you the ability to use adequate storage for a given task - think of using Shared Preferences for simple key-values, while still being able to use SQLite database for larger data, using the same API.

Key benefits :

  • Use one or more storages that matches your application needs, using the same API
  • Support for out-of-the-tree platforms
  • Storages are developed independently

Implementation

There are three main parts of this feature:

  • Async Storage factory
  • Async Storage Public API
  • Storage implementation (using Common Storage API)

AS

Async Storage Factory

This factory class creates an instance of AsyncStorage with selected storage set. You can pick storage used based on platform or other condition.

class AsyncStorageFactory {
  static create(storage: AsyncStorageBackend, opts?: FactoryOptions): AsyncStorage {}
}

Creation of Async Storage can be enhanced by passing FactoryOptions options. Features such as logging can be set up here (added later, over time).

type FactoryOptions {
	// use default or provide logger function
	enableLogging: (logger?: boolean | function): any
}

Public API

class AsyncStorage {

 /* 
  * expected to return a value stored under 'key'
  * `null` when value not found
  */ 
  get(key: string, options?: AsyncStorageOptions): Promise<mixed> {}


 /*
  * expected to save `item`, using `key`
  */
  set(key: string, item: mixed, options?: AsyncStorageOptions): Promise<void> {}


 /*
  * expected to get a value for each passed key
  * returns array with retrieved items
  */
  getMultiple(
    keys: Array<string>,
    options?: AsyncStorageOptions,
  ): Promise<Array<mixed>> {}


 /*
  * expected to save key-values in storage
  */
  setMultiple(keyValueArray: Array<Object>, options?: AsyncStorageOptions): Promise<void> {}


 /*
  * expected to removed item, stored under `key`
  * returned value denotes success of operation ( true = success )
  */
  remove(key: string, options?: AsyncStorageOptions): Promise<void> {}


 /*
  * expected to remove multiple values
  */
  removeMultiple(keys: Array<string>, options?: AsyncStorageOptions): Promise<void> {}
  
  
 /*
  * expected to return all keys used to store values
  */
  getKeys(options?: AsyncStorageOptions): Promise<Array<string>> {}
  
 /*
  * expected to clear all stored data in storage
  */
  removeWholeDataFromTheStorage(options?: AsyncStorageOptions): Promise<void>

 /*
  * returns the instance of the storage backend. Useful if it has additional functionality not covered by public API
  */
  instance(): AsyncStorageBackend
}
  1. Supported actions are write/update, read and delete.

  2. Promise based API (no callbacks). This is mainly to focus on one API model and leverage Promise API (callbacks could be implemented through AsyncStorageOptions).

  3. AsyncStorageOptions is optional config passed down to storage implementation. Type is dependent on storage used.

type AsyncStorageOptions<T> = T;

Storage Backend API

interface AsyncStorageBackend<T> {

  getSingle<T>(key: string, opts?: T): Promise<T | null>

  setSingle<T>(key: string, value: T, opts?: T): Promise<void>

  getMany(keys: Array<string>, opts?: T): Promise<Array<any>>

  setMany(values: Array<{ [key: string]: any }>, opts?: T): Promise<void>

  removeSingle(key: string, opts?: T): Promise<void>

  removeMany(keys: Array<string>, opts?: T): Promise<void>

  getKeys(opts?: T): Promise<Array<string>>

  dropStorage(opts?: T): Promise<void>
}

AsyncStorageBackend interface (Common Storage API on the diagram) must be applied by any Storage Backend in order to be valid. Those methods are called by Public API, passing options down.

As an example, where's how Storage Backend would look for a Web (using localStorage):

class WebStorageBackend implements AsyncStorageBackend<OptionsType> {
  async getSingle(key: string, opts?: OptionsType) {
    return window.localStorage.getItem(key)
  }

  async setSingle(key: string, value: any, opts?: OptionsType) {
    return window.localStorage.setItem(key, value)
  }

  // ...
}

Other changes

  • Migration to Typescript (keeping support for Flow)

  • Monorepo (see questions below)

  • TBD.

Questions

  • API (both public and common storage)

  • Do we plan to maintain Storage Backends in the main repo? If yes, should we move to monorepo?

@krizzu krizzu added the enhancement New feature or request label May 28, 2019
@krizzu krizzu self-assigned this May 28, 2019
@krizzu krizzu pinned this issue May 28, 2019
@satya164
Copy link

satya164 commented May 28, 2019

One feedback I have is that since the public API and the storage backend API have 1:1 mapping, it'll be better not to have different naming for them and have the same API for less cognitive overhead.

Though it also begs the question, if the storage backend and the public API have the same methods, then what'll be the advantage of AsyncStorageFactory.create instead of using the storage backend directly? Will there be some additional functionality added by the factory?

Regarding the storage backend for web, instead of localStorage, I'd suggest using IndexedDB for the default implementation because localStorage is sync and doesn't work in web workers etc.

@3rd-Eden
Copy link

If we are going to completely break the public API, I would suggest that we look for API compatibility with the kv-storage proposal:

https://github.com/WICG/kv-storage

@matt-oakes
Copy link

This sounds good! I have one question and a couple of comments:

Maintaining backwards compatibility

Could the new public API be implemented without completely breaking older code?

I did this with the NetInfo module by checking the type of the parameters and falling back to old behaviour if needed. This should help smooth out the adoption of the new version as it will be less of a problem if two libraries depend on AsyncStorage and one hasn't been updated.

It's more work to do this, but is much nicer for users of library. You can see a chunk of the NetInfo compatibility code here:

https://github.com/react-native-community/react-native-netinfo/blob/5dab2534d49b32ffdc386c807f6f3e077c644c6c/src/index.ts#L33-L94

Using kv-storage API as a base

I like the idea, but it seems like their API does not support getting and setting multiple keys at once. This is a really good optimisation for React Native due to the bridge and not something we should loose.

@krizzu
Copy link
Member Author

krizzu commented May 29, 2019

Thank you all for the comments and suggestions!

@satya164 I've left names different to explicitly distinguish public API from Backend API, for cases like calling Storage directly (not through AsyncStorage API).
Regarding mapping, I thought about adding options to the factory, to extends the AS instance (for example, adding fallback storage, in case your primary is not available or some kind of middleware extension, like a logger, to ease storage debugging).

@3rd-Eden As @matt-oakes said we have methods to do operations on more than one data (multi set/get/remove), for optimization reasons, which are not supported in kv-storage. I also think that more explicit names like getItem, are more self-explaining.

@matt-oakes I think that adding the factory is already a big breaking change, but we could add support for old methods name (like getMulti/setMulti), by providing a flag to the factory (while creating an instance).

thanks.

@pbitkowski
Copy link
Contributor

Awesome idea! I would go with monorepo with current implementations for Android / iOS. We can let people create their own implementations, but let's provide bottom line for people who don't want to do this.

I agree with @satya164, making explicit distinguish sounds good, but why do we want to change public API? I would change the Backend API method names, not the Public API if they must be easy to differentiate.

Good direction for improvements! I will keep my fingers crossed.

@zamotany
Copy link

zamotany commented May 29, 2019

I like the idea, tough I'm against having backward compatibility as a prime idea. We are talking about a new major version of Async Storage, so it is fine to change API. IMO the public API that kv-storage has is better that the current Async Storage API - for instance setItem is no better than just set, like what else could I set except for item? It's unnecessarily verbose.

That being said backward compatibility is a important feature for some people, so IMO it is better to have it as a opt-in in factory - there could be create and createLegacy or just a flag in create.

@ferrannp
Copy link

ferrannp commented May 29, 2019

I love the idea to use SharedPrefernces and NSUserDefaults. I really do not need SQLite to save key-value settings.

Also I think it is nice pointing out that with the Async Storage Factory, it should be easier to also access the data from native directly (useful for brownfield apps and others).

Do we plan to maintain Storage Backends in the main repo? If yes, should we move to monorepo?

In favour of having some storages available.

--

 /*
  * expected to return all keys used to store values
  */
  getStorageKeys(options?: AsyncStorageOptions): Promise<Array<string>> {}
  
 /*
  * expected to clear all stored data in storage
  */
  removeWholeDataFromTheStorage(options?: AsyncStorageOptions): Promise<void>

Is the Storage word necessary? Why not getKeys or getAllKeys and something like clear or clearAll ?

@krizzu
Copy link
Member Author

krizzu commented May 29, 2019

@pbitkowski @zamotany I want public API to be as clear as it can get. I'm fine with set/get/remove/multi*/ API.

@ferrannp I'm fine with getKeys, it's just a proposal. Regarding removeWholeDataFromTheStorage - more explicit name for irrecoverable action.

Regarding backward compatibility, I was thinking about providing LegacyStorage, which will use old storage location/files, with new API (so you're not giving up on your old data).

AsyncStorageFactory could be enhanced with options object. For example, a flag to enable logging, so it'd be easier to debug what's going in and out of storages.

@tido64
Copy link
Member

tido64 commented May 29, 2019

Will the multi* functions still be relevant with the upcoming Fabric rewrite? If they're only there to reduce the number times you have to cross the bridge, I wonder if we have enough usage to warrant carrying it over to v2. I know we don't use them in our apps. I'd vote to have the API closer to kv-storage if that's what the community is moving towards.

Re: AsyncStorageBackend, I think another way to think about it is that it only provides the smallest building blocks for a storage. AsyncStorageFactory then gives us the ability to build things upon it, like logging as @krizzu mentioned.

@grabbou
Copy link

grabbou commented Jun 2, 2019

Great work on the RFC, I really like that.

One question - what do you think about supporting JavaScript objects by default and letting the backend to decide whether they want to serialise it entirely or not? Doing JSON.parse/JSON.serialise is not a big deal, but I feel like moving this responsibility over to storage backend would allow for more performant storage (if backend supports objects) and more expressive interface for setting and accessing values.

@krizzu
Copy link
Member Author

krizzu commented Jun 5, 2019

@grabbou The main assumption is to let Storage decide what to do with given data (serialize it or not, modify it somehow, etc.).

Thank you all for the feedback. I'll do adjustments to API and add necessary details so we could begin the work soon.

thanks.

@crrobinson14
Copy link

I'd love to see two things:

  1. A way to determine storage usage (bytes) via the API. There's a (lightly documented) method through the debugger, but the idea here would be to a. make this act a first-class citizen in the API, and b. allow programs to monitor themselves better (report it to QOS systems, log it to the console at app start, etc.)

  2. Clarify the API around clearing the store. Right now there is a very convenient .clear() operation with specific instructions NOT to use it because an app can delete more than its own data. It's not clear at all what "other" data would be cleared here, or why the function is provided in the first place if it's so dangerous. The docs suggest clearing per key, but this is a nuisance for app developers to do and you can see how most might reach for the more convenient option.

Clearing the store is an operation nearly every app should include if there's any kind of logout function. It would be great to clean up how this operation is done.

@krizzu
Copy link
Member Author

krizzu commented Jun 10, 2019

  1. I'd leave this as a storage backend feature. You'd be able to access it through its instance (see instance() method on public API).

  2. This functionality is strictly connected to the current implementation of AS. All all data is saved to one file and clear is there to drop this file. I want to avoid having single storage for multiple apps in v2.

I've updated the API a bit. Going to create a GH Project, so progress can be tracked down.

@krizzu
Copy link
Member Author

krizzu commented Jun 12, 2019

Plan to start v2:

  • Move current master under LEGACY branch, to keep it in maintain mode ( + Disable semantic-releases)
  • Move to monorepo, using Yarn Workspaces:
    • core (public API)
    • storages to hold maintained storages implementation
    • docs where the whole API will be documented

Going to start working on this later this week.

@krizzu krizzu added LEGACY and removed LEGACY labels Jun 18, 2019
This was referenced Jun 18, 2019
@sidferreira
Copy link

Is there a plan/discussion of using CoreData's SQLite on iOS? (Worked in similar issue a few days ago and made me wonder about it)

@krizzu
Copy link
Member Author

krizzu commented Jul 29, 2019

@sidferreira The good thing about this RFC is that it allows you to create your own Storage solution and use it with Async Storage - you just need to apply an interface.

@sidferreira
Copy link

Is there any timeline in mind?

@krizzu
Copy link
Member Author

krizzu commented Jul 29, 2019

Doing this in my free time, so trying to squeeze in as much as I can. Sorry, no timeline in mind just yet

@sidferreira
Copy link

LMK how to help (I still have to read all issues about this, maybe I can figure out of it...)

@jgcmarins
Copy link

Hello!
Great work on this RFC.
I've found an article about AsyncStorage Optimization: https://medium.com/@Sendbird/extreme-optimization-of-asyncstorage-in-react-native-b2a1e0107b34

Few things listed there:

  • Group items into a block for less disk operation
  • Batch write
  • Dump Promise
  • Memory caching

This list reminds me about Dataloader, which has browser support.
Any thoughts?

@sonnyp
Copy link

sonnyp commented Oct 12, 2019

I'd suggest implementing / "subclassing" the public kv:storage API

See

So that JS code can easily target both react-native and [react-native-]web .

@gamingumar
Copy link

Does v2 has any solution for 2mb cursor limit? I have tested in different android phones, I think the issue is only in devices with android < 8. Nexus 5 with android 6 has the issue and s9 with android 9 works fine.
#10

@krizzu
Copy link
Member Author

krizzu commented Nov 12, 2019

@gamingumar Plan is to move the current AsyncStorage implementation into v2 (named as Legacy storage), so you won't lose your data once you upgrade.

Reimplementing the Legacy storage is something that needs discussing, so that it would cover most of the cases other devs are facing.

@morrys
Copy link

morrys commented Dec 1, 2019

Hi guys,
this discussion is very interesting and I would like to have your opinion about the wora/cache-persist library that I used in react-relay-offline & wora/apollo-offline to manage persistence and offline capabilities.

In short, wora/cache-persist is an object that allows you to manage interfacing with storage (localStorage, sessionStorage, indexedDB, AsyncStorage & any custom storage) in "synchronous" mode and with the possibility of serializing and deserialization of keys and values ​​in storage (used to filter keys, encription, compression ...)

This result is achieved by combining:

  • the initial restore of data persisted in an inmemory object (this is asynchronous, so either you perform the await on restore or perform the reconciliation between the initial state and the persistent state)
  • after restore, data is read and written synchronously in the inmemory object (optimizes performance)
  • each mutation of the object in memory is inserted in a queue managed asynchronously (the storage update interval is configurable, throttle default 500ms)
  • at the end of the interval a merge of the mutations is made in order to avoid repeated writing
  • the writes are grouped and the multiSet function of the storage is invoked
  • removals are grouped and the storage multiRemove function is invoked

@Sofianio
Copy link

Why not make getMultiple return an object instead of an array?

const {name, email} = await AsyncStorage.getMultiple(['name', 'email']);

@denkristoffer
Copy link

I agree that set and get is simpler than setItem and getItem, but there's an argument to be made for conserving the existing API, since it allows for "plug and play" interoperability with a lot of libraries without having to wrap the new AsyncStorage yourself.

@krizzu
Copy link
Member Author

krizzu commented Dec 29, 2019

@Sofianio
Consistency. setMany accepts an array of key-values, to save value for given key. Similarly, getMany accepts an array of keys (strings) and retrieves values, returning an array of them.
You can use array destructuring to achieve similar output:

const [name, email] = await AsyncStorage.getMultiple(['name', 'email']);

@denkristoffer
That's interesting point. Can you elaborate bit more about it?
The point of v2 is to have a unified way of accessing persistent storage mechanism. This allows for different implementation of storage itself and support for other platforms than just Mobile, while using the same API.

@denkristoffer
Copy link

@krizzu I just think it might not be worth changing the method names for a slight API improvement if it means breaking compatibility with every library that works with the existing AsyncStorage implementation or window.localStorage 🙂 But if they are to be renamed, now's the best time to do it of course!

@Sofianio
Copy link

@krizzu Then why not make setMany take an object and getMany return an object ?
seems easier to deal with than array of arrays

@bsp003
Copy link

bsp003 commented Jan 10, 2020

@krizzu Can we set AsyncStorage location for iOS now? I didn't see anything on it in here or might have missed it out. Please suggest.

@micabe
Copy link

micabe commented Mar 2, 2020

Can't use with redux-persist it expects getItem and not get, setItem and not set !!!!?

@blackbing
Copy link

clearStorage will throw error

Simulator Screen Shot - iPhone X - 2020-03-25 at 13 13 11

and the error points to here:
Screen Shot 2020-03-25 at 1 17 08 PM

@kennetpostigo
Copy link

This still happens, and has not been fixed, it just doesn't show a "Red"/Error screen any longer. I see a warning being logged and a white screen. In the log it will say:

[TypeError: undefined is not an object (evaluating 'this.error')]

With a trace pointing to a library file. I'm only using .get at the moment. The second I comment out the line, it will render the app for me.

@krizzu
Copy link
Member Author

krizzu commented Apr 10, 2020

@kennetpostigo
Which line specifically?

@kennetpostigo
Copy link

const magic = await storage.get('@magic-link')

@krizzu
Copy link
Member Author

krizzu commented Apr 12, 2020

@kennetpostigo Could you open a new issue describing this problem with repro steps? Thanks in advance.

Thanks everyone for this conversation. It's been a long time since I started this and due to the fact that v1 also needs active attention for development, I'm putting v2 on hold. It's still open for PRs/suggestions, but I'm going to be less active for this part of AsyncStorage.

@krizzu krizzu closed this as completed Apr 12, 2020
This was referenced Apr 12, 2020
@krizzu krizzu unpinned this issue Jun 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests