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 junk in React warnings in Logbox #44812

Closed
wants to merge 3 commits into from
Closed

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Jun 6, 2024

Before all React errors showed junk like this:

Screenshot 2024-06-06 at 06 24 38

This is because isComponentStack detected a component stack but parseComponentStack couldn't actually parse it (it doesn't deal with React's current format like in Foo (created by FeedItemInner)) so componentStack was an empty array, resulting in the next block of code pushing stuff into argsWithoutComponentStack again, thus repeating its args.

The fix is not to do that. Result on my local copy:

Screenshot 2024-06-06 at 06 24 24

Ofc this doesn't actually show the component stack but that was broken before too.

I edited in-place in my node_modules so I haven't verified this 100% works on main.

Hope this is useful!

Changelog:

[General] [Fixed] - Remove accidental duplication in React warnings in Logbox

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jun 6, 2024
Copy link

github-actions bot commented Jun 6, 2024

Warnings
⚠️ 📋 Missing Summary - Can you add a Summary? To do so, add a "## Summary" section to your PR description. This is a good place to explain the motivation for making this change.
⚠️ 📋 Missing Test Plan - Can you add a Test Plan? To do so, add a "## Test Plan" section to your PR description. A Test Plan lets us know how these changes were tested.

Generated by 🚫 dangerJS against 8e7f4e6

@gaearon
Copy link
Collaborator Author

gaearon commented Jun 6, 2024

Actually maybe this already got fixed on main due to #43166 because now source-less stacks get properly parsed? Still, I think this fix is worth getting in because pushing in the same array in two rounds that can potentially overlap doesn't make sense. This would prevent it breaking again next time the format changes.

@motiz88
Copy link
Contributor

motiz88 commented Jun 6, 2024

Thanks @gaearon! Is this something we can cover with a unit test?

@rickhanlonii
Copy link
Member

@gaearon thanks, I added a test but you're right that this was already fixed on main. I requested a pick to 0.74 but I don't think it made it in.

@facebook-github-bot
Copy link
Contributor

@rickhanlonii has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@rickhanlonii has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@rickhanlonii merged this pull request in 32c3cd3.

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Jun 11, 2024
Copy link

This pull request was successfully merged by @gaearon in 32c3cd3.

When will my fix make it into a release? | How to file a pick request?

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants