react-native-dialogs
Move to promises and not use dismissListener as it causes aborts
Just wanted to share the code to move to promises instead of callbacks as discussed in #92
It contains some other changes also that are specific to my use case such as removing the dismissListener as it has been causing aborts in my app. I don't expect for the PR to merged. Just wanted to share the code for moving to promises. Please feel free to close the PR.
Hey @varungupta85 thanks for being so persistant here. I haven't got a chance yet to dive into this. Are you sure getting rid of dismissListener is the way to go? How can we detect dismiss now?
I also think removing dismissListener isn't right approach. It seems like a simple race condition we can fix. I don't even think we have to move to promise based. Just a correct small edit is needed I think.
One other way of fixing the problem could be to pass the onDismiss handler separately to the java code such that even if both the action handler and the onDismiss handlers are called, they are separate callbacks and hence won't run into the multiple callback invocation problem.
I can modify the PR as per above idea to pass two callbacks instead of one if you guys are okay with it.
@varungupta85 yes please do that.
In the mean time though how did testing out the branch here go, it should fix the crash when pressing on buttons for 99% certain - https://github.com/aakashns/react-native-dialogs/issues/92#issuecomment-412324888
@Noitidart I will update the PR as soon as I get time to work on it.
I was not able to test the branch you mentioned since it still contains the mCallbackConsumed flag which masks the problem. I couldn't reproduce the abort on my setup/phones even earlier.
Thanks @varungupta85 for your effort here! I'm real busy so i appreicate your help! I will be out all day today but will follow up to see your work.
@Noitidart As I mentioned in the PR, I only submitted the PR to share what would be involved in using Promises in the native side also as that is a cleaner API. This PR contains changes that were specific to my requirements. So, I don't expect the PR to be merged but it can start off with someone interested in using Promises on the native side also.
I really appreciate that @varungupta85 - I would love to move to a promise based API on the native side. I actually am learning the native development by working on this.
Thanks Varun for being so consistent with this. I haven't merged yet because I don't totally understand the code, to see if it has any reprecussions. I'll have to sit down with the code soon. Or if anyone else is more familiar, would be awesome to get an analysis.