[FIX] Move the storage location on iOS to Application Support #274

HeyImChris · 2020-01-04T00:01:14Z

Summary:

Right now on iOS, the data is stored in the Documents directory (NSDocumentDirectory) on iOS and the caches directory (NSCachesDirectory) on tvOS.

These are not the best locations for the following reasons:

NSDocumentDirectory- this folder is visible on iOS in scenarios where you can view the Files structure. For instance, in Word/PPT/Excel, a user can tap through to this and see a folder titled "RCTAsyncLocalStorage_V1".
NSCachesDirectory- Apple documents this as a location that can get purged in low memory scenarios "the system may delete the Caches directory on rare occasions when the system is very low on disk space."

This fix moves the location to the Application Support directory (NSApplicationSupportDirectory). This directory is documented by Apple as "[containing] all app-specific data and support files" and it can safely contain user data. There is not risk of this purging in low memory situations and this isn't visible to iOS users.

This is not a breaking change as we are able to maintain fidelity with the one-time data migration code to our new location.

The general fix is this:

Maintain our existing deprecation/removal of the old Documents/.../RNCAsyncLocalStorage_V1/manifest.json
Maintain our existing migration from Documents/.../RNCCreateStorageDirectoryPath to Documents/.../RCTAsyncLocalStorage_V1/manifest.json
Add an additional migration from Documents/.../RCTAsyncLocalStorage_V1/manifest.json to Application Support/[bundleID]/RCTAsyncLocalStorage_V1/manifest.json, but do not delete the Document version
In the rewrite of RCTCreateStorageDirectoryPath, use NSApplicationSupportDirectory instead of caches or documents
Refactor our migration methods to take in a from and a to directory path parameter so we can run the migration twice- once with the old caches/documents location paired with RCTOldStorageDirectory and again with the old caches/documents location paired with RCTStorageDirectory. We also added a shouldCleanupOldDirectory parameter so we can optionally delete the "from" path when we're migrating from Documents/RNC* but retain the "from" path when migrating from Documents/RCT*
Cache some duplicate method calls in a couple places to save on performance

Please see https://developer.apple.com/library/archive/documentation/FileManagement/Conceptual/FileSystemProgrammingGuide/MacOSXDirectories/MacOSXDirectories.html#//apple_ref/doc/uid/TP40010672-CH10-SW1 and https://developer.apple.com/library/archive/documentation/FileManagement/Conceptual/FileSystemProgrammingGuide/FileSystemOverview/FileSystemOverview.html#//apple_ref/doc/uid/TP40010672-CH2-SW1 for more documentation on this.

Test Plan:

Ran the example test app on iOS and incremented the counter. Verified this saves by hitting the Restart button and the content persisted.
Verified that the migration is correct on iOS by stashing this fix, incrementing the counter, popping back this change, and in the debugger I stepped through it copying the data over to the Application Support/[bundleID]/ location. I also verified that it correctly cleared out the old data. Finally, I checked that in the aforementioned migration scenario, there was no data loss in the test app.
Tested updating the manifest when there was no existing manifest in Documents/RCT*
Tested updating the manifest when there was no existing manifest in ApplicationSupport/RCT*
Tested updating the manifest when there was no existing manifest in Documents/RNC*
Tested updating the manifest when there was an existing manifest in Documents/RCT*
Tested updating the manifest when there was an existing manifest in ApplicationSupport/RCT*

tido64

LGTM 👍

@krizzu, do you want to take a look?

ios/RNCAsyncStorage.m

krizzu · 2020-01-07T14:30:06Z

Looks okay to me to.
@tido64 @HeyImChris WDYT about dropping the support for RNC* manifest file? I know that would be somehow a breaking change, but we could release this feature in 1.8.

tido64 · 2020-01-07T16:42:40Z

@krizzu, should that be a separate PR?

tido64 · 2020-01-07T17:03:23Z

@krizzu, I also just remembered that I had already done this, but it seems it was in a different file? Will we have to repeat this PR for v2? Seems to me v2 should just use this new directory right away.

HeyImChris · 2020-01-07T18:36:46Z

@krizzu, I also just remembered that I had already done this, but it seems it was in a different file? Will we have to repeat this PR for v2? Seems to me v2 should just use this new directory right away.

