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

Switch async-storage repository #3898

Merged
merged 5 commits into from
Jul 9, 2021

Conversation

kidroca
Copy link
Contributor

@kidroca kidroca commented Jul 7, 2021

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

This PR needs to be updated with the correct Onyx hash Expensify/react-native-onyx#85 is merged

Fixed Issues

Related to #2667

Tests

Since storage affects everything a general tests for regressions should be carried

QA Steps

Since storage affects everything a general tests for regressions should be carried

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

image

Mobile Web

image

Desktop

image

iOS

image

Android

image

@kidroca kidroca requested a review from a team as a code owner July 7, 2021 10:38
@MelvinBot MelvinBot requested review from tgolen and removed request for a team July 7, 2021 10:38
Copy link
Contributor Author

@kidroca kidroca left a comment

Choose a reason for hiding this comment

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

I've enabled the next AsyncStorage version
https://react-native-async-storage.github.io/async-storage/docs/advanced/next

I can revert the changes if you prefer
Can't really see any differences in app speeds with either the "old" async storage, the "new" and the "new + next"

android/build.gradle Show resolved Hide resolved
android/gradle.properties Show resolved Hide resolved
android/settings.gradle Show resolved Hide resolved
package.json Outdated
@@ -80,7 +80,7 @@
"react-native-image-picker": "^4.0.3",
"react-native-keyboard-spacer": "^0.4.1",
"react-native-modal": "^11.10.0",
"react-native-onyx": "git+https://github.com/Expensify/react-native-onyx.git#1e82e592032c6d0ede8e40f08beb6be790d149e8",
"react-native-onyx": "git+https://github.com/kidroca/react-native-onyx.git#064748fdc6ceef7c9ffe8c5fa62a30ef234aae2a",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This hash should be updated after the Onyx PR is merged

@tgolen
Copy link
Contributor

tgolen commented Jul 7, 2021

Cool, thanks! I think trying out the next storage will be fine to do.

@@ -38,7 +38,7 @@
"@formatjs/intl-numberformat": "^6.2.5",
"@formatjs/intl-pluralrules": "^4.0.13",
"@onfido/react-native-sdk": "^1.3.3",
"@react-native-community/async-storage": "^1.11.0",
"@react-native-async-storage/async-storage": "^1.15.5",
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, so why are we installing this lib directly? I'm not sure why the previous lib was referenced here either. AFAIK, the only way we should be interfacing with AsyncStorage is through Onyx.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not seeing it directly imported into any files, so can this just be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is related to the comment I've made on peerDependencies on the other PR
We need to have it listed as a project dependency

We also have one place where it's imported - inside mocks
The e2e tests that use Onyx stub AsyncStorage from E.cash mocks

@kidroca
Copy link
Contributor Author

kidroca commented Jul 7, 2021

I've put this comment on the wrong place, so I'm posting it again here

I've run the 3 version against E.cash and here is a short summary

Median times for 7 app startups per each AsyncStorage version

  1. The app is launched
  2. The initial chat is always the same - a conversation with 200+ messages
  3. 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

tgolen
tgolen previously approved these changes Jul 8, 2021
@tgolen tgolen changed the title Switch async-storage repository [HOLD] Switch async-storage repository Jul 8, 2021
@tgolen
Copy link
Contributor

tgolen commented Jul 8, 2021

This is approved, but I am adding a [HOLD] to the title so that it's not merged until the hash for react-native-onyx is updated.

@kidroca
Copy link
Contributor Author

kidroca commented Jul 9, 2021

Updated react-native-onyx hash

@tgolen tgolen changed the title [HOLD] Switch async-storage repository Switch async-storage repository Jul 9, 2021
@tgolen tgolen merged commit ee2f84f into Expensify:main Jul 9, 2021
@OSBotify
Copy link
Contributor

OSBotify commented Jul 9, 2021

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify
Copy link
Contributor

🚀 Deployed to production in version: 1.0.77-5🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

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