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

Fix handling of uncaught errors #655

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

Conversation

filip131311
Copy link
Collaborator

@filip131311 filip131311 commented Oct 23, 2024

This PR fixes the problem with debugger ignoring uncaught errors after reloading JS. Because errors Global Handler was only set on first connection to the debugger it would not be added after reload. The solution was to move the logic of setting the handler to happen on debugger event that is triggered both on first and consecutive application loads.

How Has This Been Tested:

  1. Run Test application and cause error
  2. reload JS
  3. repet step 1). Before the change it would pass the error to the debug console and not stop the application
  4. Test it both on application using RN version older then 76 and 76.

Additional changes:
this PR also hides related internal logging from the users debug console

Copy link

vercel bot commented Oct 23, 2024

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 Oct 24, 2024 2:21pm

const argsLen = message.params.args.length;
let output: OutputEvent;
if (argsLen > 3 && message.params.args[argsLen - 1].type === "number") {
if (message.params.args[0].value === "__RNIDE_INTERNAL") {
Copy link
Member

Choose a reason for hiding this comment

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

what if there are 0 args?

Comment on lines 206 to 207
// We also check if the log is not internal to avoid exposing it as part of
// application logs.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// We also check if the log is not internal to avoid exposing it as part of
// application logs.
// We filter out logs that start with __RNIDE_INTERNAL as those are messages
// used by IDE for tracking the app state and should not appear in the VSCode
// console.

const argsLen = message.params.args.length;
let output: OutputEvent;
if (argsLen > 3 && message.params.args[argsLen - 1].type === "number") {
if (message.params.args[0].value === "__RNIDE_INTERNAL") {
return;
Copy link
Member

Choose a reason for hiding this comment

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

add comment here to explain why we don't do anything with those logs

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

isn't this in this explained by this comment? 

// We filter out logs that start with __RNIDE_INTERNAL as those are messages
// used by IDE for tracking the app state and should not appear in the VSCode
// console. 

Copy link
Member

Choose a reason for hiding this comment

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

if the log doesn't appear anywhere what's its purpose?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it appears in metro log feed with type: client_log, can be useful for us to debug when runtime started.

Copy link
Member

Choose a reason for hiding this comment

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

yes, that's exacly what you should write here to explain why we just return and do nothing with the log.

@kmagiera
Copy link
Member

One more thing that comes to my mind here is that you should test this with RN 76 with the new debugger, as it may handle reloads differently compared to old one.

@filip131311
Copy link
Collaborator Author

@kmagiera you were right, the debugger creates execution context in a different moment in RN 76, I changed the approach to accommodate both versions.

@@ -144,6 +141,12 @@ export class DebugAdapter extends DebugSession {
case "Debugger.scriptParsed":
const sourceMapURL = message.params.sourceMapURL;

if (message.params.url) {
Copy link
Member

Choose a reason for hiding this comment

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

this deserves some explanation. what does this check do, how often do we execute onDebuggerReady

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