[android][expo-sharing] migrate to new modules API #20112
Conversation
expo-bot
added
bot: passed checks
| override fun onActivityResult(activity: Activity, requestCode: Int, resultCode: Int, data: Intent?) { | ||
| if (requestCode == REQUEST_CODE && pendingPromise != null) { | ||
| pendingPromise?.resolve(null) | ||
| pendingPromise = null | ||
| } | ||
| } |
There was a problem hiding this comment.
Overall, it looks great, but you can't just remove that function and resolve the promise earlier. The time when the promise will be resolved will be different.
Before:
js calls shareAsync -> intent is invoked -> share activity was opened -> user shares something -> execution goes back to the application -> the promise is resolved
Now:
js calls shareAsync -> intent is invoked -> promise is resolved
In many cases, the flow from the js perspective looks similar, because when the rn app goes to the background (when the shared activity is active), there is a small time window when the app code can still execute. However, it not always may be true.
There was a problem hiding this comment.
Yeah, I thought this mightn't be right. I'll put it back. So resolving with null is just a way of notifying the js that an action has been completed? You'd only need to do this in situations where it's some kind of callback? If it all completes inside the AsyncFunction body, you can just let the function complete without resolving if you are not returning anything?
There was a problem hiding this comment.
Yes, exactly. The AsyncFunction will automatically call promise.resolve(null) if the closure doesn't take the promise as an argument and the function is complete. In the case where a promise is the last argument of the function, you take responsibility for managing the promise itself. It's helpful when the function is completed, but the rest of the operation happens on a different thread.
There was a problem hiding this comment.
Great, that makes sense, thanks Łukasz 👍
Why
Migrate module to new modules API
How
Followed the usual steps involved in migrating a module
Test Plan
Tested in the bare expo app and all works as before. I removed onActivityResult as all it did was resolve the promise with null but I don't think this is necessary anymore with the new API. It can easily be put back if that was not correct.