[Bridge] Add support for JS async functions to RCT_EXPORT_METHOD

[ide]

Adds support for JS async methods and helps guide people writing native modules w.r.t. the callbacks. With this diff, on the native side you write:

RCT_EXPORT_METHOD(getValueAsync:(NSString *)key
                       resolver:(RCTPromiseResolver)resolve
                       rejecter:(RCTPromiseRejecter)reject)
{
  NSError *error = nil;
  id value = [_nativeDataStore valueForKey:key error:&error];

  // "resolve" and "reject" are automatically defined blocks that take
  // any object (nil is OK) and an NSError, respectively
  if (!error) {
    resolve(value);
  } else {
    reject(error);
  }
}

On the JS side, you can write:

var {DemoDataStore} = require('react-native').NativeModules;
DemoDataStore.getValueAsync('sample-key').then((value) => {
  console.log('Got:', value);
}, (error) => {
  console.error(error);
  // "error" is an Error object whose message is the NSError's description.
  // The NSError's code and domain are also set, and the native trace is
  // available under nativeStackIOS
});

And if you take a time machine or use Babel w/stage 1, you can write:

try {
  var value = await DemoDataStore.getValueAsync('sample-key');
  console.log('Got:', value);
} catch (error) {
  console.error(error);
}

Test Plan: Defined a sample async method:

RCT_EXPORT_METHOD(testAsync:(RCTPromiseResolver)resolve
                   rejecter:(RCTPromiseRejecter)reject)
{
  static BOOL toggle = NO;
  toggle = !toggle;
  if (toggle) {
    resolve(@"result");
  } else {
    reject(nil);
  }
}

Called it from JS to verify that the JS method returns a promise and is correctly resolved or rejected. In the rejection case, we get a JS Error object with the message passed from native (a default error message in this case since the NSError was nil), code, domain, and native stack trace.

Tested parameter verification by removing the rejecter parameter. The assertion in the bridge failed and printed a helpful error message with the name of the offending native class and selector.

Fixes #172
Makes #186 less relevant

[ide]

One thing I'd like to discuss is the macro API. I'm not very satisfied with RCT_EXPORT_NULLARY_PROMISE, which exists solely to support selectors that don't have any parameters since adding the resolver & rejecter parameters is syntactically different than when adding them to selectors with parameters (method -> method:(RCTPromiseRejecterBlock)reject... vs. method:(id)arg -> method:(id)arg __rct_rejecter:(RCTPromiseRejecterBlock)reject...). Maybe someone here knows a macro trick that would let us consolidate the macros.

Another idea is to require the resolver & rejecter parameters to be manually declared. I like the simplicity of this despite the extra typing.

RCT_EXPORT_PROMISE(getValueAsync:(NSString *)key
                        resolver:(RCTPromiseResolverBlock)resolve
                        rejecter:(RCTPromiseRejecterBlock)reject)
{
}

[If we were to go this route, I'd want to modify the JS bridge code that currently assumes the order of callbacks to be (onFail, onSucc) and change it to (onSucc, onFail) so that the Obj-C selectors take (resolve, reject) which matches the order of args when creating a JS Promise. I believe this is a safe change since the order isn't enforced and modules currently do whatever they want.]

[robertjpayne]

@ide I'd say go for the more verbose but declarative macro approach. Macros that begin to automagically make scoped variables available start to erode clarity and make debugging harder.

[ide]

Switched to the more explicit API for now. If we want implicit resolve/reject that can be added on top as another macro anyway. Usage now looks like:

RCT_EXPORT_PROMISE(getValueAsync:(NSString *)key
                        resolver:(RCTPromiseResolver)resolve
                        rejecter:(RCTPromiseRejecter)reject)
{
  resolve(@"whatever");
}

The onFail and onSucc callbacks in the message queue have now been flipped to be (onSucc, onFail) to match (resolve, reject).

[ide]

cc reviewers @nicklockwood @sahrens @vjeux

[vjeux]

@ide convinced me that it's the right way to model all our one-shot asynchronous api to use promises.

1. They are standard
2. They provide an explicit and standard way to handle errors
3. We can now use async/await to work with them!

Right now, we've got as many versions of asynchronous calls as there are ways to do it. Moving to Promise seems like a clear win all across the board from the JS side of things.

[vjeux]

It bothers me that we use a real Promise here because it executes the calls in a setImmediate and therefore changes the ordering of the responses. Non promises will be executed first, then react batch processing, then finally the promises but outside of the batch.

Can we instead vend a super dumb promise polyfill that executes callbacks right away?

[ide]

Just to make sure I understand what you're saying:

