Add ActivityView callback (completion handler)

[cmcewen]

Thanks for creating this - wanted to do some tracking on whether a user completed an activity, so I implemented a callback with the results of the ActivityView. It's a bit hacky to make the callback optional, but I didn't want to break the API. If this looks good I'll update the Readme to explain more.

[chirag04]

if callback is optional then is this invariant required?

[naoufal]

@cmcewen Thanks for your contribution. Can you confirm that this resolves #14?

[cmcewen]

Hi @chirag04 and @naoufal - apologies for taking so long to follow up on this. I reworked the PR so that it adds a new function, mainly due to the fact that there is no way to include an optional callback nicely. This does resolve #14, and also includes the activityType selected by the user in the callback. It also removes the invariant requirement based on your comments.

[naoufal]

@cmcewen I've been thinking about this and I think I might prefer using the EventEmitter for this. Something like this.

Reason is, I'd like to avoid have two methods that have similar functionality. React Native also uses something similar for Keyboard Events.

Would this implementation address the use case you were addressing with this PR?

[cmcewen]

@naoufal it would be possible to use the event emitter, but I'm not sure that is an appropriate API for this use case.

iOS natively has both completion handlers and the NSNotificationCenter, which I think correspond to callbacks and events. In this case the activity view uses a completion handler, and I think it would make more sense to use a callback on the JS side as well.

The activity view brings up a separate UI, and there is no possible interaction until either an action is taken or it is cancelled, so I feel like it makes sense to handle that interaction as a callback.

On the other hand, something like a keyboard typing or a movie playing (which I know natively requires subscribing to notifications) seems like a better fit to use events.

I agree that having two separate methods that act similarly is not ideal. In that case, I suggest either the callback is a required parameter, or an empty callback is automatically included (which is what the first version of this PR used)

[naoufal]

Fair enough.

In that case, let's pass the callback in the options object of the show method.

[ilanasufrin]

Is this still being actively worked on?

[cmcewen]

@ilanasufrin I've just been using my fork - at this point the functionality is working fine, just figuring out the right API

@naoufal passing in a function in options still feels a bit weird to me - also i'm not sure exactly how RCTConvert works with functions not passed in with the function call. How about returning a promise? AsyncStorage uses Promises (https://github.com/facebook/react-native/blob/master/Libraries/Storage/AsyncStorage.js#L44) and it seems like other pieces of core are being refactored into promises as well facebook/react-native#5512

[grabbou]

I think we could just change show(opts) to be show(opts, callback) and that should do the trick (callback is optional). I have decided to quickly submit my take on that, hope you don't mind @cmcewen 🔥

[cmcewen]

@grabbou that was actually the very first version of this PR I submitted #21 (comment)

[tomprogers]

Is there any interest in resolving the conflicts and merging this down? I still have a need for this functionality.