Haven't taken a look at V2, but agree that we should drop storage support in the caches and documents directories.

krizzu · 2020-01-08T13:45:51Z

@krizzu, should that be a separate PR?

Yes, I'd do that.

@krizzu, I also just remembered that I had already done this, but it seems it was in a different file? Will we have to repeat this PR for v2? Seems to me v2 should just use this new directory right away.

Haven't taken a look at V2, but agree that we should drop storage support in the caches and documents directories.

Yes please - v2 to have Application Support directory as default location.

Rotemy · 2020-01-30T10:07:45Z

When is this scheduled to be merged?

HeyImChris · 2020-01-30T17:09:36Z

When is this scheduled to be merged?

Thanks for your patience, been busy with other work recently. As we discussed on discord, I'll have to make another update where we don't delete the RCT* files. Will try and get that out today and can hopefully merge thereafter.

…nto or out of deleting deprecated storage locations

Palid · 2020-02-04T15:42:28Z

Hey, do you need any help with reviewing/merging this thing? I'd love to start working on adding app storage support for iOS, but it looks like this merge would immensly help me with this.

HeyImChris · 2020-02-04T18:22:23Z

Hey, do you need any help with reviewing/merging this thing? I'd love to start working on adding app storage support for iOS, but it looks like this merge would immensly help me with this.

@Palid - A review would be great. @krizzu had some great feedback that I added in the latest iteration. Once no one's waiting on it then I'd love for someone with permissions to merge it in.

…m/HeyImChris/async-storage into heyImChris/storageLocationFix_iOS

krizzu · 2020-02-07T13:01:37Z

ios/RNCAsyncStorage.m

+  RCTStorageDirectoryMigrationCheck(RCTCreateStorageDirectoryPath_deprecated(RCTOldStorageDirectory), RCTCreateStorageDirectoryPath_deprecated(RCTStorageDirectory), YES, YES);
+
+  // Then migrate what's in "Documents/.../RCTAsyncLocalStorage_V1" to "Application Support/[bundleID]/RCTAsyncLocalStorage_V1"
+  RCTStorageDirectoryMigrationCheck(RCTCreateStorageDirectoryPath_deprecated(RCTStorageDirectory), RCTCreateStorageDirectoryPath(RCTStorageDirectory), NO, NO);


Since you're passing YES, YES to first migration, then NO, NO to the second one, you can actually use one parameter for that, right?

We could, but they represent pretty different actions. Also it sounds like we're going to drop RNC* migration support soon anyway so this leads us to a pretty easy plan there (drop the last parameter and keep the other one)

krizzu

@tido64 Do you mind giving a second look on this?

HeyImChris · 2020-02-10T18:19:25Z

@tido64 Do you mind giving a second look on this?

Thanks for the reviews. If it looks good, does one of you have permissions to merge? Doesn't look like I can.

# Why react-native 0.71 removed the builtin async storage, to keep the backward compatibility, we should now vendor the *@react-native-async-storage/async-storage* close ENG-7190 # How - [tools] add @react-native-async-storage/async-storage to vendoring config - to support expo-go's scoped async storage, we should further patch react-native-async-storage. they are now in the vendoring patch. - previously on ios, we had the patch [in our fork](expo/react-native@c7d3f6d) - previously on android, we did this from [the transform tool for our fork](https://github.com/expo/expo/blob/9047ecfbe233924e4848722b518d8ce1fa8b7173/android/tools/src/main/java/host/exp/exponent/tools/ReactAndroidCodeTransformer.java#L215-L216) - the new react-native-async-storage saves data in a different storage, so they have the migration steps. to keep backward compatibility for existing expo-go. i skip the migrations and read the storage data from previous expo-go paths. - the new react-native-async-storage saves ios data in [*Application Support* rather than *Documents*](react-native-async-storage/async-storage#274). i don't want to follow the design. the reason is, besides backward compatibility, expo-go as a developer tool, it's good for developers to view the storage in *Documents*. - [ios] migrate the AsyncStorage native module from `RCTAsyncLocalStorage` to `RNCAsyncStorage` - [android] migrate `ExponentAsyncStorageModule` parent class to the new `AsyncStorageModule` - the new react-native-async-storage doesn't support turbo module, we have to move our integration from *ExpoTurboPackage* to *ExponentPackage* - the classic native module system respects the `@ReactMethod` annotation, we should add all of them in the `ExponentAsyncStorageModule`. # Test Plan - [ncl] AsyncStorage test case before this pr and after. makes sure the AsyncStorage value is kept after the changes.

