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

[JSCRuntime] Add runtimeConfig to set debugger options #38942

Closed
wants to merge 2 commits into from

Conversation

Saadnajmi
Copy link
Contributor

@Saadnajmi Saadnajmi commented Aug 11, 2023

Summary:

When using React Native with JavascriptCore with a brownfield app, you may have multiple instances of React Native running, each with a JSContext. You attach a debugger to this JSContext via the Safari->Develop menu. To distinguish multiple JSContexts apart, JSContext has a name property, with a C style setter. We currently don't take advantage of that property, meaning that all your React Native instances show in the Develop menu with the default name "JSContext", making them hard to distinguish from each other.

Let's solve this by adding a runtimeConfig parameter to the method makeJSCRuntime(), and pass through the properties enableDebugger and debuggerName to it. These names / the API to set them are heavily inspired by a similar change made to HermesExecutorFactory: 32d12e8

Changelog:

[IOS] [CHANGED] - Add facebook::jsc::runtimeConfig to set enable debugger and set debugger

Test Plan:

Running RNTester with JSC shows the JSContext with the updated name.

Screenshot 2023-08-16 at 5 48 08 AM

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Microsoft Partner: Microsoft Partner Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Aug 11, 2023
@analysis-bot
Copy link

analysis-bot commented Aug 11, 2023

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 17,657,546 +23,343
android hermes armeabi-v7a n/a --
android hermes x86 n/a --
android hermes x86_64 n/a --
android jsc arm64-v8a 21,039,214 +21,155
android jsc armeabi-v7a n/a --
android jsc x86 n/a --
android jsc x86_64 n/a --

Base commit: d2e8225
Branch: main

Copy link
Contributor

@motiz88 motiz88 left a comment

Choose a reason for hiding this comment

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

Hey @Saadnajmi! I agree with the intent of this PR and would like to ship something close to this, but see some concerns/suggestions in comments.

packages/react-native/ReactCommon/jsc/JSCRuntime.cpp Outdated Show resolved Hide resolved
packages/react-native/ReactCommon/jsc/JSCRuntime.cpp Outdated Show resolved Hide resolved
@Saadnajmi
Copy link
Contributor Author

Saadnajmi commented Aug 16, 2023

Closing this PR till I rework it with the feedback above.
EDIT: which I did a couple of hours later 😅

@Saadnajmi Saadnajmi closed this Aug 16, 2023
@Saadnajmi Saadnajmi reopened this Aug 16, 2023
@Saadnajmi Saadnajmi changed the title [JSCRuntime] Set JSContext name to JSCRuntime description [JSCRuntime] Add runtimeConfig to set enable debugger and set debugger name Aug 16, 2023
@Saadnajmi Saadnajmi marked this pull request as draft August 16, 2023 12:57
@Saadnajmi Saadnajmi changed the title [JSCRuntime] Add runtimeConfig to set enable debugger and set debugger name [JSCRuntime] Add runtimeConfig to set debugger options Aug 16, 2023
@Saadnajmi Saadnajmi requested a review from motiz88 August 22, 2023 02:59
@Saadnajmi
Copy link
Contributor Author

@motiz88 friendly ping, in case you could take a second look

Copy link
Contributor

@motiz88 motiz88 left a comment

Choose a reason for hiding this comment

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

So sorry about the delay, but I have some concerns here (plus some nits).

packages/react-native/ReactCommon/jsc/JSCRuntime.h Outdated Show resolved Hide resolved
packages/react-native/ReactCommon/jsc/JSCRuntime.cpp Outdated Show resolved Hide resolved
packages/react-native/ReactCommon/jsc/JSCRuntime.cpp Outdated Show resolved Hide resolved
packages/react-native/ReactCommon/cxxreact/JSExecutor.h Outdated Show resolved Hide resolved
packages/react-native/ReactCommon/jsc/JSCRuntime.cpp Outdated Show resolved Hide resolved
@Saadnajmi
Copy link
Contributor Author

@motiz88 Could you take another look? I've addressed your new set of comments

Comment on lines +30 to +34
#if DEBUG
bool enableDebugger_ = true;
#else
bool enableDebugger_ = false;
#endif
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oversight caught internally: Initially this was always true so release builds would have the debugger enabled. I changed it so the default is true only for debug. This way, one could still enable debugging for a ship/release build if they wished.

I considered copying what Hermes does and putting all debugging code behind a custom preprocessor variable (#37723), but that seemed overkill and would prevent us from enable debugging on ship.
For reference, equivalent PRs in React Native macOS:
microsoft#1976
microsoft#1977
microsoft#1978
microsoft#1979

Copy link

github-actions bot commented May 6, 2024

This PR is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days.

@github-actions github-actions bot added the Stale There has been a lack of activity on this issue and it may be closed soon. label May 6, 2024
Copy link

This PR was closed because it has been stalled for 7 days with no activity.

@github-actions github-actions bot closed this May 14, 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. p: Microsoft Partner: Microsoft Partner Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. Stale There has been a lack of activity on this issue and it may be closed soon.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants