Switch async-storage repository #85

kidroca · 2021-07-06T21:00:41Z

Details

The package @react-native-community/async-storage is deprecated from @react-native-community repo
It's moved and maintained as @react-native-async-storage/async-storage
There are some improvements that have happened since then
There are no breaking changes listed

The package is listed in `peerDependencies` now

Libraries that are managed by the parent project should be listed as peerDependencies
There are some gradle settings that are tweaked from E.cash
a peer dependency ensures the app is using the same module in both Onyx and E.cash

The package is listed as a `devDependency` as well

npm 6 would not install peerDependencies during npm install
it's added as devDependency so that it is installed and used in tests
while when react-native-onyx is installed in a project - onyx will use the version of AsyncStorage that E.cash provides

This update need to go hand in hand with E.cash

E.cash needs to install the new @react-native-async-storage/async-storage (as a regular dependency)
And also update Onyx's hash to the current repository

Related Issues

Automated Tests

Updated tests to use the newer library mock

Linked PRs

Expensify/App#3898

kidroca · 2021-07-06T21:06:54Z

You can either merge this and update E.cash immediately
or we can run some tests with the /next version that uses Room to manage SQLite
https://react-native-async-storage.github.io/async-storage/docs/advanced/next

This Async Storage feature improves the persistence layer, using modern approaches to access SQLite (using Room), to reduce possible anomalies to the minimum.

quinthar · 2021-07-06T21:21:28Z

I'm a little fuzzy on what it's actually doing under the hood that's different/better. Room makes some pretty ambiguous statements about "compile time verification of queries", but does the new AsyncStorage actually use that? If the performance is the same, I'm skeptical that they have done anything other than switch direct SQLite with Room, but are probably still not using compiled statements and such.

Libraries that are managed by the parent project should be listed as `peerDependencies` There are some gradle settings that are tweaked from E.cash a peer dependency ensures the app is using the same module in both Onyx and E.cash

…dependency The library should be installed so that it's available in tests

kidroca · 2021-07-07T07:17:10Z

lib/Onyx.js

+            if (keyValuePairsForNewCollection.length > 0) {
+                promises.push(AsyncStorage.multiSet(keyValuePairsForNewCollection));
+            }


After the update AsyncStorage.multiSet throws when it is called with an empty array

kidroca · 2021-07-07T13:29:22Z

Some benchmark information between the 3 versions

Median times for 7 app startups per each AsyncStorage version

The app is launched
The initial chat is always the same - a conversation with 200+ messages
Timings are captured after the app stops loading

Timing for `Onyx.get`

	1.12.1	1.15.5	1.15.5 next
avg call time	131.55ms	140.95ms	134.90ms
total call time	32862.34ms	35175.71ms	33552.87ms
total calls count	529	529	529

note: a lot of the calls are resolved from Onyx cache, the actual hard reads vary from 200 to 1000ms

More data is available here:
https://docs.google.com/spreadsheets/d/1ABHCzs9yZIXmHxMJnDboyyMw8ICjQQ3sxIP_23c7Lws/edit?usp=sharing

kidroca · 2021-07-07T15:32:57Z

@quinthar

I'm a little fuzzy on what it's actually doing under the hood that's different/better. Room makes some pretty ambiguous statements about "compile time verification of queries", but does the new AsyncStorage actually use that? If the performance is the same, I'm skeptical that they have done anything other than switch direct SQLite with Room, but are probably still not using compiled statements and such.

I've reviewed the code and the documentation and they are indeed using whatever "compile time verification" Room provides
With Room they've removed boilerplate code and now storage handling is down to this:
https://github.com/react-native-async-storage/async-storage/blob/master/android/src/main/java/com/reactnativecommunity/asyncstorage/next/StorageSupplier.kt#L34-L75

    @Transaction
    @Query("SELECT * FROM $TABLE_NAME WHERE `$COLUMN_KEY` IN (:keys)")
    suspend fun getValues(keys: List<String>): List<Entry>

Data access is defined through annotations, these annotations provide the compile time verification

Overall with Room the code has become a lot smaller and easy to follow, compared to what it was before:

old multiGet: https://github.com/react-native-async-storage/async-storage/blob/master/android/src/main/java/com/reactnativecommunity/asyncstorage/AsyncStorageModule.java#L113
new multiGet: https://github.com/react-native-async-storage/async-storage/blob/master/android/src/main/java/com/reactnativecommunity/asyncstorage/next/StorageModule.kt
- note: getValues is implemented by Room

As you can see all the operations are managed by Room. And I can confirm it's very much in line with their documentation on how to setup and use Room
This information together with the fast read/writes time I see in the standalone benchmark app suggest that the issue with slow reads in E.cash/Onyx might be somewhere else

deetergp

The reasoning behind dropping @react-native-community/async-storage for @react-native-async-storage/async-storage seems pretty sound, and the changes look pretty straight forward. The benchmarks definitely seem like they are trending in the wrong direction, but I'm not sure what we can do about that.

I would love to get a second opinion from @tgolen and/or @marcaaron before giving this a thumbs up.

marcaaron · 2021-07-07T17:02:22Z

There are some improvements that have happened since then

Not quite following with the benefits of this change are or how it relates to the linked issue. What improvements were added? @kidroca can you give us a brief summary of why updating this package is valuable in the short term?

I saw some plans to essentially re-write AsyncStorage from the ground up using the JSI instead of React Native's bridge, but not sure if this PR pushes us towards that goal or does something else?

