AsyncStorage.multiRemove method has different behavior on Android and iOS

[jhoobergs]

When calling the AsyncStorage.multiRemove method with an empty array, the behavior on Android and iOS is different:

• Android: throw as error (https://github.com/react-native-community/react-native-async-storage/blob/master/android/src/main/java/com/reactnativecommunity/asyncstorage/AsyncStorageModule.java#L263)
• iOS: does nothing (https://github.com/react-native-community/react-native-async-storage/blob/master/ios/RNCAsyncStorage.m#L409)

I created this issue at the facebook/react-native repo: facebook/react-native#23412 where I was told to create it at this repo.

[krizzu]

Thanks @jhoobergs for raising this issue. Good catch!
I think we should throw an error on iOS too, to have the same behavior on both platforms. Would like to open a PR with fix for that?

[hawkrives]

I personally don't see why calling multiRemove with an empty array should be an error - it just means that I'm not asking it to remove anything.

I'm curious as to why you feel that throwing an error is the correct default?

I agree that we should align the platforms!

[jhoobergs]

The current error message on Android is also a bit strange: "Error: Invalid key" without a key name because there is no key. We got a lot of these errors in our sentry and couldn't figure out where they came from.

I think @hawkrives is right about not throwing an error on empty arrays.

[pbitkowski]

I prefer to get feedback about actions. If not get an error then maybe boolean value would be a way to go?

To solve this issue we need to add validation to RNCAsyncStorage.m, which you can find here. Check if an array of keys is empty and then throw an error.

@jhoobergs would you like to fix it and send PR?

I think that we should unify the way in which we validate inputs for all methods. The API behavior should be consistent.

WDYT @krizzu ?

[jhoobergs]

I have no experience with Objective C, so I can't really fix it.

[krizzu]

I'm up for having feedback from actions - throwing an error tells us we could do something wrong. We can also make those Errors more meaningful, with hints of what might fix the issue (like "Check if the array is not empty" ).

@pbitkowski Feel free to post PR

[hawkrives]

Sure, but the API contract of "multiRemove" is "delete all of these things" - why special-case having zero items?

I don't particularly relish having to add application-level checks to make sure I'm not calling with an empty array; I often build up a list of keys from some other data, and if multiRemove throws on an empty array, I'd need to check before I call it.

That just seems like an odd design choice to me.

[krizzu]

@hawkrives Thank you for the feedback, I understand your concern.

All of AsyncStorage methods are rejecting promise if something bad happens. That's why I'd suggest to wrap every AsyncStorage method call into try/catch, if using it in async function. Otherwise, callback is being called with error. It simply looks like empty array check is just missing from iOS.

[pbitkowski]

@krizzu I'm going to prepare such PR soon.

[pbitkowski]

I thought about it @krizzu, and the keys parameter in multiRemove is declared as Array<string>. An array can be empty. So maybe we should rather eliminate an error on Android side than create a new one on iOS side? What do you think?

https://github.com/react-native-community/react-native-async-storage/blob/65341b8d26ccbe8e9c21d2d1e7b340888668c72d/lib/AsyncStorage.js#L289

We've got the same situation with multiSet.

[krizzu]

@pbitkowski @hawkrives @jhoobergs
I've come to realize that it would make more sense to have multiRemove not throwing an error on an empty array. The same principle can be applied to for multiSet.

If you'd like to help with it, PR that deletes array length check on android would be awesome.

Thanks for your input!

[krizzu]

🎉 This issue has been resolved in version 1.1.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