1. Non-promise callbacks are invoked. Also the resolve/reject functions are invoked, which schedules setImmediate/nextTick.
2. React batch processing -- this checks the message queue for new JS -> native calls and sends them to native(?)
3. --- next tick ---
4. The then/catch handlers of the promises run

I'm not sure what the right answer is though I have a couple of thoughts. Ideally this should not affect the program's correctness, since generally speaking it's not possible to know for sure when the promise will be fulfilled so better to program defensively. It may affect performance because of the extra tick and reduced batching -- wish that could be better. I do like the fact that global.Promise can be swapped out for a custom promise library like bluebird so features like long stack traces are available -- this is the main reason I would not be in favor of relying on a non-standard polyfill. Maybe the right tradeoff is to have a synchronous polyfill by default but still allow bluebird, etc. at the cost of the extra setImmediate tick.

[vjeux]

Your timeline is correct, and yes, it will not affect correctness but will affect performance and make debugging a tad harder as the queue won't be effectively executed in order.

The way batching works is that all the messages during a frame are sent via one call from obj-c to js. Before processing the first message, we start a flag saying that we are in batch mode, we process all the events and there's a lot of setState being fired, we call flush and React processes all the elements that have been dirtied.

I'm not suggesting to replace Promise globally, but just here returning a dumb object with a then and a catch method that executes synchronously.

[vjeux]

If you think this is not a good idea, then we can just use Promise

[ide]

I was thinking we would want the behavior and extra functions of global.Promise, like long stack traces and promise.done().

Setting out-of-order execution aside for a moment, it looks like the setImmediate handlers should be batched. That way we'll keep most of the performance though there will be two batches instead of one unless we refactor the code a bit.

Here's what I think we should do longer-term:

1. Call ReactUpdates.batchedUpdate when processing the setImmediate handlers since they do not appear batched right now
2. Rename setImmediate to process.nextTick. The current behavior of setImmediate is closer to process.nextTick than setImmediate since the handlers are synchronously invoked at the end of the JS event loop.
3. Most Promise libraries will automatically use nextTick if it is available. Nothing should break.
4. Implement setImmediate according to the spec or as much as is reasonable =)
5. Convert most bridge methods to promises. Now they are batched and in order again (if everyone is using Promises).

I'd like to start with the built-in Promise library if that's OK, and then can optimize later if it's causing problems. The really sensitive code like the UIManager should keep using callbacks for now -- but modules like AsyncStorage and the network library are fine if they aren't batched since they fire at unpredictable times.

[ide]

Here's a sample trace with bluebird's long stack traces enabled in JSC. Code looks like:

  componentDidMount() {
    new Promise((resolve) => resolve(1)).then(() => {
      require('NativeModules').Testing.testAsync().catch(
        (err) => console.log(err.stack)
      );
    });
  }

Error: Unknown error from a native module
    _createErrorFromErrorData@UIExplorer:7330:24
    _invokeCallback@UIExplorer:7659:15
    forEach@[native code]
    perform@UIExplorer:6188:24
    batchedUpdates@UIExplorer:18840:26
    batchedUpdates@UIExplorer:4720:34
    applyWithGuard@UIExplorer:882:25
    guardReturn@UIExplorer:7487:30
From previous event:
    callTimer@UIExplorer:7981:17
    callImmediates@UIExplorer:8030:34
    _flushedQueueUnguarded@UIExplorer:7855:37
    applyWithGuard@UIExplorer:882:25
    guardReturn@UIExplorer:7490:37
From previous event:
    UIExplorerApp_componentDidMount@UIExplorer:1157:12
    notifyAll@UIExplorer:4957:26
    close@UIExplorer:19254:35
    closeAll@UIExplorer:6261:29
    perform@UIExplorer:6202:24
    batchedMountComponentIntoNode@UIExplorer:20834:22
    batchedUpdates@UIExplorer:18838:15
    batchedUpdates@UIExplorer:4720:34
    renderComponent@UIExplorer:20903:32
    ReactMount__renderNewRootComponent@UIExplorer:5056:26
    render@UIExplorer:1350:39
    renderApplication@UIExplorer:40446:15
    run@UIExplorer:40387:34
    runApplication@UIExplorer:40409:26
    jsCall@UIExplorer:7438:34
    _callFunction@UIExplorer:7693:21
    forEach@[native code]
    perform@UIExplorer:6188:24
    batchedUpdates@UIExplorer:18840:26
    batchedUpdates@UIExplorer:4720:34
    applyWithGuard@UIExplorer:882:25
    guardReturn@UIExplorer:7487:30
    processBatch@UIExplorer:7712:23

It's not clean but it's really good that it shows the error originated from componentDidMount. Compare it to the error with the current Promise library and no long stack traces:

Error: Unknown error from a native module
    _createErrorFromErrorData@UIExplorer:7328:24
    UIExplorer:7283:54
    _invokeCallback@UIExplorer:7657:15
    UIExplorer:7720:39
    forEach@[native code]
    UIExplorer:7712:22
    perform@UIExplorer:6186:24
    batchedUpdates@UIExplorer:13850:26
    batchedUpdates@UIExplorer:4718:34
    UIExplorer:7711:34
    applyWithGuard@UIExplorer:882:25
    guardReturn@UIExplorer:7485:30
    processBatch@UIExplorer:7710:23
    [native code]
    global code

So I think this will help contribute to much better debugging.

[vjeux]

Sweet :)

[nicklockwood]

I'm cautiously in favor of this idea. Some thoughts on the implementation:

The RCTPromiseRejecter callback for handling errors is a good idea, but I've been wanting to introduce something like this as a general solution for error callbacks, replacing the existing RCTMakeError() mechanism, which is inadequate anyway as it doesn't enforce any kind of standard for domain/code info. So let's just add an RCTResponseErrorBlock (or whatever) to complement RCTResponseSenderBlock (which I now realise is a terrible name anyway) and allow it to be used in any callback.

The automatic resolve/reject declaration idea you had originally was nice, but if that's not workable, then there's not really any need to have a new macro for promises. Instead of declaring a method to be of type promise, and then validating that the arguments meet the criteria, just let any method with a success and error callback be used as a promise. It doesn't seem like the actual calling mechanism is any different on the native side.

On the JS side, when enumerating over the exported module methods we can identify methods that meet the criteria to be a promise, and either add a second function with promise semantics, e.g.

MyNativeModule.getValue() // the original exported function
MyNativeModule.getValuePromise() // promise-ized version added by BatchedBridgeFactory.js

Or, since functions are objects anyway, we could add a property, something like:

MyNativeModule.getValue() // the original exported function
MyNativeModule.getValue.promise() // promise-ized version added by BatchedBridgeFactory.js

[vjeux]

Question: do we get a good stack trace when creating the error here? Is there a better place to?

[ide]

The stack trace will have an extra frame for _createErrorFromErrorData but it's going to be full of internal methods with BatchedBridgeFactory and MessageQueue anyway. We could also use the Error.prepareStackTrace API (V8 only though) to modify the trace too.

The nice thing about this overall diff is that the Promise library can enable long stack traces. I have not tried it yet, but if a user wants to set global.Promise = bluebird and turn on debugging mode, I believe they will get long stack traces for free =)

[sahrens]

I think you can do something like

error.framesToPop = 1;

[ide]

Looks like a neat fb extension, will add this.

[vjeux]

Idea: one thing we can do is to turn everything with a callback (except for event listeners) into a promise by default since the two don't APIs don't clash:

AsyncStorage.getValue(someData, func, func) // old
AsyncStorage.getValue(someData).then(func).catch(func) // new
AsyncStorage.getValue(someData, func, func).then(func).catch(func) // mix of old and new

then, whenever we don't have any non-promise call sites anymore, we can just turn off the old way of doing things. To ease the transition we can throw a warning if you call it the old way.

The advantage of this is that we don't need to change the ios/android side of things. we just assume that 1 callback is only error and 2 callbacks is success then error (or the opposite i never remember).

If we are to change this as the bridge level then we also need to change android which you don't have access.

[nicklockwood]

@vjeux that sounds like a great idea. Although why ever deprecate calling the non-promise way? Having the ability to seamlessly switch between Promise and function-style calling conventions for the same API sounds pretty sweet to me.

[vjeux]

@nicklockwood if you're using callbacks, then we allocate a promise object that's never going to be used? Also, having two ways to do the same thing is often not a desirable property to have in a system.

[nicklockwood]

@vjeux can't it be allocated lazily?

I agree about it being bad to have two ways to do the exact same thing, but these aren't exactly the same. I see this more like how ES6 provides both regular functions and arrow functions. Arrows are better for many purposes, but it wouldn't be a good idea to deprecate function() syntax and mandate the use of arrows.

[ide]

@nicklockwood: if I understand the motivation behind RCTResponseErrorBlock it's to support NSError -> JS Error conversion -- do I have that right? Here's a proposal if so: the bridge could marshal NSErrors in general, so you'd even be able to write senderBlock(@[someObject, someNSError, someNSNumber]) so we wouldn't need RCTResponseErrorBlock. We'd still want RCTPromiseResolver and RCTPromiseRejecter because they enforce that the resolver should take only one argument and the rejecter should take an error.

@vjeux:

we just assume that 1 callback is only error and 2 callbacks is success then error (or the opposite i never remember).

Modules in the RN ecosystem are doing everything right now: only error callback, only success callback, (onSuccess, onError), (onError, onSuccess). So I think it might be a big bandaid to rip if we make assumptions about the callbacks.

For now I am in favor of having async functions (i.e. those that return promises) separate from normal functions. It's simpler and sidesteps all backwards compatibility issues. Node was trying to do something similar and backed off for a couple of reasons (latest proposal I heard a few months back was to make ES6 modules expose a Promise-only API if you import them and expose the legacy callback API if you require() them). We can still add a warning to normal functions if we want to deprecate them but I think it is too early to make the call that every bridged method must be async as in ES7 async.

[nicklockwood]

@ide, that makes sense to me. I like the idea of the bridge automatically resolving NSErrors into a standard format (in fact, that could apply to any type of object - maybe we need a whole set of reverse RCTConvert functions...)

And yes, it would also make sense to keep RCTPromiseResolver and RCTPromiseRejecter (maybe RCTPromiseResolveBlock and RejectBlock for consistency?), but we still wouldn't need the RCT_EXPORT_PROMISE() macro, right? We'd just check if an exported method had both of those in its argument list and treat it as a promise if so.

[ide]

we still wouldn't need the RCT_EXPORT_PROMISE() macro, right?

Yeah. It's a question of how explicit we want to be and if supporting different function kinds (official term from the es spec) is important, which is why I added the struct field. Other function kinds are generators and constructors.

[vjeux]

Happy for it to be explicit, this way we can do a per method migration. It's probably safer than a global migration :)

[ide]

A summary of where the diff is at:

Obj-C API

• RCT_EXPORT_PROMISE_EXPERIMENTAL requires explicit resolve & reject blocks. This makes handling methods with parameters and without parameters consistent. Can later add a macro that implicitly defines the resolve & reject parameters if desired.
• Block types are named RCTPromiseResolveBlock and RCTPromiseRejectBlock. ResolveBlock takes one object, RejectBlock takes one NSError.
• Both the resolve and reject blocks must be declared in the method signature. Can later make it so you only need to declare the resolve block (i.e. the method never fails) if it turns out that's convenient. Starting out simple for now; easier to add convenience later than to take it away.

Obj-C implementation

• Function kind (normal, async) is explicitly recorded. Pro: provides a pattern for adding new function kinds like generators. Con: static libs need to be rebuilt once.
• NSError -> JS Error conversion is handled only for RejectBlocks, not all ResponseSenders. When there is a generalizable Obj-C object -> JS object converter that can handle custom types, we can add support for NSError and treat promise rejection the same as any other native -> JS callback. Building the generic converter requires changing the JSON encoder so that it calls a method like -[id<JSONSerializable> toJSON] or +[RCTJSConvert Type:] on non-Foundation objects, so I want to defer that.

JS API

• The new functions return promises, which are part of ES6 and are compatible with ES7 async functions.
• When ES7 async/await is natively supported by JSC, we can just add the async keyword to the function created in BatchedBridgeFactory.js.
• Async functions are separate from normal functions for now. That is, functions that take callbacks are not automatically promisified nor the other way around. If we come up with a good heuristic for promisification it should be pretty straightforward to implement it in BatchedBridgeFactory.

JS implementation

• The global Promise implementation is used. Pro: can inject in custom Promise implementations that support long stack traces and other features. Cons: then() handlers run after plain callbacks and are not batched. If this causes problems we can swap out the Promise implementation.
• In a follow-up diff I will batch the setImmediate handlers. Better to commit this separately so it's easier to bisect to.

Unless I missed a blocker I think this is shippable as an experimental API and it'd be helpful if a couple of FB's modules were converted so we can learn what the issues are.

[sahrens]

I like the idea of reverse RCTConvert - then we could return proper Map, Error, and other objects.

[ide]

@nicklockwood params are now enforced in RCTBridge.m and the function kind is inferred from the parameters. added a test plan to the commit too.

[vjeux]

The JS part looks good to me, thanks!

[ide]

I just noticed this wasn't imported into Differential - can someone summon the bot?

[vjeux]

@facebook-github-bot import

[ide]

Thanks 🙌🏻

[facebook-github-bot]

Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/465355620295660/int_phab to review.

[ide]

@nicklockwood could you take a look at the Obj-C side when you have a chance?