Tweaked captured error log slightly based on feedback #8785
6 participants
|
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? |
|
I agree with Dan. Error message, then component stack, then JS stack |
|
Right-o. Will do. |
|
Ok. New screenshot. |
|
Can we remove the second error message and just show the stack? Like |
|
Was also about to suggest this. I'd use "The error was thrown at:" instead of "JS Stack:" though. |
|
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? |
| @@ -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.
| @@ -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.
|
Okay! Error stack format has been tidied up for consistency across browsers. Screenshots have been added for each. Thanks for the catch @gaearon! |
|
PS Chrome's formatting of errors is head and shoulders above Firefox's is not so great. Yellow text on a red background. |
| + } = error; | ||
| + | ||
| + const errorSummary = message | ||
| + ? `${name}: ${message}\n\n` | ||
| : ''; |
| - 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.
| 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
| - `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.
|
Hey @gaearon |
| - `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 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. |
1 check passed
|
Dan approved it, you know. :)
|
|
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. |
|
GitHub loves caching. I Cmd+R it all the time. |
Before
After
Chrome
Firefox
Safari
Internet Explorer
Edge