-
Notifications
You must be signed in to change notification settings - Fork 25k
[Bridge] Add support for JS async functions to RCT_EXPORT_METHOD #1232
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,9 +19,17 @@ var slice = Array.prototype.slice; | |
|
|
||
| var MethodTypes = keyMirror({ | ||
| remote: null, | ||
| remoteAsync: null, | ||
| local: null, | ||
| }); | ||
|
|
||
| type ErrorData = { | ||
| message: string; | ||
| domain: string; | ||
| code: number; | ||
| nativeStackIOS?: string; | ||
| }; | ||
|
|
||
| /** | ||
| * Creates remotely invokable modules. | ||
| */ | ||
|
|
@@ -36,21 +44,40 @@ var BatchedBridgeFactory = { | |
| */ | ||
| _createBridgedModule: function(messageQueue, moduleConfig, moduleName) { | ||
| var remoteModule = mapObject(moduleConfig.methods, function(methodConfig, memberName) { | ||
| return methodConfig.type === MethodTypes.local ? null : function() { | ||
| var lastArg = arguments.length > 0 ? arguments[arguments.length - 1] : null; | ||
| var secondLastArg = arguments.length > 1 ? arguments[arguments.length - 2] : null; | ||
| var hasSuccCB = typeof lastArg === 'function'; | ||
| var hasErrorCB = typeof secondLastArg === 'function'; | ||
| hasErrorCB && invariant( | ||
| hasSuccCB, | ||
| 'Cannot have a non-function arg after a function arg.' | ||
| ); | ||
| var numCBs = (hasSuccCB ? 1 : 0) + (hasErrorCB ? 1 : 0); | ||
| var args = slice.call(arguments, 0, arguments.length - numCBs); | ||
| var onSucc = hasSuccCB ? lastArg : null; | ||
| var onFail = hasErrorCB ? secondLastArg : null; | ||
| return messageQueue.call(moduleName, memberName, args, onFail, onSucc); | ||
| }; | ||
| switch (methodConfig.type) { | ||
| case MethodTypes.remote: | ||
| return function() { | ||
| var lastArg = arguments.length > 0 ? arguments[arguments.length - 1] : null; | ||
| var secondLastArg = arguments.length > 1 ? arguments[arguments.length - 2] : null; | ||
| var hasErrorCB = typeof lastArg === 'function'; | ||
| var hasSuccCB = typeof secondLastArg === 'function'; | ||
| hasSuccCB && invariant( | ||
| hasErrorCB, | ||
| 'Cannot have a non-function arg after a function arg.' | ||
| ); | ||
| var numCBs = (hasSuccCB ? 1 : 0) + (hasErrorCB ? 1 : 0); | ||
| var args = slice.call(arguments, 0, arguments.length - numCBs); | ||
| var onSucc = hasSuccCB ? secondLastArg : null; | ||
| var onFail = hasErrorCB ? lastArg : null; | ||
| messageQueue.call(moduleName, memberName, args, onSucc, onFail); | ||
| }; | ||
|
|
||
| case MethodTypes.remoteAsync: | ||
| return function(...args) { | ||
| return new Promise((resolve, reject) => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just to make sure I understand what you're saying:
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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you think this is not a good idea, then we can just use Promise
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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)
);
});
}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: So I think this will help contribute to much better debugging.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sweet :) |
||
| messageQueue.call(moduleName, memberName, args, resolve, (errorData) => { | ||
| var error = _createErrorFromErrorData(errorData); | ||
| reject(error); | ||
| }); | ||
| }); | ||
| }; | ||
|
|
||
| case MethodTypes.local: | ||
| return null; | ||
|
|
||
| default: | ||
| throw new Error('Unknown bridge method type: ' + methodConfig.type); | ||
| } | ||
| }); | ||
| for (var constName in moduleConfig.constants) { | ||
| warning(!remoteModule[constName], 'saw constant and method named %s', constName); | ||
|
|
@@ -59,7 +86,6 @@ var BatchedBridgeFactory = { | |
| return remoteModule; | ||
| }, | ||
|
|
||
|
|
||
| create: function(MessageQueue, modulesConfig, localModulesConfig) { | ||
| var messageQueue = new MessageQueue(modulesConfig, localModulesConfig); | ||
| return { | ||
|
|
@@ -80,4 +106,14 @@ var BatchedBridgeFactory = { | |
| } | ||
| }; | ||
|
|
||
| function _createErrorFromErrorData(errorData: ErrorData): Error { | ||
| var { | ||
| message, | ||
| ...extraErrorInfo, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. destructuring like this is so awesome, I love it <3 |
||
| } = errorData; | ||
| var error = new Error(message); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Question: do we get a good stack trace when creating the error here? Is there a better place to?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 =)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you can do something like error.framesToPop = 1;
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like a neat fb extension, will add this. |
||
| error.framesToPop = 1; | ||
| return Object.assign(error, extraErrorInfo); | ||
| } | ||
|
|
||
| module.exports = BatchedBridgeFactory; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -47,6 +47,11 @@ typedef NS_ENUM(NSUInteger, RCTBridgeFields) { | |
| RCTBridgeFieldFlushDateMillis | ||
| }; | ||
|
|
||
| typedef NS_ENUM(NSUInteger, RCTJavaScriptFunctionKind) { | ||
| RCTJavaScriptFunctionKindNormal, | ||
| RCTJavaScriptFunctionKindAsync, | ||
| }; | ||
|
|
||
| #ifdef __LP64__ | ||
| typedef uint64_t RCTHeaderValue; | ||
| typedef struct section_64 RCTHeaderSection; | ||
|
|
@@ -239,6 +244,7 @@ @interface RCTModuleMethod : NSObject | |
| @property (nonatomic, copy, readonly) NSString *moduleClassName; | ||
| @property (nonatomic, copy, readonly) NSString *JSMethodName; | ||
| @property (nonatomic, assign, readonly) SEL selector; | ||
| @property (nonatomic, assign, readonly) RCTJavaScriptFunctionKind functionKind; | ||
|
|
||
| @end | ||
|
|
||
|
|
@@ -420,6 +426,51 @@ - (instancetype)initWithReactMethodName:(NSString *)reactMethodName | |
| } else if ([argumentName isEqualToString:@"RCTResponseSenderBlock"]) { | ||
| addBlockArgument(); | ||
| useFallback = NO; | ||
| } else if ([argumentName isEqualToString:@"RCTPromiseResolveBlock"]) { | ||
| RCTAssert(i == numberOfArguments - 2, | ||
| @"The RCTPromiseResolveBlock must be the second to last parameter in -[%@ %@]", | ||
| _moduleClassName, objCMethodName); | ||
| RCT_ARG_BLOCK( | ||
| if (RCT_DEBUG && ![json isKindOfClass:[NSNumber class]]) { | ||
| RCTLogError(@"Argument %tu (%@) of %@.%@ must be a promise resolver ID", index, | ||
| json, RCTBridgeModuleNameForClass(_moduleClass), _JSMethodName); | ||
| return; | ||
| } | ||
|
|
||
| // Marked as autoreleasing, because NSInvocation doesn't retain arguments | ||
| __autoreleasing RCTPromiseResolveBlock value = (^(id result) { | ||
| NSArray *arguments = result ? @[result] : @[]; | ||
| [bridge _invokeAndProcessModule:@"BatchedBridge" | ||
| method:@"invokeCallbackAndReturnFlushedQueue" | ||
| arguments:@[json, arguments] | ||
| context:context]; | ||
| }); | ||
| ) | ||
| useFallback = NO; | ||
| _functionKind = RCTJavaScriptFunctionKindAsync; | ||
| } else if ([argumentName isEqualToString:@"RCTPromiseRejectBlock"]) { | ||
| RCTAssert(i == numberOfArguments - 1, | ||
| @"The RCTPromiseRejectBlock must be the last parameter in -[%@ %@]", | ||
| _moduleClassName, objCMethodName); | ||
| RCT_ARG_BLOCK( | ||
| if (RCT_DEBUG && ![json isKindOfClass:[NSNumber class]]) { | ||
| RCTLogError(@"Argument %tu (%@) of %@.%@ must be a promise rejecter ID", index, | ||
| json, RCTBridgeModuleNameForClass(_moduleClass), _JSMethodName); | ||
| return; | ||
| } | ||
|
|
||
| // Marked as autoreleasing, because NSInvocation doesn't retain arguments | ||
| __autoreleasing RCTPromiseRejectBlock value = (^(NSError *error) { | ||
| NSDictionary *errorData = [RCTModuleMethod dictionaryFromError:error | ||
| stackTrace:[NSThread callStackSymbols]]; | ||
| [bridge _invokeAndProcessModule:@"BatchedBridge" | ||
| method:@"invokeCallbackAndReturnFlushedQueue" | ||
| arguments:@[json, @[errorData]] | ||
| context:context]; | ||
| }); | ||
| ) | ||
| useFallback = NO; | ||
| _functionKind = RCTJavaScriptFunctionKindAsync; | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -498,9 +549,18 @@ - (void)invokeWithBridge:(RCTBridge *)bridge | |
|
|
||
| // Safety check | ||
| if (arguments.count != _argumentBlocks.count) { | ||
| NSInteger actualCount = arguments.count; | ||
| NSInteger expectedCount = _argumentBlocks.count; | ||
|
|
||
| // Subtract the implicit Promise resolver and rejecter functions for implementations of async functions | ||
| if (_functionKind == RCTJavaScriptFunctionKindAsync) { | ||
| actualCount -= 2; | ||
| expectedCount -= 2; | ||
| } | ||
|
|
||
| RCTLogError(@"%@.%@ was called with %zd arguments, but expects %zd", | ||
| RCTBridgeModuleNameForClass(_moduleClass), _JSMethodName, | ||
| arguments.count, _argumentBlocks.count); | ||
| actualCount, expectedCount); | ||
| return; | ||
| } | ||
| } | ||
|
|
@@ -528,6 +588,26 @@ - (NSString *)description | |
| return [NSString stringWithFormat:@"<%@: %p; exports %@ as %@;>", NSStringFromClass(self.class), self, _methodName, _JSMethodName]; | ||
| } | ||
|
|
||
| + (NSDictionary *)dictionaryFromError:(NSError *)error stackTrace:(NSArray *)stackTrace | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sweet |
||
| { | ||
| NSString *errorMessage; | ||
| NSMutableDictionary *errorInfo = [NSMutableDictionary dictionaryWithDictionary:@{ | ||
| @"nativeStackIOS": stackTrace, | ||
| }]; | ||
|
|
||
| if (error) { | ||
| errorMessage = error.localizedDescription ?: @"Unknown error from a native module"; | ||
| errorInfo[@"domain"] = error.domain ?: RCTErrorDomain; | ||
| errorInfo[@"code"] = @(error.code); | ||
| } else { | ||
| errorMessage = @"Unknown error from a native module"; | ||
| errorInfo[@"domain"] = RCTErrorDomain; | ||
| errorInfo[@"code"] = @(-1); | ||
| } | ||
|
|
||
| return RCTMakeError(errorMessage, nil, errorInfo); | ||
| } | ||
|
|
||
| @end | ||
|
|
||
| /** | ||
|
|
@@ -606,7 +686,7 @@ - (NSString *)description | |
| * }, | ||
| * "methodName2": { | ||
| * "methodID": 1, | ||
| * "type": "remote" | ||
| * "type": "remoteAsync" | ||
| * }, | ||
| * etc... | ||
| * }, | ||
|
|
@@ -630,7 +710,7 @@ - (NSString *)description | |
| [methods enumerateObjectsUsingBlock:^(RCTModuleMethod *method, NSUInteger methodID, BOOL *_stop) { | ||
| methodsByName[method.JSMethodName] = @{ | ||
| @"methodID": @(methodID), | ||
| @"type": @"remote", | ||
| @"type": method.functionKind == RCTJavaScriptFunctionKindAsync ? @"remoteAsync" : @"remote", | ||
| }; | ||
| }]; | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll be so happy when we get rid of this crazy block of code