iOS clean migration from RNCAsyncStorage_V1 to RTCAsyncStorage_V1 #64

reilem · 2019-04-09T11:10:31Z

Summary:

See: #55

Test Plan:

The only type of tests I have already run is:

Hardcode set the variable RCTStorageDirectory to RNCAsyncStorage_V1
Run the app & set some values to cache
Close the app
Reset RCTStorageDirectory to RCTAsyncStorage_V1
Run the app and see that cached values have been migrated

I have performed these tests with and without a pre-existing storage directory and they work in both cases.

My only concerns are the way I am currently handling errors. If something goes wrong an NSException is thrown which will trigger a SIGABRT shutdown of the app since it is never caught. I am not extremely experienced with error handling in Objective-C so perhaps someone has some better ideas.

krizzu · 2019-04-09T15:52:51Z

Hey @reilem, thanks for your work!
Code looks fine to me, so let me just test this on my end and let's merge it afterwards.

ios/RNCAsyncStorage.m

tido64 · 2019-04-09T17:45:35Z

ios/RNCAsyncStorage.m

+      if (!newStorageDirectoryExists) {
+        // If new storage direction doesn't exist, copy old storage directory to new location
+        if (![[NSFileManager defaultManager] copyItemAtPath:RCTCreateStorageDirectoryPath(OldStorageDirectory) toPath:RCTGetStorageDirectory() error:&error]) {
+          @throw RCTStorageDirectionMigrationException(@"Failed to copy old storage directory to new storage directory", error);


We probably shouldn't take down the whole app if we fail to migrate. I think in the rare cases where IO might fail, maybe it's probably fine to just say that we "lost" the data?

Yeah I am not a big fan of crashing the app either, but I'm not a fan of leaving errors unused either. I guess in this case it would be just fine to ignore the errors? Cause I don't think adding NSLogs will be of much use either.

I think there's also a real risk of going into a crash loop. I think we should ignore it, and use RCTLogWarn or RCTLogError to log errors. What do you think, @krizzu?

Ah cool, didn't know these existed. Errors would also cause app crashes so I guess we could go with the Warnings to be safe. Right now I've replaced everything with NSLogs for now, so let me know what you guys think is best.

I think I prefer RCTLogWarn because they will at least be visible during debug as a yellow box, in addition to being logged to the console.

krizzu · 2019-04-09T18:11:55Z

@tido64 Thanks for having a look. @reilem Thank you for your work 💪

On a second though, simply deleting RCT folder first would lead to some data loss as well.

Say, someone used pre-1.2.2 AS, then upgrades to 1.2.3/4 (RCT used here, meaning "fresh start"). Now, upgrading to next version (with this feature included), will again end with overriding with stuff from RNC.

I like the approach that @tido64 suggested, to check when each of the files (from RNC and RTC) were accessed and based on that proceed with migration.

Something like that:

if(RNC_DIR.exists){
    if(RCT_DIR.exists){
        if(RNC_DIR.accessTime > RCT_DIR.accessTime){
            // RNC being used, pre 1.2.2
            rm RCT_DIR;
            mv RNC_DIR RCT_DIR;
        else {
            // RCT in use, so ditch RNC
            rm RNC_DIR;
        }
    else {
        // RCT does not exists, so move RNC
        mv RNC_DIR RCT_DIR;
    }
}

Let me know what you think!

reilem · 2019-04-09T21:46:58Z

I've made the requested changes however I have encountered a strange issue during testing. While the NSFileManager displays that old storage directories have been successfully removed and no longer exist (explicitly checked) I still somehow manage to get the actual old data displayed in my app. I'm guessing that my form of testing really isn't robust enough since it seems like some sort of cached data is being accessed.

Aside from the above issue, I have debugged the flow of the migration in the four cases and they seem to be working:

Old data not exists --> do nothing
Old data exists --> New data not exists --> copy --> remove old
Old data exists --> New data exists --> New data is newer --> remove old
Old data exists --> New data exists --> Old data is newer --> remove new --> copy --> remove old

krizzu · 2019-04-10T06:51:20Z

@reilem Hey,

This strange behavior might come from the regression we had on iOS in version 1.2.3 and 1.2.4. Please rebase with latest master and check if it works for you and let me know.

reilem · 2019-04-10T10:22:54Z

I think I discovered my issue, during refactoring I made the manifest file path using stringByAppendingString instead of stringByAppendingPathComponent.

* Use const and RCT prefix for RCTOldStorageDirectory * Log errors instead of throwing exceptions * Disable main queue setup

reilem · 2019-04-10T10:46:46Z

I have rebased on the latest PR merge, fixed the manifest issue and now use RCTLogWarn. All my "manual" tests are working now without issue. I thought about automating the tests, but I don't know of any accurate way to automate app restarts and I couldn't find anything about mocking const values in objective-c. Unless you guys have any ideas? 😄

krizzu · 2019-04-10T12:03:58Z

Looking good! Going to test it manually later today to confirm it from my side.

thanks.

tido64

Thanks for the good work! Code looks good to me. Just one little nitpick 😛

I haven't tested it yet. Maybe I have some time later today if @krizzu hasn't done it already 👍

ios/RNCAsyncStorage.m

krizzu

I've checked every scenario locally and I can confirm it works as expected! Outstanding work @reilem 👏

Let's wait for #71 to be merged, so we could get a new release.

krizzu · 2019-04-11T18:58:01Z

@reilem @tido64 Thanks guys! 🥇

## [1.3.1](v1.3.0...v1.3.1) (2019-04-12) ### Bug Fixes * iOS clean migration from RNCAsyncStorage_V1 to RTCAsyncStorage_V1 ([#64](#64)) ([8d275ad](8d275ad))

krizzu · 2019-04-12T08:41:42Z

🎉 This PR is included in version 1.3.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

## [1.3.1](react-native-async-storage/async-storage@v1.3.0...v1.3.1) (2019-04-12) ### Bug Fixes * iOS clean migration from RNCAsyncStorage_V1 to RTCAsyncStorage_V1 ([#64](react-native-async-storage/async-storage#64)) ([8d275ad](react-native-async-storage/async-storage@8d275ad))

krizzu added enhancement New feature or request platform: iOS This is iOS specific labels Apr 9, 2019

tido64 requested changes Apr 9, 2019

View reviewed changes

reilem force-pushed the master branch from e9c44f1 to 0304480 Compare April 10, 2019 08:52

reilem added 9 commits April 10, 2019 12:37

Add init & extract storage directory path creation

15eaafe

Implement RCTStorageDirectoryMigrationCheck

b165aa3

Fix RCTStorageDirectoryMigrationCheck build errors & styling

9a54cdd

Use explicit item deletion instead of replaceItem & added NSExceptions

3f7fc19

Revert styling changes

0601d5c

Minor requested changes

2aa0c17

* Use const and RCT prefix for RCTOldStorageDirectory * Log errors instead of throwing exceptions * Disable main queue setup

Check manifest's last modified date during migration

964b16a

Make RCTOldStorageDirectory static

e55fd44

Fix manifest path

3a26f42

reilem force-pushed the master branch from 9c96c83 to 3a26f42 Compare April 10, 2019 10:38

reilem added 2 commits April 10, 2019 12:40

Use RCTLogWarn instead of NSLog to log errors

bcce592

Revert style changes

c28e978

danielgreane mentioned this pull request Apr 10, 2019

Not working on iOS after RN upgrade to 0.59.2 rt2zz/redux-persist#1014

Open

tido64 previously approved these changes Apr 11, 2019

View reviewed changes

ios/RNCAsyncStorage.m Outdated Show resolved Hide resolved

Move OldStorageDirectory const to top

0b5034d

reilem dismissed tido64’s stale review via 0b5034d April 11, 2019 07:35

krizzu approved these changes Apr 11, 2019

View reviewed changes

tido64 approved these changes Apr 11, 2019

View reviewed changes

tido64 merged commit 8d275ad into react-native-async-storage:master Apr 11, 2019

krizzu pushed a commit that referenced this pull request Apr 12, 2019

chore(release): 1.3.1 [skip ci]

42fec96

## [1.3.1](v1.3.0...v1.3.1) (2019-04-12) ### Bug Fixes * iOS clean migration from RNCAsyncStorage_V1 to RTCAsyncStorage_V1 ([#64](#64)) ([8d275ad](8d275ad))

krizzu added the Released label Apr 12, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

iOS clean migration from RNCAsyncStorage_V1 to RTCAsyncStorage_V1 #64

iOS clean migration from RNCAsyncStorage_V1 to RTCAsyncStorage_V1 #64

reilem commented Apr 9, 2019

krizzu commented Apr 9, 2019

tido64 Apr 9, 2019

reilem Apr 9, 2019

tido64 Apr 9, 2019

reilem Apr 9, 2019

tido64 Apr 10, 2019

krizzu commented Apr 9, 2019

reilem commented Apr 9, 2019 •

edited

Loading

krizzu commented Apr 10, 2019

reilem commented Apr 10, 2019

reilem commented Apr 10, 2019

krizzu commented Apr 10, 2019

tido64 left a comment

krizzu left a comment •

edited

Loading

krizzu commented Apr 11, 2019

krizzu commented Apr 12, 2019

iOS clean migration from RNCAsyncStorage_V1 to RTCAsyncStorage_V1 #64

iOS clean migration from RNCAsyncStorage_V1 to RTCAsyncStorage_V1 #64

Conversation

reilem commented Apr 9, 2019

Summary:

Test Plan:

krizzu commented Apr 9, 2019

tido64 Apr 9, 2019

Choose a reason for hiding this comment

reilem Apr 9, 2019

Choose a reason for hiding this comment

tido64 Apr 9, 2019

Choose a reason for hiding this comment

reilem Apr 9, 2019

Choose a reason for hiding this comment

tido64 Apr 10, 2019

Choose a reason for hiding this comment

krizzu commented Apr 9, 2019

reilem commented Apr 9, 2019 • edited Loading

krizzu commented Apr 10, 2019

reilem commented Apr 10, 2019

reilem commented Apr 10, 2019

krizzu commented Apr 10, 2019

tido64 left a comment

Choose a reason for hiding this comment

krizzu left a comment • edited Loading

Choose a reason for hiding this comment

krizzu commented Apr 11, 2019

krizzu commented Apr 12, 2019

reilem commented Apr 9, 2019 •

edited

Loading

krizzu left a comment •

edited

Loading