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.

Sign up for GitHub

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

Jump to bottom

fix: android native rejections should be instanceof Error #44487

Closed
wants to merge 2 commits into from
Closed

fix: android native rejections should be instanceof Error #44487

wants to merge 2 commits into from

Conversation

huzhanbo1996
Copy link
Contributor

Summary:

fix #44050

Changelog:

[ANDROID] [FIXED] - fix: android native rejections should be instanceof Error

Test Plan:

reject returns Error.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label May 8, 2024
@huzhanbo1996
Copy link
Contributor Author

huzhanbo1996 commented May 8, 2024 •
edited
Loading

Hi, @javache

As in #44051, you suggests that a fix from Java to C++. But I found out that same as fix on iOS #41955, we can treat reject on C++.

If we want to fix it from Java, we probably need a Java Error type (currently WritableNativeMap is universal argument for callback), and the argument conversion should support Java Error -> Dynamic -> JSI Object to this specific type. It looks this solution needs huge work.

If you have any other ideas, please let me know.

@facebook-github-bot facebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label May 8, 2024
@analysis-bot
Copy link

analysis-bot commented May 8, 2024 •
edited
Loading

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 19,495,386 -45,446
android hermes armeabi-v7a n/a --
android hermes x86 n/a --
android hermes x86_64 n/a --
android jsc arm64-v8a 22,867,572 -44,417
android jsc armeabi-v7a n/a --
android jsc x86 n/a --
android jsc x86_64 n/a --

Base commit: 54f582f
Branch: main

javache
javache reviewed May 9, 2024
View reviewed changes
...-native/ReactCommon/react/nativemodule/core/platform/android/ReactCommon/JavaTurboModule.cpp Outdated
}

auto createRejectionError(jsi::Runtime& rt, folly::dynamic args) {
DCHECK(args.size() == 1) << "promise reject should has only one argument";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use react_native_assert so we get consistent assert handling

javache
javache reviewed May 9, 2024
View reviewed changes
...-native/ReactCommon/react/nativemodule/core/platform/android/ReactCommon/JavaTurboModule.cpp Outdated
Comment on lines 138 to 139
return;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: make this an else

javache
javache reviewed May 9, 2024
View reviewed changes
...-native/ReactCommon/react/nativemodule/core/platform/android/ReactCommon/JavaTurboModule.cpp Outdated
Comment on lines 112 to 113
jsError.asObject(rt).setProperty(
rt, propertyName, valueAsObject.getProperty(rt, propertyName));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cache jsError.asObject so we don't repeatedly create a new jsi::Value here

javache
javache reviewed May 9, 2024
View reviewed changes
...-native/ReactCommon/react/nativemodule/core/platform/android/ReactCommon/JavaTurboModule.cpp Outdated

auto propertyNames = valueAsObject.getPropertyNames(rt);
for (size_t i = 0; i < propertyNames.size(rt); ++i) {
auto propertyName = propertyNames.getValueAtIndex(rt, i).asString(rt);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrap in PropNameID.forString() to avoid doing this conversion twice.

@javache
Copy link
Member

javache commented May 9, 2024

Looks good overall, just a few nits

@huzhanbo1996
Copy link
Contributor Author

Looks good overall, just a few nits

Thanks for the advice. Changes updated, PTAL again

@facebook-github-bot
Copy link
Contributor

@javache has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@huzhanbo1996
Copy link
Contributor Author

@javache has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Hi @javache, is the CI check OK?

@facebook-github-bot
Copy link
Contributor

@javache merged this pull request in 62cbdbb.

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label May 14, 2024
@github-actions GitHub Actions
Copy link

This pull request was successfully merged by huzhanbo.luc in 62cbdbb.

When will my fix make it into a release? | How to file a pick request?

kosmydel pushed a commit to kosmydel/react-native that referenced this pull request Jun 11, 2024
@huzhanbo1996 @kosmydel
fix: android native rejections should be instanceof Error (facebook#4…
15b5af2 
…4487)

Summary:
fix facebook#44050

## Changelog:

[ANDROID] [FIXED] - fix: android native rejections should be instanceof Error

Pull Request resolved: facebook#44487

Test Plan: reject returns Error.

Reviewed By: NickGerleman

Differential Revision: D57205131

Pulled By: javache

fbshipit-source-id: a5950481d0c4909be4dbea0b430e75222258ae68
Titozzz pushed a commit that referenced this pull request Jun 18, 2024
@huzhanbo1996 @Titozzz
fix: android native rejections should be instanceof Error (#44487)
43ca3de 
Summary:
fix #44050

## Changelog:

[ANDROID] [FIXED] - fix: android native rejections should be instanceof Error

Pull Request resolved: #44487

Test Plan: reject returns Error.

Reviewed By: NickGerleman

Differential Revision: D57205131

Pulled By: javache

fbshipit-source-id: a5950481d0c4909be4dbea0b430e75222258ae68
Titozzz pushed a commit that referenced this pull request Jun 18, 2024
@huzhanbo1996 @Titozzz
fix: android native rejections should be instanceof Error (#44487)
78723a4 
Summary:
fix #44050

## Changelog:

[ANDROID] [FIXED] - fix: android native rejections should be instanceof Error

Pull Request resolved: #44487

Test Plan: reject returns Error.

Reviewed By: NickGerleman

Differential Revision: D57205131

Pulled By: javache

fbshipit-source-id: a5950481d0c4909be4dbea0b430e75222258ae68
Titozzz pushed a commit that referenced this pull request Jun 18, 2024
@huzhanbo1996 @Titozzz
fix: android native rejections should be instanceof Error (#44487)
f4b0fcb 
Summary:
fix #44050

## Changelog:

[ANDROID] [FIXED] - fix: android native rejections should be instanceof Error

Pull Request resolved: #44487

Test Plan: reject returns Error.

Reviewed By: NickGerleman

Differential Revision: D57205131

Pulled By: javache

fbshipit-source-id: a5950481d0c4909be4dbea0b430e75222258ae68
This was referenced Jun 28, 2024
Closed
Merged
@cipolleschi cipolleschi mentioned this pull request Jul 22, 2024
Merged
@cipolleschi cipolleschi mentioned this pull request Jul 31, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@javache javache javache left review comments

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Android native module rejections are not instanceof Error
4 participants
@huzhanbo1996 @analysis-bot @javache @facebook-github-bot