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

Enable app reloads for other runtimes than Hermes React Native #864

Closed
wants to merge 10 commits into from
Closed

Conversation

Kwasow
Copy link

@Kwasow Kwasow commented Sep 6, 2022

Motivation

Hi, I'm a member of the react-native-reanimated team at Software Mansion, where we develop a module for React Native. Due to the architecture of this module, we need to create our own runtime. It has long been the case that code being executed in our runtime environment, could not have been debugged using most debugging tools. This is due to change soon, as the work done in this PR enables full support for debugging using Chrome DevTools, when using Hermes as the runtime.

Unfortunately, this runtime makes the app crash if a reload is performed. The same happens for the default React Native runtime. This is fixed by the metro-inspector-proxy package, that creates a React Native Experimental (Improved Chrome Reloads) “virtual” runtime, which doesn't have this issue and handles changes due to app reloads.

Similar issues may be encountered by brownfield apps, which can have multiple JS runtimes.

Proposed solution

I've updated the implementation of Device.js in the metro-inspector-proxy package for it to be more general. Previously it would only create a “virtual” runtime for the Hermes React Native runtime. Now it uses a map and dynamically creates a reloadable “virtual” runtime for every registered runtime and then handles reloads for all of them.

Test plan

The output from localhost:8081/json is correct and debugging in Flipper works (both for the React Native runtime and our own Reanimated runtime).

Related PRs

@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 Sep 6, 2022
@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 Sep 6, 2022
piaskowyk pushed a commit to software-mansion/react-native-reanimated that referenced this pull request Sep 22, 2022
## 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)
@codecov-commenter
Copy link

Codecov Report

Base: 79.73% // Head: 79.67% // Decreases project coverage by -0.06% ⚠️

Coverage data is based on head (cbcfe05) compared to base (553241c).
Patch coverage: 2.85% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #864      +/-   ##
==========================================
- Coverage   79.73%   79.67%   -0.07%     
==========================================
  Files         210      210              
  Lines       10671    10680       +9     
  Branches     2584     2587       +3     
==========================================
  Hits         8509     8509              
- Misses       2162     2171       +9     
Impacted Files Coverage Δ
packages/metro-inspector-proxy/src/Device.js 2.80% <2.85%> (-0.13%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@robhogan robhogan self-assigned this Jan 4, 2023
fluiddot pushed a commit to wordpress-mobile/react-native-reanimated that referenced this pull request Jun 5, 2023
…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)
@Kwasow Kwasow closed this by deleting the head repository Apr 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants