-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
[dev-launcher][ios] Show blue screen only if needed #13421
Conversation
ENG-1420 blue screen rather than red screen shown for some JS errors
repros in 0.3.0 and 0.4.1 example of failing code: https://snack.expo.io/@thetc/dev-client-blue-screen |
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.
The review previously left here is no longer valid, jump to the latest one 👉 #13421 (review)
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.
tiny nit, but it might read more clearly in this case to do something like
if (isUpdate || errorCookie != -1) {
// These errors should be handled by LogBox
return;
}
[self.logBox hide];
....
since that's sort of the "odd" case that we're bailing out early from. But not a big deal if you feel strongly otherwise; lgtm 👍
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.
Hi there! 👋 I'm a bot whose goal is to ensure your contributions meet our guidelines.
I've found some issues in your pull request that should be addressed (click on them to expand) 👇
⚠️ Suggestions: Missing changelog entries
Your changes should be noted in the changelog. Read Updating Changelogs guide and consider (it's optional) adding an appropriate entry to the following changelogs:
Generated by ExpoBot 🤖 against 374fd30
# Why Fixes ENG-1420 # How For some reason, RN calls the `showErrorMessage` on RedBox even if the error is handled by the LogBox. I've added a check to not show the blue screen if that is the case. # Test Plan https://linear.app/expo/issue/ENG-1420/blue-screen-rather-than-red-screen-shown-for-some-js-errors
Why
Fixes ENG-1420
How
For some reason, RN calls the
showErrorMessage
on RedBox even if the error is handled by the LogBox. I've added a check to not show the blue screen if that is the case.Test Plan
https://linear.app/expo/issue/ENG-1420/blue-screen-rather-than-red-screen-shown-for-some-js-errors