Tweaked captured error log slightly based on feedback #8785

bvaughn · 2017-01-14T00:24:27Z

Before

After

Chrome

Firefox

Safari

Internet Explorer

Edge

gaearon · 2017-01-14T00:27:54Z

What concerns me here is it's easy to miss the error message. Could we put it above component stack, but leave the function stack below?

acdlite · 2017-01-14T00:28:38Z

I agree with Dan. Error message, then component stack, then JS stack

bvaughn · 2017-01-14T00:29:33Z

Right-o. Will do.

bvaughn · 2017-01-14T00:34:57Z

Ok. New screenshot.

acdlite · 2017-01-14T00:39:38Z

Can we remove the second error message and just show the stack? Like

JS stack:
  at alsdjfaksdlj.js 999
  at alsdfjaklsdfkj.js 999

gaearon · 2017-01-14T01:50:58Z

Was also about to suggest this. I'd use "The error was thrown at:" instead of "JS Stack:" though.

bvaughn · 2017-01-14T03:49:33Z

Sure, I can remove the redundant error message.

"The error was thrown at" seems awful similar to "The error was thrown in the following location". How about "The error is located at" for the component stack and "The error was thrown at" for the call stack?

gaearon · 2017-01-14T13:09:35Z

src/renderers/shared/fiber/ReactFiberErrorLogger.js

@@ -33,6 +33,9 @@ function logCapturedError(capturedError : CapturedError) : void {
      ? `React caught an error thrown by ${componentName}.`
      : 'React caught an error thrown by one of your components.';

+    const callStack = error.stack;
+    const trimmedCallStack = callStack.substring(callStack.indexOf('\n'));


This is not consistent between browsers.
For example Firefox or Safari don’t seem to put the message in stack.

I suggest checking if the first line of the stack ends with error message. If it does, cut it out, otherwise keep it.

I had no idea. Thanks for the catch. I'll push up a small fix shortly.

gaearon · 2017-01-14T13:10:08Z

src/renderers/shared/fiber/ReactFiberErrorLogger.js

@@ -33,6 +33,9 @@ function logCapturedError(capturedError : CapturedError) : void {
      ? `React caught an error thrown by ${componentName}.`
      : 'React caught an error thrown by one of your components.';

+    const callStack = error.stack;


On line 29, I think you should use error.name instead of "Error" because it might be a TypeError or something custom.

bvaughn · 2017-01-14T17:40:50Z

Okay! Error stack format has been tidied up for consistency across browsers. Screenshots have been added for each. 😄

Thanks for the catch @gaearon!

bvaughn · 2017-01-14T17:52:47Z

PS Chrome's formatting of errors is head and shoulders above 🙇

Firefox's is not so great. Yellow text on a red background. ☹️

gaearon · 2017-01-14T19:09:21Z

src/renderers/shared/fiber/ReactFiberErrorLogger.js

+    } = error;
+
+    const errorSummary = message
+      ? `${name}: ${message}\n\n`
      : '';


Would be nice to include the name even if there's no message?

Hm, yes I suppose it would be. 😄

gaearon · 2017-01-14T19:11:17Z

src/renderers/shared/fiber/ReactFiberErrorLogger.js

-    const callStack = error.stack;
-    const trimmedCallStack = callStack.substring(callStack.indexOf('\n'));
+    // Error stack varies by browser; normalize it for our logging.
+    let formattedCallStack = stack.indexOf(message) >= 0


I think it would be safer to split the first line first. Then check the first line for matching the message.
Otherwise this code behave in a weird way if the message was not on the first line.

Now that I think of it, messages could also be multiline. So maybe it's best to check for a very specific pattern that Chrome does (and cut it out based on where the message ends), and not touch it in any other case.

Agreed. Hadn't considered the multiline case. Shouldn't code before ☕️ 😁

I think this version (just pushed) is more robust. Tested various browsers with single-line message, multi-line message, and no message.

sophiebits · 2017-01-15T00:12:20Z

src/renderers/shared/fiber/ReactFiberErrorLogger.js

    const componentNameMessage = componentName
      ? `React caught an error thrown by ${componentName}.`
      : 'React caught an error thrown by one of your components.';

+    // Error stack varies by browser; normalize it for our logging.
+    const formattedCallStack = stack.replace(new RegExp(`^${errorSummary}\n`), '')


This would need to be escaped in case it includes regex metachars. How about just:

if (stack.slice(0, errorSummary.length) === errorSummary) { stack = stack.slice(errorSummary.length); } if (stack.charAt(0) === '\n') { stack = stack.slice(1); }

Thanks for pointing that out Ben.

The latest commit has been tested with single-line message, multi-line message, no message, and messages containing special regex chars.

Browsers tested: Chrome, Firefox, Safari, IE

sophiebits · 2017-01-15T00:12:43Z

src/renderers/shared/fiber/ReactFiberErrorLogger.js

-      `The error was thrown in the following location: ${componentStack}`
+      `${errorSummary}\n\n` +
+      `The error is located at: ${componentStack}\n\n` +
+      `The error was thrown at: ${formattedCallStack}`


I think you could also get away with just showing the call stack without any intro line before it.

Edge-case in message content and browser formatting differences are why I initially just logged error.stack by itself. The "thrown at" header and stripping the redundant error message (in the case of Chrome) were suggestions from others (above).

Though FWIW I think the stack, by itself, is not as intuitive without the header and formatting for certain browsers.

(y) I'm good with this or just "Call stack:" then.

bvaughn · 2017-01-17T17:25:54Z

Hey @gaearon 😄 Thanks again for the review. Any other changes you want to see on this PR?

sophiebits

lgtm

sophiebits · 2017-01-17T17:33:55Z

src/renderers/shared/fiber/ReactFiberErrorLogger.js

-      `The error was thrown in the following location: ${componentStack}`
+      `${errorSummary}\n\n` +
+      `The error is located at: ${componentStack}\n\n` +
+      `The error was thrown at: ${formattedCallStack}`


(y) I'm good with this or just "Call stack:" then.

bvaughn · 2017-01-17T22:31:40Z

I think Dan's out for the week on vacation now. Since this has 2 approvals, and no remaining requested changes from him, I'm going to go ahead and merge it. 😄 Can always tweak it further in the future.

sophiebits · 2017-01-17T23:12:49Z

Dan approved it, you know. :)

bvaughn · 2017-01-17T23:15:04Z

Weird. Maybe that Github lag thing he was mentioning. My Github did not show Dan as approving it at the time I left that comment. 😁

gaearon · 2017-01-18T06:12:11Z

GitHub loves caching. I Cmd+R it all the time.

bvaughn assigned gaearon Jan 14, 2017

facebook-github-bot added CLA Signed GH Review: review-needed labels Jan 14, 2017

Tweaked captured error log slightly based on feedback

7b2914c

bvaughn force-pushed the captured-error-log-bikeshedding branch from 5467a66 to 7b2914c Compare January 14, 2017 00:34

sebmarkbage approved these changes Jan 14, 2017

View reviewed changes

Tweaked wording further

2c64f1b

gaearon requested changes Jan 14, 2017

View reviewed changes

gaearon reviewed Jan 14, 2017

View reviewed changes

bvaughn force-pushed the captured-error-log-bikeshedding branch from 015f82b to 937d13a Compare January 14, 2017 20:38

sophiebits reviewed Jan 15, 2017

View reviewed changes

Normalized stack format/display for different browsers

7902e81

bvaughn force-pushed the captured-error-log-bikeshedding branch from 937d13a to 7902e81 Compare January 15, 2017 01:07

sophiebits approved these changes Jan 17, 2017

View reviewed changes

gaearon approved these changes Jan 17, 2017

View reviewed changes

bvaughn merged commit 16abcef into facebook:master Jan 17, 2017

bvaughn deleted the captured-error-log-bikeshedding branch January 17, 2017 22:32

bvaughn mentioned this pull request Mar 15, 2017

Expose DEV-mode warnings in devtools UI #9189

Closed

Tweaked captured error log slightly based on feedback #8785

Tweaked captured error log slightly based on feedback #8785

Uh oh!

Conversation

bvaughn commented Jan 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Before

After

Chrome

Firefox

Safari

Internet Explorer

Edge

Uh oh!

gaearon commented Jan 14, 2017

Uh oh!

acdlite commented Jan 14, 2017

Uh oh!

bvaughn commented Jan 14, 2017

Uh oh!

bvaughn commented Jan 14, 2017

Uh oh!

acdlite commented Jan 14, 2017

Uh oh!

gaearon commented Jan 14, 2017

Uh oh!

bvaughn commented Jan 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bvaughn commented Jan 14, 2017

Uh oh!

bvaughn commented Jan 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaearon Jan 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bvaughn Jan 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bvaughn commented Jan 17, 2017

Uh oh!

sophiebits left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bvaughn commented Jan 17, 2017

Uh oh!

sophiebits commented Jan 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bvaughn commented Jan 17, 2017

Uh oh!

gaearon commented Jan 18, 2017

Uh oh!

Uh oh!

bvaughn commented Jan 14, 2017 •

edited

Loading

bvaughn commented Jan 14, 2017 •

edited

Loading

bvaughn commented Jan 14, 2017 •

edited

Loading

gaearon Jan 14, 2017 •

edited

Loading

bvaughn Jan 15, 2017 •

edited

Loading

sophiebits commented Jan 17, 2017 •

edited

Loading