[Bridge] Order of callbacks on native bridge modules is backwards

[ide]

The JS (BatchedBridgeFactory & MessageQueue) expects the last two arguments of async bridge methods to be errorCallback, successCallback:

        var onSucc = hasSuccCB ? lastArg : null;
        var onFail = hasErrorCB ? secondLastArg : null;

but the RCTBridgeModules define successCallback:errorCallback:. This isn't causing any issues - pointing it out mostly in case it causes confusion down the road.

[vjeux]

Good catch. @jordwalke attempted to always have error callback THEN success callback. The motivation was to never forget the error callback but this introduced more confusion than anything and didn't pick up. There are still some remaining of this attempt here and there.

[sahrens]

There is no way to enforce which is which so any method can rename them how it wants. We should probably just rename the vars in the bridge impl to cb1 and cb2.

On Mar 24, 2015, at 8:59 AM, Christopher Chedeau [email protected] wrote:

Good catch. @jordwalke attempted to always have error callback THEN success callback. The motivation was to never forget the error callback but this introduced more confusion than anything and didn't pick up. There are still some remaining of this attempt here and there.

—
Reply to this email directly or view it on GitHub.

[ide]

It might be worth keeping some structure and documenting the (successCallback, errorCallback) convention. One benefit of the convention is that it makes the migration towards Promises / ES7 async functions (the discussion in #172) easier. Concretely the migration layer could roughly look like this:

function promisifyLegacyBridgeMethodThatHonorsConvention(method) {
  return function() {
    var context = this;
    var args = Array.slice.call(arguments);
    return new Promise((resolve, reject) {
      // Assume last two args to the legacy method are the success and error callbacks
      // by convention... might need a wrapper function that massages the args into a form
      // that resolve/reject expect but this is the gist of the idea
      args.push(resolve);
      args.push(reject);
      method.apply(context, args);
    });
  };
}

[ide]

Fixed by #1232 / 90439ce. The callbacks in the bridge code consistently go (successCallback, errorCallback), which now matches most of the built-in native modules. And with promises the callback arguments are entirely hidden.