Skip to content

Gracefully handle the case where inputs to multiSet are not of the type Array<Array<String>> #519

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
bushshrub opened this issue Jan 8, 2021 · 6 comments · Fixed by #573
Labels
enhancement New feature or request

Comments

@bushshrub
Copy link

Motivation

At present, React Native applications will crash when AsyncStorage.multiSet is called with invalid arguments, for example, AsyncStorage.multiSet(['a', 'b'], ['c', 'd']). Note that in this case, there are 2 arguments provided to the function for the keyValuePairs required to store as data. However, this happens to be a typo and the developer actually intended to wrap it with square brackets, but failed to do so.

Since there is no error thrown, the application literally just crashes and makes debugging almost impossible.

Description

An exception should be raised when the type of the inputs to the keyValuePairs argument of multiSet is incorrect, instead of simply letting the application crash.

New feature implementation

Type comparison of the first argument would be crucial in correcting this.

@tido64 tido64 added the enhancement New feature or request label Feb 3, 2021
@kiranjd
Copy link
Contributor

kiranjd commented Mar 27, 2021

I would like to work on this

@tido64
Copy link
Member

tido64 commented Mar 27, 2021

@kiranjd: Please go ahead and submit a PR 😃

@bushshrub
Copy link
Author

@kiranjd Another possibility would be to handle the case where multiSet is called with n arguments

@kiranjd
Copy link
Contributor

kiranjd commented Mar 28, 2021

@xiurobert That approach might not work as the second arg could be passed as well.
I'm thinking of checking if the second type is an array. If so, show the relevant warning message. What do you think?

@kiranjd
Copy link
Contributor

kiranjd commented Mar 28, 2021

As I looked to add regression tests, I see that multiSet, muliGet, and a few others are not covered in e2e tests. Hence, I have skipped tests as it would be outside the scope for the current PR.

Maybe we can create adding tests as issues and then add them incrementally?

@krizzu
Copy link
Member

krizzu commented Apr 2, 2021

🎉 This issue has been resolved in version 1.15.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@krizzu krizzu added the released label Apr 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants