Create [bundle ID] folder during migration if none exists #303
Conversation
|
Please can we get this merged? Currently we can't update to 1.8.0 due to the failed migration. |
ios/RNCAsyncStorage.m
Outdated
| if (error == nil) { | ||
| RCTStorageDirectoryMigrate(oldDirectoryPath, newDirectoryPath, shouldCleanupOldDirectory); | ||
| return; | ||
| } |
There was a problem hiding this comment.
| } | |
| } else { | |
| RCTStorageDirectoryMigrationLogError(@"Failed to create storage directory during the migration.", error); | |
| return; | |
| } |
Let's warn about directory creation fail
Co-Authored-By: Krzysztof <[email protected]>
|
We are really looking forward to this fix, thanks for the work done! |
|
@HeyImChris Hey, would you mind having a look here? |
|
Sorry guys! I'm a fool. I checked the wrong branch. Checked from the correct migration-fix branch, it worked! Thanks! I’ll delete my comment so as not to embarrass people. |
| attributes:nil | ||
| error:error]; | ||
| } | ||
|
|
||
| static void RCTStorageDirectoryMigrate(NSString *oldDirectoryPath, NSString *newDirectoryPath, BOOL shouldCleanupOldDirectory) | ||
| { | ||
| NSError *error; | ||
| // Migrate data by copying old storage directory to new storage directory location | ||
| if (![[NSFileManager defaultManager] copyItemAtPath:oldDirectoryPath toPath:newDirectoryPath error:&error]) { |
There was a problem hiding this comment.
What if we put a fileExistsAtPath: check before this and if false, call _createStorageDirectory. Then add an "error != nil" to the copyItemAtPath: check to make sure the fileExistsAtPath succeeded and leave the rest of the method unchanged?
Seems a little messy to try to copy and recursively respond to that failure instead of checking if we should create first, putting ourselves in a consistent state in all cases and going from there.
There was a problem hiding this comment.
hey chris.. i'm down to make this change but i was reading on the apple docs that its better to attempt an operation and respond than to try to predict on how an operation will go based on the file system's current state. https://developer.apple.com/documentation/foundation/nsfilemanager/1415645-fileexistsatpath
since this is a race condition issue and we are the only thing writing to that folder, it'll be okay.
i'll test and try to get something up within today
There was a problem hiding this comment.
Didn't know about that doc. I think what you're doing is good then!
|
Thanks a lot everyone! |
Summary:
fixes issue described in #301
Test Plan:
you can use my sample repo here: https://github.com/frankenthumbs/AsyncStorageUpgradePOC and use the repro steps but the gist is, on iOS: