-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Fix runtime creation #1960
Fix runtime creation #1960
Conversation
android/src/main/java/com/swmansion/reanimated/NativeProxy.java
Outdated
Show resolved
Hide resolved
@piaskowyk I tried pulling this PR down locally and running it but it didn't seem to fix the issue on Android. I still am getting a crash when connecting the Hermes Debugger in Flipper. |
android/src/main/JNI/Android.mk
Outdated
LOCAL_STATIC_LIBRARIES := libjsi callinvokerholder | ||
LOCAL_SHARED_LIBRARIES := libfolly_json libfbjni libreactnativejni | ||
LOCAL_STATIC_LIBRARIES := libjsi callinvokerholder libhermes-inspector | ||
LOCAL_SHARED_LIBRARIES := libhermes libfolly_json libfbjni libreactnativejni |
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.
just curious that if adding libhermes here, will mark libhermes.so as NEEDED in libreanimated.so again?
For JSC apps to load libreanimated.so, there should be errors as libhermes.so is not packed into apk.
btw thank you @piaskowyk for the awesome work trying to decouple hermes in reanimated 👍
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 think it won't work because RN doesn't provide libhermes.so if you choose to use jsc.
@@ -136,6 +144,7 @@ if(${FOR_HERMES}) | |||
${PACKAGE_NAME} | |||
${LOG_LIB} | |||
${HERMES_LIB} | |||
${HERMES_INSPECTOR_LIB} |
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 believe Hermes inspector is available only in debug build so we cannot merge this in the current shape.
I want to back to this in the future. |
## Description Replacement for PR #1960 ## Checklist - [x] Ensured that CI passes
@piaskowyk i fixed the v8 here are some of my proposals:
|
Point 4. sounds very interesting. I've also thought about simply exposing their code to create a runtime instead, which can potentially be overwritten by other runtimes (v8, or more). |
Just wanted to add that @Kudo has been extremely helpful, and I've tried his This would be huge for all of us v8 users. At a minimum, bundling the v8 AAR inside reanimated would be a great first step. |
@piaskowyk sorry to bother you, but are there any updates on v8 support? |
## Description This PR is related to #1960 (and its replacement #2047). It adds the ability to use Chrome DevTools to add breakpoints and debug worklets (or the UI context in general) both on Android and iOS. ## Major changes Runtime creation has been moved to `ReanimatedRuntime` for both Android and iOS (in the `Common/cpp/ReanimatedRuntime` directory). Before (Android) [file: `NativeProxy.cpp`]: ```cpp #if JS_RUNTIME_HERMES auto config = ::hermes::vm::RuntimeConfig::Builder().withEnableSampleProfiling(false); std::shared_ptr<jsi::Runtime> animatedRuntime = facebook::hermes::makeHermesRuntime(config.build()); #elif JS_RUNTIME_V8 … #else … #endif ``` After (shared) [file: `NativeProxy.cpp`]: ```cpp ReanimatedRuntime::make(jsQueue); ``` The custom build config has been removed, as sample profiling is set to false by default. The created `HermesRuntimeManager` object is the stored inside `ReanimatedNativeModule` as it is important that it’s lifetime is synced with the lifetime of the module. ## Testing - [x] Builds on Android - [x] Builds on Android in release mode - [x] App reloads work on Android - [x] Builds on iOS - [x] Builds on iOS in release mode - [x] App reloads work on iOS - [x] JSC still works - [x] JSC debugging in Safari with iOS still works - [x] Paper still works Versions of React-Native tested: 0.70 ## Things to look into - ~~Breakpoint labels do not appear on iOS~~ _I tested this on a new app with Flipper on the main JS thread, and it was also the case, so it seem to not be an issue on our side_ - ~~After removing a breakpoint on iOS it can't be set in the same location again~~ _This seems to be an issue with Chrome DevTools as connecting to Reanimated's runtime through Flipper fully works_ - ~~Edge-case: the app will crash if a reload is performed while the debugger is open~~ _Will be fixed in a PR to metro and [58e9e7b](https://github.com/software-mansion/react-native-reanimated/pull/3526/commits/58e9e7bcd7d3b6b590a5f6fca0db93a951eaa39e)_ - The latest release of Chrome (105) broke source maps, so only Flipper works now. I reported the issue [here](https://bugs.chromium.org/p/chromium/issues/detail?id=1360298#makechanges), and I'm waiting for a response from the Chromium team ## TODO - [x] Open PR to update debugging docs in documentation (will be included in #3446) - [x] Open PR in [facebook/flipper](https://github.com/facebook/flipper) to enable Flipper support on custom runtimes (facebook/flipper#4047) - [x] Open PR in [facebook/metro](https://github.com/facebook/metro) to support debugger reloads on custom runtimes (facebook/metro#864)
…are-mansion#3526) ## Description This PR is related to software-mansion#1960 (and its replacement software-mansion#2047). It adds the ability to use Chrome DevTools to add breakpoints and debug worklets (or the UI context in general) both on Android and iOS. ## Major changes Runtime creation has been moved to `ReanimatedRuntime` for both Android and iOS (in the `Common/cpp/ReanimatedRuntime` directory). Before (Android) [file: `NativeProxy.cpp`]: ```cpp #if JS_RUNTIME_HERMES auto config = ::hermes::vm::RuntimeConfig::Builder().withEnableSampleProfiling(false); std::shared_ptr<jsi::Runtime> animatedRuntime = facebook::hermes::makeHermesRuntime(config.build()); #elif JS_RUNTIME_V8 … #else … #endif ``` After (shared) [file: `NativeProxy.cpp`]: ```cpp ReanimatedRuntime::make(jsQueue); ``` The custom build config has been removed, as sample profiling is set to false by default. The created `HermesRuntimeManager` object is the stored inside `ReanimatedNativeModule` as it is important that it’s lifetime is synced with the lifetime of the module. ## Testing - [x] Builds on Android - [x] Builds on Android in release mode - [x] App reloads work on Android - [x] Builds on iOS - [x] Builds on iOS in release mode - [x] App reloads work on iOS - [x] JSC still works - [x] JSC debugging in Safari with iOS still works - [x] Paper still works Versions of React-Native tested: 0.70 ## Things to look into - ~~Breakpoint labels do not appear on iOS~~ _I tested this on a new app with Flipper on the main JS thread, and it was also the case, so it seem to not be an issue on our side_ - ~~After removing a breakpoint on iOS it can't be set in the same location again~~ _This seems to be an issue with Chrome DevTools as connecting to Reanimated's runtime through Flipper fully works_ - ~~Edge-case: the app will crash if a reload is performed while the debugger is open~~ _Will be fixed in a PR to metro and [58e9e7b](https://github.com/software-mansion/react-native-reanimated/pull/3526/commits/58e9e7bcd7d3b6b590a5f6fca0db93a951eaa39e)_ - The latest release of Chrome (105) broke source maps, so only Flipper works now. I reported the issue [here](https://bugs.chromium.org/p/chromium/issues/detail?id=1360298#makechanges), and I'm waiting for a response from the Chromium team ## TODO - [x] Open PR to update debugging docs in documentation (will be included in software-mansion#3446) - [x] Open PR in [facebook/flipper](https://github.com/facebook/flipper) to enable Flipper support on custom runtimes (facebook/flipper#4047) - [x] Open PR in [facebook/metro](https://github.com/facebook/metro) to support debugger reloads on custom runtimes (facebook/metro#864)
Description
In #1842 we use runtime factory from React Native. This factory can decorate runtime by chrome debugger - code here.
facebook::hermes::inspector::chrome::enableDebugging(std::move(adapter), "Hermes React Native");
This line registers runtime in the Metro server with the name
Hermes React Native
. When we use the same runtime factory to create reanimated runtime, we override React Native runtime in the Metro server because both runtimes have the same name. This happens just with Hermes runtime, so I changed the place where we create runtime and I moved the creation of Hermes runtime to C++ part. Now Metro server can see both runtimes.Output form Metro server http://localhost:8081/json
json data
I decided to use
getDefaultJSExecutorFactory()
method from React Native, to avoid the compilation of Reanimated AAR's files for every JS engine. This a little tricky because I need access to the private method unfortunately but thanks to this we still need just 3 AARs. In additionally this PR should automatically add support for V8 runtime because the library with the V8 engine for React Native replacing the body ofgetDefaultJSExecutorFactory()
method to method returning just V8 runtime - code here.This PR is also the first big step to make Reanimated debuggable by Flipper or chrome debugger.
Fixes #1955
Fixes #1923
Update about V8:
I can start app with V8 runtime and use V8 as Reanimated runtime, but I found the issue in
callWithThis()
- this not setjsThis
properly here, in effect ProxySetter and SharedValue don't work. This looks like bug in V8 implementation.Checklist