[general][changed] cleanup RedBox message and stack output #24662

thymikee · 2019-04-30T10:56:05Z

Summary

Cleanup RedBox messages and stack traces. This PR consists of 2 changes (I'm good with splitting them up if you'd like):

[general] filter out some of the internal callsites from the symbolicated stack (I thought about using monospace font for title with code frame, but it looks weird)
[ios][android] strip ANSI characters (coming from colored Babel code frame) from the error message

I think it's ok to strip it inside native handlers so we can still have a colorful code frame in the terminal output.

JS Code frame:

before	after

before	after

Filtered stack traces:

before	after

There's still a lot of places that are hard to read, but I think this is a good start towards more readable errors.

cc @cpojer

Changelog

[General][Changed] - Cleanup RedBox message and stack output

Test Plan

Played around in RNTester

cpojer · 2019-04-30T12:07:50Z

React/Modules/RCTRedBox.m

 - (void)showErrorMessage:(NSString *)message withStack:(NSArray<RCTJSStackFrame *> *)stack isUpdate:(BOOL)isUpdate
 {
+    // Remove ANSI color codes from the message
+    NSString * messageWithoutAnsi = [self stripAnsi:message];


*messageWithoutAnsi

cpojer · 2019-04-30T12:21:37Z

Thank you so much for doing this, this is a great start and makes stack traces so much cleaner. My only concern is about debugging React/React Native changes because when errors do happen, having them stripped out of those files may give a wrong impression to people. Do you think we could add a button at the bottom of the trace, using the same styling as a stack trace to be unobtrusive, that will show hidden parts of the stack when tapped?

thymikee · 2019-04-30T12:34:34Z

Do you think we could add a button at the bottom of the trace, using the same styling as a stack trace to be unobtrusive, that will show hidden parts of the stack when tapped?

Definitely. Thought about it, but since it involves writing stateful UI logic on both platforms, I decided to go with this as a PoC for now. Happy to give it a spin next week (I'm on vacation starting today.)

cpojer · 2019-04-30T12:41:17Z

Enjoy your vacation! I think we can ship this for now as it is a good improvement and I'm gonna give others on the RN team a heads up in case they are doing a React sync soon so they are aware that a redbox is excluding some things.

cc @TheSavior @fkgozali @ejanzer

cpojer · 2019-04-30T12:43:09Z

Libraries/Core/ExceptionsManager.js

@@ -12,6 +12,8 @@

 import type {ExtendedError} from 'parseErrorStack';

+const INTERNAL_CALLSITES_REGEX = /ReactNativeRenderer-dev\.js|MessageQueue\.js/;


Actually should we make this an exact match with ^ and $?

For now I've added the $ but I'm going to compare how the paths look in normal app vs RNTester, because they differ.

Adjusted the paths: eb97ee3. They're now generated from an array, so it should be easier to add new ones (or generalize more, we'll see about that).

Libraries/Core/ExceptionsManager.js

facebook-github-bot

@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

julioxavierr · 2019-04-30T14:13:32Z

Awesome. Thanks

pvinis · 2019-04-30T14:16:13Z

Could we do one more thing, and put the text in the red part monospace? At least the part with the line numbers like 30, 31, etc in the first example.

react-native-bot · 2019-04-30T14:40:04Z

This pull request was successfully merged by @thymikee in 49d26eb.

^{When will my fix make it into a release? | Upcoming Releases}

thymikee · 2019-04-30T14:46:31Z

@pvinis to quote myself: "I thought about using monospace font for title with code frame, but it looks weird". I can play with that later though

pvinis · 2019-04-30T14:55:57Z

Oops, I forgot about this parenthesis while looking at the rest :p
If you tried and it's weird, ok, but it feels kinda silly to have all the > | ^ to show things that should be aligned while using a non-monospace font.

elicwhite · 2019-04-30T15:04:11Z

This will make things so much better, thanks!

As @cpojer mentioned, it will be pretty important to be able to see the hidden frames. Whenever we do react syncs, those frames are how we debit issues in the renderer. Hopefully you can do a quick follow up to add that.

pvinis · 2019-04-30T15:24:26Z

Just for comparison (only the red part):

before	after

I think the right one looks way easier to understand what's up. If it was horizontally scrollable it could be even better, but already it feels nicer.

What do you think?

cpojer · 2019-04-30T15:32:03Z

@pvinis I think it works for the code frame but not so well for the rest.

pvinis · 2019-04-30T15:48:43Z

true. I'll try with only the code frame. it was a quick try. maybe I make a PR for it and we see there. thanks :)

Summary: Cleanup RedBox messages and stack traces. This PR consists of 2 changes (I'm good with splitting them up if you'd like): - [general] filter out some of the internal callsites from the symbolicated stack (I thought about using monospace font for title with code frame, but it looks weird) - [ios][android] strip ANSI characters (coming from colored Babel code frame) from the error message I think it's ok to strip it inside native handlers so we can still have a colorful code frame in the terminal output. **JS Code frame:** |before|after| |--|--| |<img width="400" alt="Screenshot 2019-04-30 at 12 32 05" src="https://user-images.githubusercontent.com/5106466/56956590-ef678d80-6b44-11e9-9019-6801f050ab0d.png">|<img width="400" alt="Screenshot 2019-04-30 at 12 52 43" src="https://user-images.githubusercontent.com/5106466/56957302-f42d4100-6b46-11e9-869b-ea9c7ce5b90f.png">| |before|after| |--|--| |![image](https://user-images.githubusercontent.com/5106466/56959472-c8618980-6b4d-11e9-84be-6261d8375f4a.png)|![image](https://user-images.githubusercontent.com/5106466/56959463-bc75c780-6b4d-11e9-9d8b-25ffe46c87cf.png)| **Filtered stack traces:** |before|after| |--|--| |<img width="50%" alt="Screenshot 2019-04-30 at 12 27 21" src="https://user-images.githubusercontent.com/5106466/56956641-0908d500-6b45-11e9-8cdc-8c2a34a071e5.png"><img width="50%" alt="Screenshot 2019-04-30 at 12 27 28" src="https://user-images.githubusercontent.com/5106466/56956642-0908d500-6b45-11e9-921c-fabfb8515cc0.png">|<img width="100%" alt="Screenshot 2019-04-30 at 12 26 55" src="https://user-images.githubusercontent.com/5106466/56956650-0efeb600-6b45-11e9-9f5f-f10dd69580d1.png">| There's still a lot of places that are hard to read, but I think this is a good start towards more readable errors. cc cpojer [General][Changed] - Cleanup RedBox message and stack output Pull Request resolved: facebook/react-native#24662 Differential Revision: D15147571 Pulled By: cpojer fbshipit-source-id: 1de4e521af988fa7fc709b6accd0ddd984388e72

aleclarson · 2019-05-02T13:29:42Z

@pvinis If you could use a regular expression to detect the character range of the code frame (excluding the preceding error message), that would be great!

Summary: Changes `ExceptionsManager` to respect the `collapse` field in each symbolicated stack frame returned from Metro, in preference to the client-side regex. This is ultimately driven by a Metro config option (now also set in the RN new project template). Note that the client-side regex is intentionally ignored for any frame where `collapse` is present ( = all of them if using the latest Metro). Also updates the client-side regex to match the new renderer paths. This is part of a redesign of work done originally in facebook#24662; we will eventually remove the client-side regex from RN. Reviewed By: cpojer Differential Revision: D16500277 fbshipit-source-id: 85a94712a3686f8efcb04fca7814e99cf9726c10

Summary: Pull Request resolved: #435 Adds the `symbolicator.customizeFrame` config option to Metro. This is a function that receives a symbolicated stack frame and can modify the response before it's sent back to the client. The `collapse` field is the first supported customization, intended for auto-collapsing "internal" or infra-related stack frames. This is a redesign of work done originally in facebook/react-native#24662; we will eventually remove the client-side regex from RN. Reviewed By: cpojer Differential Revision: D16499755 fbshipit-source-id: eb748ac74ceee8a9069eac9df4f1be4c2564fdcd

…#25839) Summary: Pull Request resolved: facebook#25839 Changes `ExceptionsManager` to respect the `collapse` field in each symbolicated stack frame returned from Metro, in preference to the client-side regex. This is ultimately driven by a Metro config option (now also set in the RN new project template). Note that the client-side regex is intentionally ignored for any frame where `collapse` is present ( = all of them if using the latest Metro). Also updates the client-side regex to match the new renderer paths. This is part of a redesign of work done originally in facebook#24662; we will eventually remove the client-side regex from RN. Reviewed By: cpojer Differential Revision: D16500277 fbshipit-source-id: 557f93129d334b719200dc8603cc2345ffb45102

Summary: Changes `ExceptionsManager` to respect the `collapse` field in each symbolicated stack frame returned from Metro. This is ultimately driven by a Metro config option (which will have a default set in react-native-community/cli#596). This is part of a redesign of work done originally in facebook#24662. Reviewed By: cpojer Differential Revision: D16500277 fbshipit-source-id: bded9a7b037b9f944b27125d43e35fcce5bd109a

Summary: Pull Request resolved: #25839 Changes `ExceptionsManager` to respect the `collapse` field in each symbolicated stack frame returned from Metro. This is ultimately driven by a Metro config option (which will have a default set in react-native-community/cli#596). This is part of a redesign of work done originally in #24662. Reviewed By: cpojer Differential Revision: D16500277 fbshipit-source-id: b0b035618cb000935a555796523637b5f0a688d3

Summary: Cleanup RedBox messages and stack traces. This PR consists of 2 changes (I'm good with splitting them up if you'd like): - [general] filter out some of the internal callsites from the symbolicated stack (I thought about using monospace font for title with code frame, but it looks weird) - [ios][android] strip ANSI characters (coming from colored Babel code frame) from the error message I think it's ok to strip it inside native handlers so we can still have a colorful code frame in the terminal output. **JS Code frame:** |before|after| |--|--| |<img width="400" alt="Screenshot 2019-04-30 at 12 32 05" src="https://user-images.githubusercontent.com/5106466/56956590-ef678d80-6b44-11e9-9019-6801f050ab0d.png">|<img width="400" alt="Screenshot 2019-04-30 at 12 52 43" src="https://user-images.githubusercontent.com/5106466/56957302-f42d4100-6b46-11e9-869b-ea9c7ce5b90f.png">| |before|after| |--|--| |![image](https://user-images.githubusercontent.com/5106466/56959472-c8618980-6b4d-11e9-84be-6261d8375f4a.png)|![image](https://user-images.githubusercontent.com/5106466/56959463-bc75c780-6b4d-11e9-9d8b-25ffe46c87cf.png)| **Filtered stack traces:** |before|after| |--|--| |<img width="50%" alt="Screenshot 2019-04-30 at 12 27 21" src="https://user-images.githubusercontent.com/5106466/56956641-0908d500-6b45-11e9-8cdc-8c2a34a071e5.png"><img width="50%" alt="Screenshot 2019-04-30 at 12 27 28" src="https://user-images.githubusercontent.com/5106466/56956642-0908d500-6b45-11e9-921c-fabfb8515cc0.png">|<img width="100%" alt="Screenshot 2019-04-30 at 12 26 55" src="https://user-images.githubusercontent.com/5106466/56956650-0efeb600-6b45-11e9-9f5f-f10dd69580d1.png">| There's still a lot of places that are hard to read, but I think this is a good start towards more readable errors. cc cpojer [General][Changed] - Cleanup RedBox message and stack output Pull Request resolved: facebook/react-native#24662 Differential Revision: D15147571 Pulled By: cpojer fbshipit-source-id: 1de4e521af988fa7fc709b6accd0ddd984388e72

[general][changed] cleanup redbox message and stack output

cdf4a5c

thymikee requested a review from shergin as a code owner April 30, 2019 10:56

facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Callstack Partner: Callstack Partner labels Apr 30, 2019

react-native-bot added the Includes Changelog label Apr 30, 2019

thymikee added 2 commits April 30, 2019 13:40

[android][change] Remove ANSI color codes from the RedBox title

f95b2cf

remove unnecessary string allocation

fc05e61

cpojer reviewed Apr 30, 2019

View reviewed changes

fix spacing

bb83387

cpojer approved these changes Apr 30, 2019

View reviewed changes

cpojer reviewed Apr 30, 2019

View reviewed changes

Libraries/Core/ExceptionsManager.js Outdated Show resolved Hide resolved

cpojer reviewed Apr 30, 2019

View reviewed changes

Libraries/Core/ExceptionsManager.js Outdated Show resolved Hide resolved

thymikee added 2 commits April 30, 2019 14:45

cleanup

30fd2cb

tighten regex

eb97ee3

thymikee force-pushed the feat/cleaner-redbox branch from 663f8c6 to eb97ee3 Compare April 30, 2019 13:08

facebook-github-bot approved these changes Apr 30, 2019

View reviewed changes

facebook-github-bot closed this in 49d26eb Apr 30, 2019

react-native-bot added the Merged This PR has been merged. label Apr 30, 2019

thymikee deleted the feat/cleaner-redbox branch April 30, 2019 14:45

This was referenced Jul 26, 2019

Add customizeFrame symbolicator config option facebook/metro#435

Closed

Use Metro support for auto-collapsing internal stack frames #25839

Closed

motiz88 mentioned this pull request Jul 29, 2019

feat: set a default symbolicator.customizeFrame value react-native-community/cli#596

Merged

acoates-ms mentioned this pull request Mar 9, 2020

Strip out color code characters from redbox error message microsoft/react-native-windows#4278

Merged

		@@ -12,6 +12,8 @@

		import type {ExtendedError} from 'parseErrorStack';

		const INTERNAL_CALLSITES_REGEX = /ReactNativeRenderer-dev\.js\|MessageQueue\.js/;

[general][changed] cleanup RedBox message and stack output #24662

[general][changed] cleanup RedBox message and stack output #24662

Uh oh!

Conversation

thymikee commented Apr 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changelog

Test Plan

Uh oh!

cpojer Apr 30, 2019

Choose a reason for hiding this comment

Uh oh!

cpojer commented Apr 30, 2019

Uh oh!

thymikee commented Apr 30, 2019

Uh oh!

cpojer commented Apr 30, 2019

Uh oh!

cpojer Apr 30, 2019

Choose a reason for hiding this comment

Uh oh!

thymikee Apr 30, 2019

Choose a reason for hiding this comment

Uh oh!

thymikee Apr 30, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

julioxavierr commented Apr 30, 2019

Uh oh!

pvinis commented Apr 30, 2019

Uh oh!

react-native-bot commented Apr 30, 2019

Uh oh!

thymikee commented Apr 30, 2019

Uh oh!

pvinis commented Apr 30, 2019

Uh oh!

elicwhite commented Apr 30, 2019

Uh oh!

pvinis commented Apr 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cpojer commented Apr 30, 2019

Uh oh!

pvinis commented Apr 30, 2019

Uh oh!

aleclarson commented May 2, 2019

Uh oh!

Uh oh!

thymikee commented Apr 30, 2019 •

edited

Loading

pvinis commented Apr 30, 2019 •

edited

Loading