marcaaron · 2021-07-07T17:06:04Z

Ah I see the comment left by @tgolen now 👍

I think there is no harm in routinely updating to the most recent version of this package. But doesn't seem like we should expect any improvements? Lmk if that sounds correct.

tgolen · 2021-07-07T19:43:54Z

package.json

@@ -41,6 +41,9 @@
    "react-test-renderer": "16.13.1",
    "metro-react-native-babel-preset": "^0.61.0"
  },
+  "peerDependencies": {
+    "@react-native-async-storage/async-storage": "^1.15.5"


I've never used peerDependencies before. What is it for and is it necessary?

I've put that into the description but here's some more information

AsyncStorage is linked to native ios/android through the project that uses it - E.cash
You can set some gradle properties and settings there and this cannot work if it's not a part of E.cash

It sounds counterintuitive to provide a dependency to a package you're installing but this addresses problems like 2 or more different version of AsyncStorage being installed

You need AsyncStorage in E.cash so you can configure it and link it to the app
As you've seen E.cash and Onyx actually declared different version of AsyncStorage
To avoid this problem only one package should install the dependency
When npm install installs react-native-onyx it would hook it to the same version of AsyncStorage installed for the project

Usually libraries need to work together with other libraries that use the same packages, sometimes they even need to share the same instance of a module as well that's why they leave the project that installs them to resolve their peer dependencies

Oh wow, that's really interesting. Funny that I've never ran into this before, but ReactNative is a bit of a different monster that I'm still getting used to. Thanks for the information!

kidroca · 2021-07-07T20:24:19Z

I think there is no harm in routinely updating to the most recent version of this package. But doesn't seem like we should expect any improvements? Lmk if that sounds correct.

This version (1.15.1) includes a total rework for the SQL handling (enabled optionally), we've been discussing in the linked thicket
Unfortunately it doesn't seem to improve E.cash

kidroca requested a review from a team as a code owner July 6, 2021 21:00

MelvinBot requested review from deetergp and removed request for a team July 6, 2021 21:01

kidroca added 5 commits July 7, 2021 10:11

chore: switch async-storage repository

0227177

chore: also include @react-native-async-storage/async-storage as dev …

fbccb45

…dependency The library should be installed so that it's available in tests

refactor: update @react-native-async-storage references in tests

a1ec213

fix: calling multiSet with an empty array throws an exception

064748f

kidroca force-pushed the kidroca/update-async-storage-version branch from 4ff2179 to 064748f Compare July 7, 2021 07:11

kidroca commented Jul 7, 2021

View reviewed changes

kidroca mentioned this pull request Jul 7, 2021

Switch async-storage repository Expensify/App#3898

Merged

5 tasks

kidroca mentioned this pull request Jul 7, 2021

[Hold for payment 2021-08-17] [Performance] Optimize AsyncStorage on Android Expensify/App#2667

Closed

deetergp reviewed Jul 7, 2021

View reviewed changes

tgolen assigned kidroca Jul 7, 2021

tgolen reviewed Jul 7, 2021

View reviewed changes

tgolen approved these changes Jul 8, 2021

View reviewed changes

deetergp approved these changes Jul 9, 2021

View reviewed changes

deetergp merged commit 84a27cd into Expensify:master Jul 9, 2021

kidroca mentioned this pull request Jul 11, 2021

Update react to v17 #86

Merged

tgolen mentioned this pull request Aug 19, 2021

Fix MD Hyperlink Regex for nested cases Expensify/expensify-common#407

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch async-storage repository #85

Switch async-storage repository #85

kidroca commented Jul 6, 2021 •

edited

Loading

kidroca commented Jul 6, 2021

quinthar commented Jul 6, 2021

kidroca Jul 7, 2021

kidroca commented Jul 7, 2021 •

edited

Loading

kidroca commented Jul 7, 2021

deetergp left a comment

marcaaron commented Jul 7, 2021

marcaaron commented Jul 7, 2021

tgolen Jul 7, 2021 •

edited

Loading

kidroca Jul 7, 2021

tgolen Jul 8, 2021

kidroca commented Jul 7, 2021 •

edited

Loading

Switch async-storage repository #85

Switch async-storage repository #85

Conversation

kidroca commented Jul 6, 2021 • edited Loading

Details

The package is listed in peerDependencies now

The package is listed as a devDependency as well

This update need to go hand in hand with E.cash

Related Issues

Automated Tests

Linked PRs

kidroca commented Jul 6, 2021

quinthar commented Jul 6, 2021

kidroca Jul 7, 2021

Choose a reason for hiding this comment

kidroca commented Jul 7, 2021 • edited Loading

Timing for Onyx.get

kidroca commented Jul 7, 2021

deetergp left a comment

Choose a reason for hiding this comment

marcaaron commented Jul 7, 2021

marcaaron commented Jul 7, 2021

tgolen Jul 7, 2021 • edited Loading

Choose a reason for hiding this comment

kidroca Jul 7, 2021

Choose a reason for hiding this comment

tgolen Jul 8, 2021

Choose a reason for hiding this comment

kidroca commented Jul 7, 2021 • edited Loading

kidroca commented Jul 6, 2021 •

edited

Loading

The package is listed in `peerDependencies` now

The package is listed as a `devDependency` as well

kidroca commented Jul 7, 2021 •

edited

Loading

Timing for `Onyx.get`

tgolen Jul 7, 2021 •

edited

Loading

kidroca commented Jul 7, 2021 •

edited

Loading