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

Start separate debug adapters for Metro and JS debugging #1008

Merged
merged 3 commits into from
Mar 7, 2025

Conversation

jwajgelt
Copy link
Contributor

@jwajgelt jwajgelt commented Mar 5, 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 requested a review from filip131311 March 5, 2025 08:57
@jwajgelt jwajgelt self-assigned this Mar 5, 2025
Copy link

vercel bot commented Mar 5, 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 6, 2025 9:06am

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.

Left some inline comments

this.connectJSDebugger(configuration as unknown as CDPConfiguration);
}

private connectJSDebugger(cdpConfiguration: CDPConfiguration) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am getting confused by the nameing convention here, is JS debugger the parrent debugger or the debugger for applications, I think it would be good to introduce consistant naming convention for example: JSDebuger or parentDebugger for the top one and cdpDebugger, applicationDebugger or ReactNtiveDebugger for the bottom one.

Note: I know it might be me mistake in #964 originally sorry about that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"JS Debugger" is the child of the "React Native Debugger" (which just forwards the metro logs at this point), not the other way around.
This method should probably be just called startCDPSession or connectToApplication, since we're already in the "debugger" (or rather "debugger adapter") implementation here.

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.

LGTM

@jwajgelt jwajgelt force-pushed the @jwajgelt/refactor_debug_adapter branch from 1f33ec7 to ec228d3 Compare March 6, 2025 09:06
@jwajgelt jwajgelt requested a review from filip131311 March 6, 2025 09:24
@jwajgelt jwajgelt merged commit ea9042e into main Mar 7, 2025
4 checks passed
@jwajgelt jwajgelt deleted the @jwajgelt/refactor_debug_adapter branch March 7, 2025 07:43
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