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

Jump to bottom

Migrate from AsyncStorage to FileStorage #2084

Merged
merged 13 commits into from Apr 1, 2021
Merged

Migrate from AsyncStorage to FileStorage #2084

merged 13 commits into from Apr 1, 2021
+102 −9

Conversation

rickycodes
Copy link
Member

@rickycodes rickycodes commented Jan 4, 2021 •
edited

Description

This is a migration that gets Redux Persist using redux-persist-filesystem-storage instead of AsyncStorage. This fixes a rather nasty bug on Android from cropping up that you can read more about: here and here

the migration logic can be found in app/store/index.js

Checklist

  • There is a related GitHub issue
  • Tests are included if applicable
  • Any added code is fully documented

Issue

Resolves #2100

@rickycodes rickycodes changed the title WIP: Migrate from AsyncStorage to FileStorage Migrate from AsyncStorage to FileStorage Jan 7, 2021
@rickycodes rickycodes marked this pull request as ready for review January 7, 2021 20:21
@rickycodes rickycodes requested a review from a team as a code owner January 7, 2021 20:21
@rickycodes rickycodes added the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Jan 7, 2021
@rickycodes rickycodes force-pushed the storage-migration branch 2 times, most recently from a039c6b to 23527e2 Compare January 7, 2021 23:51
@andrepimenta
Copy link
Member

I have some considerations to possibly address:

  • I am not sure if the filesystem is the best storage to consider because of security. If we take a look at this table, files saved with the filesystem on the document folder are accessible from other apps. I think the lib we are using on this PR saves on the document folder because it needs external storage access and the default config for the folder is RNFetchBlob.fs.dirs.DocumentDir.
  • We could instead use redux-persist-sqlite-storage or redux-persist-sensitive-storage (which uses sharedpreferences). We should test performance.
  • For the migration there is a guide that we could also use: https://github.com/robwalkerco/redux-persist-filesystem-storage#migration-from-previous-storage

@omnat omnat added the DO-NOT-MERGE Pull requests that should not be merged label Jan 14, 2021
@rickycodes rickycodes removed the DO-NOT-MERGE Pull requests that should not be merged label Mar 25, 2021
@rickycodes
Copy link
Member Author

LGTM

@sethkfman sethkfman added the needs-qa Any New Features that needs a full manual QA prior to being added to a release. label Mar 26, 2021
@ibrahimtaveras00 ibrahimtaveras00 added QA in Progress QA has started on the feature. and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Mar 31, 2021
ibrahimtaveras00
ibrahimtaveras00 approved these changes Mar 31, 2021
View reviewed changes
Copy link
Contributor

@ibrahimtaveras00 ibrahimtaveras00 left a comment

Choose a reason for hiding this comment

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

QA Passed 👍🏽

@ibrahimtaveras00 ibrahimtaveras00 added QA Passed A successful QA run through has been done and removed QA in Progress QA has started on the feature. labels Mar 31, 2021
@sethkfman sethkfman merged commit d3174cd into develop Apr 1, 2021
@sethkfman sethkfman deleted the storage-migration branch April 1, 2021 19:13
rickycodes added a commit that referenced this pull request Apr 8, 2021
@rickycodes
Merge branch 'develop' into feature/confusables
6f99419 
* develop:
  v2.1.0 (#2481)
  Analytics v2 (priority 1) (#2456)
  Fix/gas estimations (#2408)
  remove controllers tgz (#2479)
  Improvement/assets by chainid (#2441)
  Improvement/chain ticker (#2442)
  Remove instapay (#2372)
  Fix iOS build (#2467)
  Migrate from AsyncStorage to FileStorage (#2084)
rickycodes added a commit that referenced this pull request Jan 31, 2022
@rickycodes @andrepimenta @sethkfman
Migrate from AsyncStorage to FileStorage (#2084)
394fae8 
* Filesystem storage migration working

* Logging

* Add mock for rn-fetch

* Update package.json

* added throw to failed AsyncStorage so data will not be overwritten

* added try catch to filesystem

* async to set and remove methods in  migration

* updated key checking logic

* removed key check

Co-authored-by: andrepimenta <andrepimenta7@gmail.com>
Co-authored-by: sethkfman <seth.kaufman@consensys.net>
Co-authored-by: sethkfman <10342624+sethkfman@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ibrahimtaveras00 ibrahimtaveras00 ibrahimtaveras00 approved these changes

Assignees
No one assigned
Labels
needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) next release QA Passed A successful QA run through has been done
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Storage limit issue
5 participants
@rickycodes @andrepimenta @ibrahimtaveras00 @sethkfman @omnat