fix logbox error stack #47303
Merged
fix logbox error stack #47303
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Summary: Pull Request resolved: #46639 These tests were skipped when we were switching to component stacks, which also hid a bug later in the stack. Re-enable them. Changelog: [Internal] Reviewed By: javache Differential Revision: D63349616 fbshipit-source-id: ccde7d5bb3fcd9a27adf4af2068a160f02f7432a
Summary: Pull Request resolved: #46636 Adds more integration tests for LogBox (currently incorrect, but fixed in a later diff). Changelog: [Internal] Reviewed By: javache Differential Revision: D63349614 fbshipit-source-id: 8f5c6545b48a1ed18aea08d4ecbecd7a6b9fa05a
Summary: Pull Request resolved: #46638 This is annoying, but in the next diff that fixes a bug I need to test using the default warning filter instead of a mock (really, all this mocking is terrible, idk why I did it this way). Unfortunately, in Jest you can't just reset mocks from `jest.mock`, `restoreMocks` only resets spies and not mocks (wild right). So in this diff I converted all the `jest.mock` calls to `jest.spyOn`. I also corrected some of the mocks that require `monitorEvent: 'warning',` like the warning filter sets. I also added a test that works without the fix. Changelog: [Internal] Reviewed By: javache Differential Revision: D63349615 fbshipit-source-id: 4f2a5a8800c8fe1a10e3613d3c2d0ed02fca773e
Summary: Ok so this is a doozy. ## Overview There was a report that some console.error calls were being shown as warnings in LogBox but as console.error in the console. The only time we should downlevel an error to a warning is if the custom warning filter says so (which is used for some noisy legacy warning filter warnings internally). However, in when I switched from using the `Warning: ` prefix, to using the presence of component stacks, I subtly missed the default warning filter case. In the internal warning filter, the `monitorEvent` is always set to something other than `unknown` and if it's set to `warning_unhandled` then `suppressDialog_LEGACY` is always false. However, the default values for the warning filter are that `monitorEvent = 'unknown'` and `suppressDialog_LEGACY = true`. In this case, we would downlevel the error to a warning. ## What's the fix? Change the default settings for the warning filter. ## What's the root cause? Bad configuration combinations in a fragile system that needs cleaned up, and really really bad testing practices with excessive mocking and snapshot testing (I can say that, I wrote the tests) ## How could it have been caught? It was, but I turned off the integration tests while landing the component stack changes because of mismatches between flags internally and in OSS, and never turned them back on. Changelog: [General] [Fixed] - Fix logbox reporting React errors as Warnings Pull Request resolved: #46637 Reviewed By: huntie Differential Revision: D63349613 Pulled By: rickhanlonii fbshipit-source-id: 32e3fa4e2f2077114a6e9f4feac73673973ab50c
- Older version has old URL, updated tests - Comments on test don't match what's being tested. Updated.
facebook-github-bot
added
the
CLA Signed
|
blakef
commented
Oct 30, 2024
| ); | ||
| expect(console.error).toBeCalledTimes(1); | ||
| expect(console.error.mock.calls[0]).toEqual([ | ||
| 'Warning: Each child in a list should have a unique "key" prop.%s%s See https://reactjs.org/link/warning-keys for more information.%s', |
There was a problem hiding this comment.
The fix was to apply what the above comments asked for: "Warning" prefix should be there. I also had to revert the react.dev → reactjs.org url because of the version of DevTools that ships with 0.76.
huntie
approved these changes
Oct 30, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Commit to add Logbox changes to 0.76.2:
Changelog: [General][Added] Logbox warning fixes