PromiseExtensions.Reject and InnerExceptions

[btpnlsl]

I am looking into an error where LauncherModule.openURL is failing to open a URI because a native call to Launcher.LaunchUriAsync is throwing an exception for an unknown reason. Within openURL the exception is caught by a catch-block and the code is repackaging the exception within an InvalidOperationException before calling Promise.Reject.

In Promise.Reject (in PromiseExtensions.cs) the code is not pulling any info about the exceptions HResult or the InnerException, so all the information about why the native API failed is lost. I notice that this pattern of repackaging exceptions is used in a number of places.

At minimum RNW should not repackage exceptions if it is not providing the original exception information to Promise.Reject. Ideally RNW should also include HResult and InnerException in the Promise.Reject.

[rozele]

@btpnlsl - there's an interesting trade-off here. Everything we include in the promise will be serialized to JavaScript, and we want to avoid serializing too much. At the same time, we want to be able to allow people to get as much detail out of the native exception as possible. Ideally, we'd have something like an Error module that allowed you to ask for additional details on an error from a rejected promise (i.e., we would serialize just top level information when the promise is initially rejected, and "pay for play" to serialize more details from the exception).

This would be tricky to implement, though, because there is no concept of a finalizer in JavaScript. We could handle this natively in Chakra via
https://github.com/Microsoft/ChakraCore/wiki/JsObjectBeforeCollectCallback, but for consistency with iOS and Android, we'd need to implement a similar feature for JSC.

[rozele]

There is actually currently a similar issue w.r.t. the BlobModule in react-native, and how blobs are not being cleaned up after fetch requests. See facebook/react-native#19333

[rozele]

RN Android is equally lossy - https://github.com/facebook/react-native/blob/master/ReactAndroid/src/main/java/com/facebook/react/bridge/PromiseImpl.java#L47 :-(

[btpnlsl]

@rozele We're already copying the exception's Data into userInfo. Why not just grab the InnerException as well? https://github.com/Microsoft/react-native-windows/blob/0a2dff3c939c14a253b23cb3d1194a0dbe7f4225/ReactWindows/ReactNative.Shared/Bridge/PromiseExtensions.cs#L76-L82

[rozele]

We need to be careful about how much information we serialize across the bridge, it really should be a "pull" feature to get detailed information about the exception, rather than a push feature.

[ghost]

We are not investing in new features or lower priority bug fixes on vCurrent. The bulk of investment is now in vNext as we prepare to make that the default choice. If this issue is still relevant on the vNext implementation please open a vNext issue. If this issue is of significant severity for a vCurrent app and vNext is not an option, re-open with justification.