Skip to content
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

[android] Allow 'userInfo' for native promise.reject + add native error stack support #20940

Closed
wants to merge 12 commits into from

Conversation

Projects
None yet
8 participants
@Salakar
Copy link
Collaborator

commented Sep 1, 2018

Changes

As mentioned here, 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) and Windows (here) already support this so this is a relatively minor addition to bring Android in line with the other platforms. (JS support is also here)

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.

Reason for changes

  • Currently inconsistent with other platforms.
  • Blocking a couple of issues over at 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.

Test Plan:

  1. Fixed all build/test issues on master for android, detox & obj-c CI stages and sent up a PR which is now in the process of being imported - #20854. CI for this PR will only pass after that has been merged into master - will rebase when this has happened.
  2. With the fixes from '1' locally applied I ran the following suites/scripts with no issues:
    2.1. npm run test - Jest tests
    2.2. ./scripts/run-android-local-integration-tests.sh
    2.3. ./scripts/run-android-local-unit-tests.sh
    2.4. ./scripts/test-manual-e2e.sh - built Android and ran the app, clicked through several samples/examples including the Share one with no issues.
  3. [✈️🔥] Switched over invertase/react-native-firebase tests app (powered by Detox & Jet) to build from RN Android source with the diff from this PR applied. All our e2e test suites ran with no issues - several hundred methods using Androids native Promise Implementation tested with no issues.
  4. [✈️] For bonus points and sanity checking: using Jet in our tests project (3) I built an extensive e2e test suite in our tests app (3) to test every native promise method (resolve & reject) and the new additions. Code can be see in the following places:
    4.1. /com/testing/RNPromiseTest.java - native module exposing all variations of PromiseImpl's methods
    4.2. /tests/e2e/promise.e2e.js JS test suite directly calling these methods from 4.1 and validating codes, messages, default codes & messages, types, instanceof checks and nativeStack frames.

Imgur

Feedback

Would be good to get some feedback on the following:

  • void reject(String message); - can this deprecated method be removed? It's several versions old and I couldn't find any usages of it anymore.
  • nativeStack* platform format - the support I added for nativeStackAndroid provides an array of Stack Frames in the same format as already utilised in RN JavaScript code here - this makes it easier to convert to a JS like stack trace if needed or for Crash/Error reporting frameworks to utilise. However, the pre-existing iOS support at nativeStackIOS provides an unparsed array of call stack symbols which personally I'm not too keen on. What may be good is if both platforms return the same Stack Frame format as implemented on Android in this PR. Once both platforms are in the same format they can then just use nativeStack rather than the current post-fixed key names. Feedback would be appreciated :) I could unify these in a subsequent PR based on the feedback - right now not a major issue as the keys are post-fixed by platform.
  • Stack Frames are hard set at 10 frames - easily changable but not from userland code. 10 too much too little?

Release Notes:

  • [ANDROID] [ENHANCEMENT] [NativeModules] - added support for rejecting a promise with an additional WritableMap arg for extra properties (userInfo). See the inteface defined in Promise.java for available methods. Accessible in JavaScript as Error.userInfo. This is to match iOS's existing Error.userInfo support.
  • [ANDROID] [ENHANCEMENT] [NativeModules] - added a nativeStackAndroid property to promises rejected with an Exception/Throwable - making native error stacks available inside Javascript: Error.nativeStackAndroid. This is to match iOS's existing Error.nativeStackIOS support.

Salakar added some commits Aug 25, 2018

[android] remove inconsistent promise.reject with null arg - unnecess…
…ary and not done anywhere else - prevents a 'ambiguous method' error as rejects 3rd arg can be Throwable or Writable map.
[android] Promise implementation changes
Cleanup to match the interface and additionally adds support/fixes for `TODO(8850038)`
[android] update ShareModuleTest SimplePromise
Updated to match `PromiseImpl` changes.
[android][PromiseImpl] add stackTrace length check to ensure we don't…
… cause an `IndexOutOfBounds` exception
@kelset

This comment has been minimized.

Copy link
Collaborator

commented Sep 20, 2018

@dulmandakh would you mind reviewing this PR? It seems it's right up your alley

cpojer added some commits Dec 11, 2018

@pull-bot

This comment has been minimized.

Copy link

commented Dec 11, 2018

Warnings
⚠️

📋 Changelog - This PR appears to be missing Changelog.

Generated by 🚫 dangerJS

@facebook-github-bot
Copy link

left a comment

@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@react-native-bot

This comment has been minimized.

Copy link
Collaborator

commented Dec 11, 2018

@Salakar merged commit 794d226 into facebook:master.

@facebook facebook locked as resolved and limited conversation to collaborators Dec 11, 2018

grabbou added a commit that referenced this pull request Dec 17, 2018

Allow 'userInfo' for native promise.reject + add native error stack s…
…upport (#20940)

Summary:
As mentioned [here](react-native-community/releases#34 (comment)), 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](microsoft/react-native-windows#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: #20940

Differential Revision: D13412527

Pulled By: cpojer

fbshipit-source-id: 2ca6c5f3db9ff2c2986b02edda80bc73432f66d3

matt-oakes pushed a commit to matt-oakes/react-native that referenced this pull request Feb 7, 2019

Allow 'userInfo' for native promise.reject + add native error stack s…
…upport (facebook#20940)

Summary:
As mentioned [here](react-native-community/releases#34 (comment)), 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](microsoft/react-native-windows#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: facebook#20940

Differential Revision: D13412527

Pulled By: cpojer

fbshipit-source-id: 2ca6c5f3db9ff2c2986b02edda80bc73432f66d3
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.