From 8420e5d92058a25ea1b35a901abd113c3084def9 Mon Sep 17 00:00:00 2001 From: Salakar Date: Mon, 10 Dec 2018 21:13:34 -0800 Subject: [PATCH] Allow 'userInfo' for native promise.reject + add native error stack support (#20940) Summary: As mentioned [here](https://github.com/react-native-community/react-native-releases/issues/34#issuecomment-417718601), Android is missing native Promise reject with a `userInfo` `WritableMap` support and also `nativeStack` support (which addresses `TODO(8850038)`). This PR adds Android support for both of these. React Native on iOS ([here](https://github.com/facebook/react-native/blob/master/React/Base/RCTUtils.m#L433)) and Windows ([here](https://github.com/Microsoft/react-native-windows/pull/732)) already support this so this is a relatively minor addition to bring Android in line with the other platforms. (JS support is also [here](https://github.com/facebook/react-native/blob/master/Libraries/BatchedBridge/NativeModules.js#L145-L148)) Existing methods remain unchanged other than general cleanup of variable names (`e -> throwable`) and adding code comments/docs. Additionally, the `ShareTestModule` implementation of Promise (SimplePromise) was updated to reflect these changes - other line changes in this file are from formatting in Android Studio - if this is an issue let me know. - Currently inconsistent with other platforms. - Blocking a couple of issues over at [invertase/react-native-firebase](https://github.com/invertase/react-native-firebase) - save for re-writing everything to Promise resolve only - which is no small change and isn't a great solution either. Pull Request resolved: https://github.com/facebook/react-native/pull/20940 Differential Revision: D13412527 Pulled By: cpojer fbshipit-source-id: 2ca6c5f3db9ff2c2986b02edda80bc73432f66d3 --- .../main/java/com/facebook/react/bridge/BUCK | 1 + .../com/facebook/react/bridge/Promise.java | 101 ++++++-- .../facebook/react/bridge/PromiseImpl.java | 223 ++++++++++++++++-- .../react/modules/netinfo/NetInfoModule.java | 4 +- .../react/modules/share/ShareModuleTest.java | 131 +++++++--- 5 files changed, 382 insertions(+), 78 deletions(-) diff --git a/ReactAndroid/src/main/java/com/facebook/react/bridge/BUCK b/ReactAndroid/src/main/java/com/facebook/react/bridge/BUCK index f76357ec486ca7..af47db17a7227c 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/bridge/BUCK +++ b/ReactAndroid/src/main/java/com/facebook/react/bridge/BUCK @@ -22,6 +22,7 @@ rn_android_library( proguard_config = "reactnative.pro", provided_deps = [ react_native_dep("third-party/android/support/v4:lib-support-v4"), + react_native_dep("third-party/android/support-annotations:android-support-annotations"), ], required_for_source_only_abi = True, visibility = [ diff --git a/ReactAndroid/src/main/java/com/facebook/react/bridge/Promise.java b/ReactAndroid/src/main/java/com/facebook/react/bridge/Promise.java index 2ee1626f02396e..27d4d697cf86af 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/bridge/Promise.java +++ b/ReactAndroid/src/main/java/com/facebook/react/bridge/Promise.java @@ -7,48 +7,119 @@ package com.facebook.react.bridge; +import javax.annotation.Nonnull; import javax.annotation.Nullable; -/** +/* * Interface that represents a JavaScript Promise which can be passed to the native module as a * method parameter. * - * Methods annotated with {@link ReactMethod} that use {@link Promise} as type of the last parameter + * Methods annotated with {@link ReactMethod} that use a {@link Promise} as the last parameter * will be marked as "promise" and will return a promise when invoked from JavaScript. */ public interface Promise { /** - * Successfully resolve the Promise. + * Successfully resolve the Promise with an optional value. + * + * @param value Object */ void resolve(@Nullable Object value); /** - * Report an error which wasn't caused by an exception. + * Report an error without an exception using a custom code and error message. + * + * @param code String + * @param message String */ void reject(String code, String message); /** - * Report an exception. + * Report an exception with a custom code. + * + * @param code String + * @param throwable Throwable */ - void reject(String code, Throwable e); + void reject(String code, Throwable throwable); /** - * Report an exception with a custom error message. + * Report an exception with a custom code and error message. + * + * @param code String + * @param message String + * @param throwable Throwable */ - void reject(String code, String message, Throwable e); + void reject(String code, String message, Throwable throwable); + /** - * Report an error which wasn't caused by an exception. - * @deprecated Prefer passing a module-specific error code to JS. - * Using this method will pass the error code "EUNSPECIFIED". + * Report an exception, with default error code. + * Useful in catch-all scenarios where it's unclear why the error occurred. + * + * @param throwable Throwable */ - @Deprecated - void reject(String message); + void reject(Throwable throwable); + + /* --------------------------- + * With userInfo WritableMap + * --------------------------- */ /** - * Report an exception, with default error code. + * Report an exception, with default error code, with userInfo. * Useful in catch-all scenarios where it's unclear why the error occurred. + * + * @param throwable Throwable + * @param userInfo WritableMap + */ + void reject(Throwable throwable, WritableMap userInfo); + + /** + * Reject with a code and userInfo WritableMap. + * + * @param code String + * @param userInfo WritableMap */ - void reject(Throwable reason); + void reject(String code, @Nonnull WritableMap userInfo); + + /** + * Report an exception with a custom code and userInfo. + * + * @param code String + * @param throwable Throwable + * @param userInfo WritableMap + */ + void reject(String code, Throwable throwable, WritableMap userInfo); + + /** + * Report an error with a custom code, error message and userInfo, + * an error not caused by an exception. + * + * @param code String + * @param message String + * @param userInfo WritableMap + */ + void reject(String code, String message, @Nonnull WritableMap userInfo); + + /** + * Report an exception with a custom code, error message and userInfo. + * + * @param code String + * @param message String + * @param throwable Throwable + * @param userInfo WritableMap + */ + void reject(String code, String message, Throwable throwable, WritableMap userInfo); + + /* ------------ + * Deprecated + * ------------ */ + + /** + * Report an error which wasn't caused by an exception. + * + * @deprecated Prefer passing a module-specific error code to JS. + * Using this method will pass the error code "EUNSPECIFIED". + */ + @Deprecated + void reject(String message); } diff --git a/ReactAndroid/src/main/java/com/facebook/react/bridge/PromiseImpl.java b/ReactAndroid/src/main/java/com/facebook/react/bridge/PromiseImpl.java index 98a46f9af90cd8..4c67957eda7e33 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/bridge/PromiseImpl.java +++ b/ReactAndroid/src/main/java/com/facebook/react/bridge/PromiseImpl.java @@ -5,25 +5,52 @@ * LICENSE file in the root directory of this source tree. */ -/** - * Implementation of two javascript functions that can be used to resolve or reject a js promise. - */ package com.facebook.react.bridge; +import javax.annotation.Nonnull; import javax.annotation.Nullable; +/* + * Implementation of {@link Promise} that represents a JavaScript Promise which can be passed to the + * native module as a method parameter. + * + * Methods annotated with {@link ReactMethod} that use a {@link Promise} as the last parameter + * will be marked as "promise" and will return a promise when invoked from JavaScript. + */ public class PromiseImpl implements Promise { + // Number of stack frames to parse and return to mReject.invoke + // for ERROR_MAP_KEY_NATIVE_STACK + private static final int ERROR_STACK_FRAME_LIMIT = 10; + + private static final String ERROR_DEFAULT_CODE = "EUNSPECIFIED"; + private static final String ERROR_DEFAULT_MESSAGE = "Error not specified."; - private static final String DEFAULT_ERROR = "EUNSPECIFIED"; + // Keys for mReject's WritableMap + private static final String ERROR_MAP_KEY_CODE = "code"; + private static final String ERROR_MAP_KEY_MESSAGE = "message"; + private static final String ERROR_MAP_KEY_USER_INFO = "userInfo"; + private static final String ERROR_MAP_KEY_NATIVE_STACK = "nativeStackAndroid"; - private @Nullable Callback mResolve; - private @Nullable Callback mReject; + // Keys for ERROR_MAP_KEY_NATIVE_STACK's StackFrame maps + private static final String STACK_FRAME_KEY_FILE = "file"; + private static final String STACK_FRAME_KEY_LINE_NUMBER = "lineNumber"; + private static final String STACK_FRAME_KEY_METHOD_NAME = "methodName"; + + private @Nullable + Callback mResolve; + private @Nullable + Callback mReject; public PromiseImpl(@Nullable Callback resolve, @Nullable Callback reject) { mResolve = resolve; mReject = reject; } + /** + * Successfully resolve the Promise with an optional value. + * + * @param value Object + */ @Override public void resolve(Object value) { if (mResolve != null) { @@ -33,43 +60,187 @@ public void resolve(Object value) { } } + /** + * Report an error without an exception using a custom code and error message. + * + * @param code String + * @param message String + */ @Override public void reject(String code, String message) { - reject(code, message, /*Throwable*/null); + reject(code, message, /*Throwable*/null, /*WritableMap*/null); } + /** + * Report an exception with a custom code. + * + * @param code String + * @param throwable Throwable + */ @Override - @Deprecated - public void reject(String message) { - reject(DEFAULT_ERROR, message, /*Throwable*/null); + public void reject(String code, Throwable throwable) { + reject(code, /*Message*/null, throwable, /*WritableMap*/null); + } + + /** + * Report an exception with a custom code and error message. + * + * @param code String + * @param message String + * @param throwable Throwable + */ + @Override + public void reject(String code, String message, Throwable throwable) { + reject(code, message, throwable, /*WritableMap*/null); + } + + /** + * Report an exception, with default error code. + * Useful in catch-all scenarios where it's unclear why the error occurred. + * + * @param throwable Throwable + */ + @Override + public void reject(Throwable throwable) { + reject(/*Code*/null, /*Message*/null, throwable, /*WritableMap*/null); + } + + /* --------------------------- + * With userInfo WritableMap + * --------------------------- */ + + /** + * Report an exception, with default error code, with userInfo. + * Useful in catch-all scenarios where it's unclear why the error occurred. + * + * @param throwable Throwable + * @param userInfo WritableMap + */ + @Override + public void reject(Throwable throwable, WritableMap userInfo) { + reject(/*Code*/null, /*Message*/null, throwable, userInfo); + } + + /** + * Reject with a code and userInfo WritableMap. + * + * @param code String + * @param userInfo WritableMap + */ + @Override + public void reject(String code, @Nonnull WritableMap userInfo) { + reject(code, /*Message*/null, /*Throwable*/null, userInfo); } + /** + * Report an exception with a custom code and userInfo. + * + * @param code String + * @param throwable Throwable + * @param userInfo WritableMap + */ @Override - public void reject(String code, Throwable e) { - reject(code, e.getMessage(), e); + public void reject(String code, Throwable throwable, WritableMap userInfo) { + reject(code, /*Message*/null, throwable, userInfo); } + /** + * Report an error with a custom code, error message and userInfo, + * an error not caused by an exception. + * + * @param code String + * @param message String + * @param userInfo WritableMap + */ @Override - public void reject(Throwable e) { - reject(DEFAULT_ERROR, e.getMessage(), e); + public void reject(String code, String message, @Nonnull WritableMap userInfo) { + reject(code, message, /*Throwable*/null, userInfo); } + /** + * Report an exception with a custom code, error message and userInfo. + * + * @param code String + * @param message String + * @param throwable Throwable + * @param userInfo WritableMap + */ @Override - public void reject(String code, String message, @Nullable Throwable e) { - if (mReject != null) { - if (code == null) { - code = DEFAULT_ERROR; + public void reject( + @Nullable String code, + @Nullable String message, + @Nullable Throwable throwable, + @Nullable WritableMap userInfo + ) { + if (mReject == null) { + mResolve = null; + return; + } + + WritableNativeMap errorInfo = new WritableNativeMap(); + + if (code == null) { + errorInfo.putString(ERROR_MAP_KEY_CODE, ERROR_DEFAULT_CODE); + } else { + errorInfo.putString(ERROR_MAP_KEY_CODE, code); + } + + // Use the custom message if provided otherwise use the throwable message. + if (message != null) { + errorInfo.putString(ERROR_MAP_KEY_MESSAGE, message); + } else if (throwable != null) { + errorInfo.putString(ERROR_MAP_KEY_MESSAGE, throwable.getMessage()); + } else { + // The JavaScript side expects a map with at least an error message. + // /Libraries/BatchedBridge/NativeModules.js -> createErrorFromErrorData + // TYPE: (errorData: { message: string }) + errorInfo.putString(ERROR_MAP_KEY_MESSAGE, ERROR_DEFAULT_MESSAGE); + } + + // For consistency with iOS ensure userInfo key exists, even if we null it. + // iOS: /React/Base/RCTUtils.m -> RCTJSErrorFromCodeMessageAndNSError + if (userInfo != null) { + errorInfo.putMap(ERROR_MAP_KEY_USER_INFO, userInfo); + } else { + errorInfo.putNull(ERROR_MAP_KEY_USER_INFO); + } + + // Attach a nativeStackAndroid array if a throwable was passed + // this matches iOS behavior - iOS adds a `nativeStackIOS` property + // iOS: /React/Base/RCTUtils.m -> RCTJSErrorFromCodeMessageAndNSError + if (throwable != null) { + StackTraceElement[] stackTrace = throwable.getStackTrace(); + WritableNativeArray nativeStackAndroid = new WritableNativeArray(); + + // Build an an Array of StackFrames to match JavaScript: + // iOS: /Libraries/Core/Devtools/parseErrorStack.js -> StackFrame + for (int i = 0; i < stackTrace.length && i < ERROR_STACK_FRAME_LIMIT; i++) { + StackTraceElement frame = stackTrace[i]; + WritableMap frameMap = new WritableNativeMap(); + // NOTE: no column number exists StackTraceElement + frameMap.putString(STACK_FRAME_KEY_FILE, frame.getFileName()); + frameMap.putInt(STACK_FRAME_KEY_LINE_NUMBER, frame.getLineNumber()); + frameMap.putString(STACK_FRAME_KEY_METHOD_NAME, frame.getMethodName()); + nativeStackAndroid.pushMap(frameMap); } - // The JavaScript side expects a map with at least the error message. - // It is possible to expose all kind of information. It will be available on the JS - // error instance. - WritableNativeMap errorInfo = new WritableNativeMap(); - errorInfo.putString("code", code); - errorInfo.putString("message", message); - // TODO(8850038): add the stack trace info in, need to figure out way to serialize that - mReject.invoke(errorInfo); + + errorInfo.putArray(ERROR_MAP_KEY_NATIVE_STACK, nativeStackAndroid); + } else { + errorInfo.putArray(ERROR_MAP_KEY_NATIVE_STACK, new WritableNativeArray()); } + + mReject.invoke(errorInfo); mResolve = null; mReject = null; } + + /* ------------ + * Deprecated + * ------------ */ + + @Override + @Deprecated + public void reject(String message) { + reject(/*Code*/null, message, /*Throwable*/null, /*WritableMap*/null); + } } diff --git a/ReactAndroid/src/main/java/com/facebook/react/modules/netinfo/NetInfoModule.java b/ReactAndroid/src/main/java/com/facebook/react/modules/netinfo/NetInfoModule.java index 12b26f0051d81f..156a3ca026363e 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/modules/netinfo/NetInfoModule.java +++ b/ReactAndroid/src/main/java/com/facebook/react/modules/netinfo/NetInfoModule.java @@ -103,7 +103,7 @@ public String getName() { @ReactMethod public void getCurrentConnectivity(Promise promise) { if (mNoNetworkPermission) { - promise.reject(ERROR_MISSING_PERMISSION, MISSING_PERMISSION_MESSAGE, null); + promise.reject(ERROR_MISSING_PERMISSION, MISSING_PERMISSION_MESSAGE); return; } promise.resolve(createConnectivityEventMap()); @@ -112,7 +112,7 @@ public void getCurrentConnectivity(Promise promise) { @ReactMethod public void isConnectionMetered(Promise promise) { if (mNoNetworkPermission) { - promise.reject(ERROR_MISSING_PERMISSION, MISSING_PERMISSION_MESSAGE, null); + promise.reject(ERROR_MISSING_PERMISSION, MISSING_PERMISSION_MESSAGE); return; } promise.resolve(ConnectivityManagerCompat.isActiveNetworkMetered(mConnectivityManager)); diff --git a/ReactAndroid/src/test/java/com/facebook/react/modules/share/ShareModuleTest.java b/ReactAndroid/src/test/java/com/facebook/react/modules/share/ShareModuleTest.java index cd505243f0a5b4..90484e1591fadd 100644 --- a/ReactAndroid/src/test/java/com/facebook/react/modules/share/ShareModuleTest.java +++ b/ReactAndroid/src/test/java/com/facebook/react/modules/share/ShareModuleTest.java @@ -11,31 +11,31 @@ import android.content.Intent; import com.facebook.react.bridge.Arguments; +import com.facebook.react.bridge.JavaOnlyMap; import com.facebook.react.bridge.Promise; -import com.facebook.react.bridge.ReactApplicationContext; import com.facebook.react.bridge.ReactTestHelper; -import com.facebook.react.bridge.JavaOnlyMap; - -import javax.annotation.Nullable; +import com.facebook.react.bridge.WritableMap; import org.junit.After; import org.junit.Before; -import org.junit.Test; import org.junit.Rule; +import org.junit.Test; import org.junit.runner.RunWith; import org.mockito.Mockito; import org.mockito.invocation.InvocationOnMock; import org.mockito.stubbing.Answer; import org.powermock.api.mockito.PowerMockito; -import org.powermock.core.classloader.annotations.PrepareForTest; import org.powermock.core.classloader.annotations.PowerMockIgnore; +import org.powermock.core.classloader.annotations.PrepareForTest; import org.powermock.modules.junit4.rule.PowerMockRule; -import org.robolectric.internal.ShadowExtractor; -import org.robolectric.Robolectric; import org.robolectric.RobolectricTestRunner; import org.robolectric.RuntimeEnvironment; +import org.robolectric.internal.ShadowExtractor; import org.robolectric.shadows.ShadowApplication; +import javax.annotation.Nonnull; +import javax.annotation.Nullable; + import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; @@ -53,12 +53,14 @@ public class ShareModuleTest { @Before public void prepareModules() throws Exception { PowerMockito.mockStatic(Arguments.class); - Mockito.when(Arguments.createMap()).thenAnswer(new Answer() { - @Override - public Object answer(InvocationOnMock invocation) throws Throwable { - return new JavaOnlyMap(); - } - }); + Mockito + .when(Arguments.createMap()) + .thenAnswer(new Answer() { + @Override + public Object answer(InvocationOnMock invocation) throws Throwable { + return new JavaOnlyMap(); + } + }); mShareModule = new ShareModule(ReactTestHelper.createCatalystContextForTest()); } @@ -83,17 +85,34 @@ public void testShareDialog() { mShareModule.share(content, dialogTitle, promise); - final Intent chooserIntent = - ((ShadowApplication)ShadowExtractor.extract(RuntimeEnvironment.application)).getNextStartedActivity(); + final Intent chooserIntent = + ((ShadowApplication) ShadowExtractor.extract(RuntimeEnvironment.application)).getNextStartedActivity(); assertNotNull("Dialog was not displayed", chooserIntent); assertEquals(Intent.ACTION_CHOOSER, chooserIntent.getAction()); - assertEquals(dialogTitle, chooserIntent.getExtras().get(Intent.EXTRA_TITLE)); - - final Intent contentIntent = (Intent)chooserIntent.getExtras().get(Intent.EXTRA_INTENT); + assertEquals( + dialogTitle, + chooserIntent + .getExtras() + .get(Intent.EXTRA_TITLE) + ); + + final Intent contentIntent = (Intent) chooserIntent + .getExtras() + .get(Intent.EXTRA_INTENT); assertNotNull("Intent was not built correctly", contentIntent); assertEquals(Intent.ACTION_SEND, contentIntent.getAction()); - assertEquals(title, contentIntent.getExtras().get(Intent.EXTRA_SUBJECT)); - assertEquals(message, contentIntent.getExtras().get(Intent.EXTRA_TEXT)); + assertEquals( + title, + contentIntent + .getExtras() + .get(Intent.EXTRA_SUBJECT) + ); + assertEquals( + message, + contentIntent + .getExtras() + .get(Intent.EXTRA_TEXT) + ); assertEquals(1, promise.getResolved()); } @@ -111,7 +130,8 @@ public void testInvalidContent() { } final static class SimplePromise implements Promise { - private static final String DEFAULT_ERROR = "EUNSPECIFIED"; + private static final String ERROR_DEFAULT_CODE = "EUNSPECIFIED"; + private static final String ERROR_DEFAULT_MESSAGE = "Error not specified."; private int mResolved; private int mRejected; @@ -126,7 +146,7 @@ public int getResolved() { public int getRejected() { return mRejected; } - + public Object getValue() { return mValue; } @@ -147,31 +167,72 @@ public void resolve(Object value) { @Override public void reject(String code, String message) { - reject(code, message, /*Throwable*/null); + reject(code, message, /*Throwable*/null, /*WritableMap*/null); } @Override - @Deprecated - public void reject(String message) { - reject(DEFAULT_ERROR, message, /*Throwable*/null); + public void reject(String code, Throwable throwable) { + reject(code, /*Message*/null, throwable, /*WritableMap*/null); + } + + @Override + public void reject(String code, String message, Throwable throwable) { + reject(code, message, throwable, /*WritableMap*/null); } @Override - public void reject(String code, Throwable e) { - reject(code, e.getMessage(), e); + public void reject(Throwable throwable) { + reject(/*Code*/null, /*Message*/null, throwable, /*WritableMap*/null); } @Override - public void reject(Throwable e) { - reject(DEFAULT_ERROR, e.getMessage(), e); + public void reject(Throwable throwable, WritableMap userInfo) { + reject(/*Code*/null, /*Message*/null, throwable, userInfo); } @Override - public void reject(String code, String message, @Nullable Throwable e) { + public void reject(String code, @Nonnull WritableMap userInfo) { + reject(code, /*Message*/null, /*Throwable*/null, userInfo); + } + + @Override + public void reject(String code, Throwable throwable, WritableMap userInfo) { + reject(code, /*Message*/null, throwable, userInfo); + } + + @Override + public void reject(String code, String message, @Nonnull WritableMap userInfo) { + reject(code, message, /*Throwable*/null, userInfo); + } + + @Override + public void reject( + String code, + String message, + @Nullable Throwable throwable, + @Nullable WritableMap userInfo + ) { mRejected++; - mErrorCode = code; - mErrorMessage = message; + + if (code == null) { + mErrorCode = ERROR_DEFAULT_CODE; + } else { + mErrorCode = code; + } + + if (message != null) { + mErrorMessage = message; + } else if (throwable != null) { + mErrorMessage = throwable.getMessage(); + } else { + mErrorMessage = ERROR_DEFAULT_MESSAGE; + } } - } + @Override + @Deprecated + public void reject(String message) { + reject(/*Code*/null, message, /*Throwable*/null, /*WritableMap*/null); + } + } }