-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
feat: Implement better reanimated logger with clean stack traces #6385
Conversation
916756d
to
def4d34
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks all good, let's figure out the types and also let's make sure we have defined behavior when copying errors from React Runtime to Workler Runtime, i.e.:
// React Runtime
const error = new ReanimatedError();
runOnUI(() => {
console.log(error); // Should fail, let's make it verbose.
};
)();
522d301
to
9cb4433
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! I feel bad about being so demanding in the review process 😅. We're very close to merge it though!
Co-authored-by: Tomasz Żelawski <40713406+tjzel@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finally! I added only some minor comments but you can ignore them if you feel so. Let's merge this!
Just a reminder, please make sure to make a follow-up PR which adds information about this feature in our Docs.
Co-authored-by: Tomasz Żelawski <40713406+tjzel@users.noreply.github.com>
Co-authored-by: Tomasz Żelawski <40713406+tjzel@users.noreply.github.com>
Co-authored-by: Tomasz Żelawski <40713406+tjzel@users.noreply.github.com>
Co-authored-by: Tomasz Żelawski <40713406+tjzel@users.noreply.github.com>
## Summary This PR is a much cleaner approach than proposed in #6364. It includes metro-config modification which is essential to collapse logs from reanimated source code, which aren't helpful to the user while tracking down the issue. The previous approach was trimming logs from reanimated source code completely - this approach just collapses them, so that they are still available to the user and can be revealed above the presented stack trace part. ## General idea To get better logs, I had to implement the following 2 changes: 1. **metro config** - the `symbolicator` must have been added to properly collapse stack frames that aren't meaningful to the user, 2. **logger object** - the new logger object uses `LogBox.addLog` method, thanks to which we can get pretty stack traces when we log a warning from the UI thread (before such warnings didn't include meaningful stack trace as error stack was created inside `LogBox` after `runOnJS` was called, so we were getting a bit limited JS stack - see [example 11](#6387 (comment)) in the follow up PR). ## Example improvement (tested on a real project to see if it works there as well) - current logs either point to the reanimated source code or aren't readable at all (if warning is logged from the UI thread as in the example below) - new logger shows correct destination of the issue culprit in the code frame, collapses stack frames in the call stack that aren't interesting to the user (reanimated source code) and focuses on the file where the user included problematic code | Before | After | |-|-| | <video src="https://github.com/user-attachments/assets/a5302586-f4d0-4636-8bd8-6c406c9d8c73" /> | <video src="https://github.com/user-attachments/assets/3121636f-69a2-4b6f-8f38-b1889d4c62e1" /> | ## Test plan See the example in the next PR (#6387). --------- Co-authored-by: Tomasz Żelawski <40713406+tjzel@users.noreply.github.com>
This PR is a much cleaner approach than proposed in #6364. It includes metro-config modification which is essential to collapse logs from reanimated source code, which aren't helpful to the user while tracking down the issue. The previous approach was trimming logs from reanimated source code completely - this approach just collapses them, so that they are still available to the user and can be revealed above the presented stack trace part. To get better logs, I had to implement the following 2 changes: 1. **metro config** - the `symbolicator` must have been added to properly collapse stack frames that aren't meaningful to the user, 2. **logger object** - the new logger object uses `LogBox.addLog` method, thanks to which we can get pretty stack traces when we log a warning from the UI thread (before such warnings didn't include meaningful stack trace as error stack was created inside `LogBox` after `runOnJS` was called, so we were getting a bit limited JS stack - see [example 11](#6387 (comment)) in the follow up PR). (tested on a real project to see if it works there as well) - current logs either point to the reanimated source code or aren't readable at all (if warning is logged from the UI thread as in the example below) - new logger shows correct destination of the issue culprit in the code frame, collapses stack frames in the call stack that aren't interesting to the user (reanimated source code) and focuses on the file where the user included problematic code | Before | After | |-|-| | <video src="https://github.com/user-attachments/assets/a5302586-f4d0-4636-8bd8-6c406c9d8c73" /> | <video src="https://github.com/user-attachments/assets/3121636f-69a2-4b6f-8f38-b1889d4c62e1" /> | See the example in the next PR (#6387). --------- Co-authored-by: Tomasz Żelawski <40713406+tjzel@users.noreply.github.com>
Summary
This PR is a much cleaner approach than proposed in #6364. It includes metro-config modification which is essential to collapse logs from reanimated source code, which aren't helpful to the user while tracking down the issue. The previous approach was trimming logs from reanimated source code completely - this approach just collapses them, so that they are still available to the user and can be revealed above the presented stack trace part.
General idea
To get better logs, I had to implement the following 2 changes:
symbolicator
must have been added to properly collapse stack frames that aren't meaningful to the user,LogBox.addLog
method, thanks to which we can get pretty stack traces when we log a warning from the UI thread (before such warnings didn't include meaningful stack trace as error stack was created insideLogBox
afterrunOnJS
was called, so we were getting a bit limited JS stack - see example 11 in the follow up PR).Example improvement
(tested on a real project to see if it works there as well)
Screen.Recording.2024-08-07.at.15.36.48.mp4
Screen.Recording.2024-08-07.at.15.40.33.mp4
Test plan
See the example in the next PR (#6387).