feat: Allow brownfield iOS apps to handle storage.

[tido64]

Summary:

Adds a delegate property to RNCAsyncStorage to allow custom handling of storage.

Implements #33.

Test Plan:

To manually test the delegate, shake the Simulator (^⌘Z) and toggle
"Set AsyncStorage Delegate". Otherwise, tests for JS have been duplicated for
the native delegate.

[tido64]

@krizzu: How are you testing RNCAsyncStorage? I had to patch AsyncStorage.js to get the correct implementation:

diff --git a/lib/AsyncStorage.js b/lib/AsyncStorage.js
index 4769097..6c007d8 100644
--- a/lib/AsyncStorage.js
+++ b/lib/AsyncStorage.js
@@ -18,6 +18,7 @@ const {NativeModules} = require('react-native');
 const RCTAsyncStorage =
   NativeModules.AsyncRocksDBStorage ||
   NativeModules.RNC_AsyncSQLiteDBStorage ||
+  NativeModules.RNCAsyncStorage ||
   NativeModules.AsyncLocalStorage;

 type ReadOnlyArrayString = $ReadOnlyArray<string>;

[tido64]

@krizzu: Also, it seems that in making it possible to run from Xcode, I broke Detox builds. Any suggestions here?

[tido64]

Never mind. Figured out why Detox broke. 😁

[tido64]

For the first question: #37

[krizzu]

@tido64
This had to come up when we've changed naming on iOS side (some issues with camel case naming).

Your changes are already merged, so feel free to rebase.

[tido64]

@krizzu Thanks 👍

[tido64]

@krizzu Let me know how I can make this PR better 😉

[krizzu]

@tido64 Great, thanks!

I'm going to need a second pair of eyes on iOS code, but overall looks good. We could have some documentation about new feature - ideally in new doc file in docs directory.

Also, some tests would be good to check it too.

[krizzu]

@tido64 Sorry for conflict that arised from merging #46.

[zhongwuzw]

@krizzu LGTM. I think we only need to add some tests and docs. 🤔

[tido64]

@krizzu @zhongwuzw I added docs and tests. Please have a look 😄

[krizzu]

@tido64 Thanks! I'll have a look once I find a time later today

[krizzu]

Hey @tido64 , thanks for your awesome work!

I left few comments/suggestion that I think would be great to consider.

Also, I might be missing here somewhere, but I assumed that this "delegation" would work as a "plugin" for iOS module, where you change underlying implementation of AsyncStorage - you provide "object" with methods named same as original AsyncStorage , so those would be used instead.

In here, I believe you created a delegate in App delegate for testing purposes, correct?

thanks.

[tido64]

Thanks for taking a look, @krizzu. I'll try to get to these within the week.

I'm not sure I follow your questions so bear with me 😄

• Yes, a delegate is basically just an object with named functions. The interface is usually defined by the one that will be calling it.
• The methods of AsyncStorage are never replaced. But it can be redirected to call the delegate instead. And if the passthrough property is set, the methods of AsyncStorage are still called.
• In the test app, the AppDelegate implements this interface for testing purposes. In a normal app, usually another object would implement it.

[krizzu]

Great work! Thanks you! 💪 🚀

[stigi]

Hi team,

just a heads up that this breaks native linking via cocoapods, as the library is linked as react-native-async-storage and expects imports of headers as #import <react-native-async-storage/RNCAsyncStorageDelegate.h> and not #import <RNCAsyncStorage/RNCAsyncStorageDelegate.h>.

There are two ways to move forward:

• change the import to #import <react-native-async-storage/RNCAsyncStorageDelegate.h>
• rename the pod (name of .podspec file and value of s.name) to RNCAsyncStorage

Let me know, I'd be happy to put up a PR

[krizzu]

@stigi Thanks for heads up!

I'm happy to go with the first approach. WDYT @tido64 ?

[stigi]

The second approach also requires the current user of cocoa pod linking to update their Podfiles, so option 1 is less intrusive, even though cringe seeing the import from <react-native-async-storage...>

[tido64]

Sorry for not testing this thoroughly with Pods. Completely slipped my mind. I’m for solution #2. Yes, it will break on upgrade but it’s the right thing to do and will be in line with Apple’s naming guidelines and what the community will expect. Thanks for bringing it to our attention and fixing it ❤️ Sent from ProtonMail Mobile

…

On Fri, Apr 12, 2019 at 17:21, Ullrich Schäfer ***@***.***> wrote: The second approach also requires the current user of cocoa pod linking to update their Podfiles, so option 1 is less intrusive, even though crunch seeing the import from <react-native-async-storage...> [:neckbeard:] — You are receiving this because you were mentioned. Reply to this email directly, [view it on GitHub](#35 (comment)), or [mute the thread](https://github.com/notifications/unsubscribe-auth/AD7rVnZtqlr7HhSr614dNyO8KtU6OvZwks5vgKRegaJpZM4bpmXH).

[lightman73]

First, thank you, you just saved me. I spent the last hour trying to understand why the hell Xcode wasn't finding RNCAsyncStorageDelegate.h ...

I manually implemented solution #1 and it's building everything as expected.

That said, I vote for #2 as well. Although it is breaking on upgrade, it's the cleanest solution, imho.

Thank again !

[krizzu]

Okay, seems like we made a decision.
Just to be clear: what steps would have to be taken to successfully upgrade with solution 2?

[tido64]

Somebody correct me if I'm mistaken, but if you're using react-native link, isn't it just a matter of re-linking it? We use manual linking at work so we would just fix the relevant line in our Podfile.

[tido64]

@krizzu @stigi Sorry, I got a little impatient since I broke this release. I've implemented the fix here: #75. I've checked that react-native link will pick up the new .podspec but it won't clean up the old one. We will unfortunately have to ask users to go into their Podfile and make manual changes. 😞