Remove strong references for delegates and completion handlers

[jw2175]

@belemaire Do you know if we need to dispatch calling the transaction completion handler on the main queue?

[belemaire]

@jw2175

If we want to remain consistent with how it's done for Android, I think we should still keep dispatching to main queue.

I'm not strongly skilled in iOS, so I'm not sure if it behaves the same way as Android, but basically we did it for Android as a convenience so that the response (either success or failure) will come back on the main UI thread so that in case the client response handler has to execute some code on UI thread (any code updating UI has to), there is no need to manually transition to it. For iOS I'm assuming it's kind of similar, dispatch to main queue = main UI thread.

If you remove this, we lose parity with Android. Also I think this would be a breaking change as it will break all response handlers that are performing operations that can only be done on the main thread, and weren't transitioning to it by themselves (because response was done on main thread implicitly, now its not).

Ideally we should maybe by default, dispatch the response on the same thread that initiated the request, be it main or separate one. Need to look at how other popular libraries are doing it, what's the common practice there. We might not be doing thing the proper way.

EDIT
Did a quick search, and that's indeed how I remember things to be done in popular networking libraries (or image fetching libraries .. whatever involves a request/response mecanism). Dispatching response to main queue by default as its what's more convenient for the client. (for example Alamofire). The same is true for AFNetworking ... Haven't look at other libs but would be interesting to know.
Altough they do offer options in addition to defaulting to main queue. We don't ... That could be something to explore as a potential improvement to the bridge (both for Android and iOS), but would require some research/design/implementation/testing, so def not light work.

Anyway, I guess I'm OK with the rest of the code changes, but removing dispatch to main queue for responses, could (will) I think break quite some clients calls... This is definitely a breaking change. In any case we should not remove the default to main queue I think, which is kinda the 'standard' and what's expected by the client for such kind of libraries. We should instead offer other options apart from this default operating mode (but much more work involved).

[jw2175]

@belemaire

thanks for detailed response. so just some explanation of how we got here:

motivation is to remove strong references to delegate properties. with id<ElectrodeFailureMessage> failureMessage of class ElectrodeBridgeResponse being the example here. in making this reference "weak", when the block executes, response.failureMessage will be nil because the reference to response is out of scope at this point. Also, luckily two of the unit tests caught this. I suppose we can make a temporary strong local reference to failureMessage and then reference this in the block instead.

ElectrodeBridgeFailureMessage *failureMessage = response.failureMessage; dispatch_async(dispatch_get_main_queue(), ^{ transaction.completion(nil, failureMessage); });

@nalex87 would this be safe in your opinion?

[kobexing86]

@belemaire

thanks for detailed response. so just some explanation of how we got here:

motivation is to remove strong references to delegate properties. with id<ElectrodeFailureMessage> failureMessage of class ElectrodeBridgeResponse being the example here. in making this reference "weak", when the block executes, response.failureMessage will be nil because the reference to response is out of scope at this point. Also, luckily two of the unit tests caught this. I suppose we can make a temporary strong local reference to failureMessage and then reference this in the block instead.

ElectrodeBridgeFailureMessage *failureMessage = response.failureMessage; dispatch_async(dispatch_get_main_queue(), ^{ transaction.completion(nil, failureMessage); });

@nalex87 would this be safe in your opinion?

This looks fine to me. can we try it out?

[kobexing86]

LGTM

[belemaire]

LGTM