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

Use vscode-js-debug as the debugger implementation #1001

Open
wants to merge 25 commits into
base: main
Choose a base branch
from

Conversation

jwajgelt
Copy link
Contributor

@jwajgelt jwajgelt commented Mar 3, 2025

Uses js-debug's implementation of the JS debugger for targets which support it (Hermes with RN version >= 0.76).
This allows us to use the features already implemented there (exception breakpoints, conditional breakpoints) without having to reimplement them, and possibly allows us to remove our CDP debugger implementation in the future, once we drop support for the older RN versions.

We patch vscode-js-debug in order to:

  • create a new session type used by Radon, based on js-debug's node debug session type
  • disable the debug toolbar for that session
  • disable the use of proposed vscode extension APIs which prevent the extension from building

We use the vscode-cdp-proxy package for implementing the CDP proxy sitting between the js-debug debugger implementation and the application.
We patch it to:

  • fix a bug which incorrectly treats CDP commands as replies when they include the id field
  • fix a bug which exports an nonexistent module from the package

How Has This Been Tested:

  • launch various apps from test-apps repo
  • verify that in each app, the debugger still works, and:
    • the debugger toolbar doesn't appear (we provide our own controls when a breakpoint is hit)
    • you can set "caught/uncaught exception" breakpoints and they work
    • setting regular breakpoints work correctly
    • setting conditional breakpoints work
    • logs are routed to the debug console
    • pausing inside RN code works correctly (i.e. when handling caught exceptions)
    • running the CPU profiler works and maps the source locations correctly

Copy link

vercel bot commented Mar 3, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
radon-ide ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 11, 2025 2:55pm

jwajgelt added a commit that referenced this pull request Mar 7, 2025
Refactors the changes introduced in #964 to separate logging metro
messages and JS debugging into separate DAP Sessions. This will also
make integrating #1001 easier with other changes made to `DebugAdapter`.

### How Has This Been Tested: 
Open an app and check the debugger still works correctly:
- breakpoints and uncaught errors should still stop the application
- debugger controls in the preview should resume/step over breakpoints
- check profiling still works
- check if Metro errors are logged to the debug console.
@jwajgelt jwajgelt marked this pull request as ready for review March 10, 2025 10:03
Copy link
Collaborator

@filip131311 filip131311 left a comment

Choose a reason for hiding this comment

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

I found three problems with this implementation:

  1. RNIDE_log is never trigered
  2. uncaught exceptions are disabled, by default in this implementation which is a change compared to the previous one and might cause users to think that they stoped working, also this is a departure from "unhandled exception overlay"
  3. the stack of unhandled exception seems to be lacking some parts in react-native-78 test app when you click on throw error button
image
Screen.Recording.2025-03-10.at.19.46.39.mov

@jwajgelt
Copy link
Contributor Author

I found three problems with this implementation:

1. RNIDE_log is never trigered

You mean "RNIDE_consoleLog"?
You're right, I didn't notice we do anything other than print them to the debug console (which is handled by js-debug) with this event. It should be fine to emit it.

2. uncaught exceptions are disabled, by default in this implementation which is a change compared to the previous one and might cause users to think that they stoped working, also this is a departure from "unhandled exception overlay"

It is, but it's also by design. Since we've got the toggle now, I think we should respect it instead of always handling the thrown exceptions.
This does result in a difference in behavior between the old and new debugger now, which may be confusing the user, but that's a wider issue with this feature that I'm not sure there's much we can do about (maybe other than slapping a warning when the old debugger is running that not all features are supported.)

3. the stack of unhandled exception seems to be lacking some parts in `react-native-78` test app when you click on throw error button

Yep.
That's because RN actually catches and rethrows the errors in its implementation (several times). When you check "uncaught exceptions", what you actually get is only the last rethrow, coming from the innards of RN.
You can get the debugger to stop on your exception by checking "caught exceptions" instead.
On one hand, this is not as nice as the system we had before, where you would be pointed to the place the exception was thrown.
On the other, this way you can (provided you check "Caught exceptions") actually inspect the state of the application at the throw site, which I'd say is more useful.
This is also consistent with how the React Native DevTools handle exception breakpoints, though that doesn't mean it's the right way to do that.

In this case, I feel that changing the behavior in this way provides a better experience.
Keeping the old overlay would provide an experience where, depending on whether you're in a "breakpoint" or "exception", you get wildly different behavior of the debugger with the same UI, which I'd say is pretty confusing.
Maybe we can improve the messaging (get rid of "Uncaught exceptions" as they are mostly useless, have just "Exceptions", maybe even filter ones which are not thrown from user code or at least those thrown from RN internals), but I'm not convinced that should go into the initial implementation of this feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants