Added the ability to set the storage location on iOS #18

adamivancza · 2019-02-23T11:59:00Z

Moved my changes from this PR: https://github.com/facebook/react-native/pull/20904/files
I didn't find any tests in the repo atm so I wasn't able to add those. I can see a PR with detox tests though - if that's merged I can add some tests.

krizzu · 2019-02-23T18:57:17Z

Hey @adamivancza, thank you for your input!

I'm going to hold off merging this PR till I finish #15 - I'd love to have some tests on new functionality, hope you don't mind waiting.

adamivancza · 2019-02-24T19:46:09Z

sure thing @krizzu - could you ping me here once you've finished with #15?

krizzu · 2019-02-25T13:58:32Z

@adamivancza Hey,
Detox is merged, so you can rebase your branch. Would be nice if you could add some tests for storage directory changes.

Also, we don't have generated docs from comments yet, so could you please add this method to docs, mark it as iOS only and describe its functionality and purpose?

Thanks a lot

adamivancza · 2019-02-26T11:04:51Z

thx @krizzu - will do those over the weekend.

zhongwuzw · 2019-03-13T09:34:31Z

ios/RNCAsyncStorage.m

@@ -16,9 +16,25 @@
 #import "RCTLog.h"
 #import "RCTUtils.h"

+typedef NS_ENUM(NSInteger, RCTStorageLocation) {
+  Documents,
+  ApplicationSupport


@krizzu Is this what we want? only support Documents and ApplicationSupport directories?

It's been discussed in #13, where ApplicationSupport is more safe to store app-specific data.

Do you have any suggestion if there should be more/less options to choose from?

@krizzu Oh, I scan those things, so now I think we may need to handle two things:

Provide options for user to save data. (I suppose support three options, Documents,Library,tmp, https://developer.apple.com/library/archive/documentation/FileManagement/Conceptual/FileSystemProgrammingGuide/FileSystemOverview/FileSystemOverview.html#//apple_ref/doc/uid/TP40010672-CH2-SW1, tmp is also meaningful IMO, provide temp storage).

Not backed up by iTunes. (For the Application Support directory, it would backed up by iTunes and iCloud, so if we use that, we need use NSURLIsExcludedFromBackupKey for files we stored includes manifest file.

not sure about tmp - do you have any use cases where it might be helpful? I think that would just cause confusion as tmp dir can be deleted deleted by OS at any time.

maybe we can add a flag to control that? it could be useful to have this file backed up.

zhongwuzw · 2019-03-14T03:30:20Z

ios/RNCAsyncStorage.m

@@ -455,4 +489,10 @@ - (NSDictionary *)_writeEntry:(NSArray<NSString *> *)entry changedManifest:(BOOL
  }
 }

+RCT_EXPORT_METHOD(setStorageLocation:(RCTStorageLocation)location)
+{
+  storageLocation = location;


We can have equal check in here.

zhongwuzw · 2019-03-14T03:34:35Z

ios/RNCAsyncStorage.m

@@ -16,9 +16,25 @@
 #import "RCTLog.h"
 #import "RCTUtils.h"

+typedef NS_ENUM(NSInteger, RCTStorageLocation) {
+  Documents,


Can we conform Apple naming style?

# Conflicts: # ios/RNCAsyncStorage.m

adamivancza · 2019-03-16T18:00:04Z

Sorry about this but I still haven't got time to finish up the tests - will try to find some time to do them.

…act-native-async-storage # Conflicts: # ios/RNCAsyncStorage.m

…act-native-async-storage # Conflicts: # example/e2e/asyncstorage.e2e.js

adamivancza · 2019-04-11T10:28:03Z

hey @krizzu - finally finished with the PR! :)

krizzu

This looks great 👏 I left two comments regarding some functionality. Please let me know what you think.

Also, I believe it'd be nice to document this feature and explain reasons why we added it.

thanks.

krizzu · 2019-04-11T14:12:46Z

lib/AsyncStorage.js

+   * Call this set the storage location.
+   * @param storageLocation The storage location. Can be 'documents' or 'applicationSupport'.
+   *
+   * **Note**: changing this only sets the location for future actions and does not migrate


This bit is bit worrying. We had similar issue #55, where storage location was changed and devs were losing data. Maybe we could come up with migration plan for data?

The default locations is still the same, imo devs can only loose data once updated to a new version which contains this change AND change the storage location specifically without copying their data.

I see, thanks. We should provide a note in docs that this can happen, so they know upfront.

krizzu · 2019-04-11T14:13:26Z

lib/AsyncStorage.js

+  },
+
+  StorageLocationIOS: {
+    documents: 'documents',


I think we could export constants to be used in JS, what do you think?

oh yeah that's a great shout! will do that

krizzu · 2019-04-11T18:59:59Z

Hey @adamivancza, thank you for your work so far 👏

We merged another PR which contains some native iOS code, I hope this won't give you much trouble.

reilem · 2019-04-14T08:10:42Z

@krizzu There are some conflicting changes with my fix, mainly with RCTGetStorageDirectory and RCTGetManifestFilePath. This branch makes some changes to those functions (that I don't entirely agree with). In current master I have made RCTCreateManifestFilePath and RCTCreateStorageDirectoryPath specifically in case you need a location that isn't the default. I would use these Create functions in conjunction with the already existing RCTGetManifestFilePath and RCTGetStorageDirectory. Otherwise the dispatch_once functionality might cause unpredictable behaviour.

krizzu · 2019-06-18T17:25:15Z

Hey @adamivancza,

Thank you so much for work you put into this PR, but I'm sorry to say that I'm going to close it. We're moving into Async Storage v2, where we do a lot of breaking changes.

There's a plan to still support the current implementation of AS as another Backend Storage in v2 - maybe you'd like to take part in it? We plan to create it from the ground up, so I believe it'd best time to add additional functionality (not preset in AS before)

thanks.

Added the ability to set the storage location on iOS

81346a8

krizzu assigned adamivancza Feb 26, 2019

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

zhongwuzw reviewed Mar 13, 2019

View reviewed changes

zhongwuzw reviewed Mar 14, 2019

View reviewed changes

adamivancza added 3 commits March 16, 2019 18:34

Merge commit 'd68889dbccd2c34e8928ae8a2d24af740c30707b'

20d18e6

# Conflicts: # ios/RNCAsyncStorage.m

ESLint fixes

3f4f92a

Code review fixes

75823da

adamivancza added 5 commits March 28, 2019 13:14

Merge branch 'master' of https://github.com/react-native-community/re…

5041f58

…act-native-async-storage # Conflicts: # ios/RNCAsyncStorage.m

UITests - WIP

7d96ab7

UITests finished

3b63039

Merge branch 'master' of https://github.com/react-native-community/re…

3d0c10c

…act-native-async-storage # Conflicts: # example/e2e/asyncstorage.e2e.js

Flow & ESLint fixes

17489ab

krizzu requested changes Apr 11, 2019

View reviewed changes

krizzu added LEGACY labels Jun 16, 2019

krizzu closed this Jun 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added the ability to set the storage location on iOS #18

Added the ability to set the storage location on iOS #18

adamivancza commented Feb 23, 2019

krizzu commented Feb 23, 2019 •

edited

Loading

adamivancza commented Feb 24, 2019

krizzu commented Feb 25, 2019 •

edited

Loading

adamivancza commented Feb 26, 2019

zhongwuzw Mar 13, 2019

krizzu Mar 13, 2019

zhongwuzw Mar 14, 2019

adamivancza Mar 16, 2019

zhongwuzw Mar 14, 2019

zhongwuzw Mar 14, 2019

adamivancza commented Mar 16, 2019

adamivancza commented Apr 11, 2019

krizzu left a comment

krizzu Apr 11, 2019

adamivancza Apr 11, 2019

krizzu Apr 11, 2019

krizzu Apr 11, 2019

adamivancza Apr 11, 2019

krizzu commented Apr 11, 2019

reilem commented Apr 14, 2019

krizzu commented Jun 18, 2019

Added the ability to set the storage location on iOS #18

Added the ability to set the storage location on iOS #18

Conversation

adamivancza commented Feb 23, 2019

krizzu commented Feb 23, 2019 • edited Loading

adamivancza commented Feb 24, 2019

krizzu commented Feb 25, 2019 • edited Loading

adamivancza commented Feb 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamivancza commented Mar 16, 2019

adamivancza commented Apr 11, 2019

krizzu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krizzu commented Apr 11, 2019

reilem commented Apr 14, 2019

krizzu commented Jun 18, 2019

krizzu commented Feb 23, 2019 •

edited

Loading

krizzu commented Feb 25, 2019 •

edited

Loading