Move the storage location on iOS to Application Support

7cb5d27

tido64 requested review from tido64 and krizzu January 6, 2020 08:47

tido64 approved these changes Jan 7, 2020

View reviewed changes

ios/RNCAsyncStorage.m Outdated Show resolved Hide resolved

ios/RNCAsyncStorage.m Outdated Show resolved Hide resolved

tido64 added the platform: iOS This is iOS specific label Jan 7, 2020

remove unnecessary dispatch_once calls

ad83d34

HeyImChris and others added 2 commits January 31, 2020 16:07

Add sequential migration and optional clearData flags so we can opt i…

0c4153d

…nto or out of deleting deprecated storage locations

Merge branch 'LEGACY' into heyImChris/storageLocationFix_iOS

4e281b3

HeyImChris added 2 commits February 6, 2020 11:10

Option to not overwrite ApplicationSupport data

1f01ddc

Merge branch 'heyImChris/storageLocationFix_iOS' of https://github.co…

a7a85ed

…m/HeyImChris/async-storage into heyImChris/storageLocationFix_iOS

krizzu reviewed Feb 7, 2020

View reviewed changes

merge two flags into one

17ac36a

krizzu approved these changes Feb 9, 2020

View reviewed changes

tido64 approved these changes Feb 10, 2020

View reviewed changes

tido64 merged commit 4e49db6 into react-native-async-storage:LEGACY Feb 10, 2020

tido64 mentioned this pull request Feb 13, 2020

Visible RCTAsyncLocalStorage_V1 folder in IOS while i allow user to see Documents Directory to access Downloaded files. #222

Closed

southerneer mentioned this pull request Mar 5, 2020

Bump @react-native-community/async-storage from 1.7.1 to 1.8.1 hippware/rn-chat#4862

Closed

snyk-bot mentioned this pull request Apr 16, 2020

[Snyk] Upgrade @react-native-community/async-storage from 1.7.1 to 1.8.1 BlueWallet/BlueWallet#991

Merged

euharrison mentioned this pull request Jul 29, 2020

[iOS] Migrate database location Nozbe/WatermelonDB#776

Closed

Kudo mentioned this pull request Jan 11, 2023

[go] vendor react-native-async-storage expo/expo#20780

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FIX] Move the storage location on iOS to Application Support #274

[FIX] Move the storage location on iOS to Application Support #274

HeyImChris commented Jan 4, 2020 •

edited

Loading

tido64 left a comment

krizzu commented Jan 7, 2020

tido64 commented Jan 7, 2020

tido64 commented Jan 7, 2020 •

edited

Loading

HeyImChris commented Jan 7, 2020

krizzu commented Jan 8, 2020

Rotemy commented Jan 30, 2020

HeyImChris commented Jan 30, 2020

Palid commented Feb 4, 2020

HeyImChris commented Feb 4, 2020

krizzu Feb 7, 2020

HeyImChris Feb 7, 2020

krizzu left a comment

HeyImChris commented Feb 10, 2020

[FIX] Move the storage location on iOS to Application Support #274

[FIX] Move the storage location on iOS to Application Support #274

Conversation

HeyImChris commented Jan 4, 2020 • edited Loading

Summary:

Test Plan:

tido64 left a comment

Choose a reason for hiding this comment

krizzu commented Jan 7, 2020

tido64 commented Jan 7, 2020

tido64 commented Jan 7, 2020 • edited Loading

HeyImChris commented Jan 7, 2020

krizzu commented Jan 8, 2020

Rotemy commented Jan 30, 2020

HeyImChris commented Jan 30, 2020

Palid commented Feb 4, 2020

HeyImChris commented Feb 4, 2020

krizzu Feb 7, 2020

Choose a reason for hiding this comment

HeyImChris Feb 7, 2020

Choose a reason for hiding this comment

krizzu left a comment

Choose a reason for hiding this comment

HeyImChris commented Feb 10, 2020

HeyImChris commented Jan 4, 2020 •

edited

Loading

tido64 commented Jan 7, 2020 •

edited

Loading