-
Notifications
You must be signed in to change notification settings - Fork 47.5k
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
Badge Environment Name on Thrown Errors from the Server #29846
Changes from 9 commits
afe92ea
153e78b
9ecc408
12ccf6f
ff2b77f
11c4047
016d86b
fcc2fa6
763ae0a
d31e852
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,10 @@ | |
module.exports = function shouldIgnoreConsoleError(format, args) { | ||
if (__DEV__) { | ||
if (typeof format === 'string') { | ||
if (format.startsWith('%c%s')) { | ||
// Looks like a badged error message | ||
args.splice(0, 3); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is ensuring we're filtering the same as before. No change in what gets filtered. |
||
if ( | ||
args[0] != null && | ||
((typeof args[0] === 'object' && | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,6 +20,8 @@ import ReactSharedInternals from 'shared/ReactSharedInternals'; | |
|
||
import {enableOwnerStacks} from 'shared/ReactFeatureFlags'; | ||
|
||
import {printToConsole} from './ReactFiberConfig'; | ||
|
||
// Side-channel since I'm not sure we want to make this part of the public API | ||
let componentName: null | string = null; | ||
let errorBoundaryName: null | string = null; | ||
|
@@ -94,13 +96,33 @@ export function defaultOnCaughtError( | |
}.`; | ||
|
||
if (enableOwnerStacks) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I only added this feature to the |
||
console.error( | ||
'%o\n\n%s\n\n%s\n', | ||
error, | ||
componentNameMessage, | ||
recreateMessage, | ||
// We let our consoleWithStackDev wrapper add the component stack to the end. | ||
); | ||
if ( | ||
typeof error === 'object' && | ||
error !== null && | ||
typeof error.environmentName === 'string' | ||
) { | ||
// This was a Server error. We print the environment name in a badge just like we do with | ||
// replays of console logs to indicate that the source of this throw as actually the Server. | ||
printToConsole( | ||
'error', | ||
[ | ||
'%o\n\n%s\n\n%s\n', | ||
error, | ||
componentNameMessage, | ||
recreateMessage, | ||
// We let our consoleWithStackDev wrapper add the component stack to the end. | ||
], | ||
error.environmentName, | ||
); | ||
} else { | ||
console.error( | ||
'%o\n\n%s\n\n%s\n', | ||
error, | ||
componentNameMessage, | ||
recreateMessage, | ||
// We let our consoleWithStackDev wrapper add the component stack to the end. | ||
); | ||
} | ||
} else { | ||
// The current Fiber is disconnected at this point which means that console printing | ||
// cannot add a component stack since it terminates at the deletion node. This is not | ||
|
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.
Can this mirror the check in
shouldIgnoreConsoleError
i.e.or are these files unrelated?
To be honest, I don't quite follow why we need to ignore this case and why there's no good default for our test suite. Could you add test to
packages/internal-test-utils/__tests__/ReactInternalTestUtils-test.js
to illustrate what this is doing?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.
In
shouldIgnoreConsoleError
the more specific one was ok because we didn't test any paths that used the server printer:https://github.com/sebmarkbage/react/blob/ba2389424a1deb617e62d8473e4c2e718a2ed984/packages/react-client/src/ReactClientConsoleConfigServer.js#L13
But tbh, none of this makes any sense. We should find a way to remove
shouldIgnoreConsoleError
. There must be some systemic issue in our tests that we can address so we don't need this anymore. I keep just hacking in whatever I need to keep it running.Regarding the argument mismatch I feel like we should just remove this assertion. We've found a case where this is valid. So if we keep it we need some way to disable it. Isn't the lint warning enough for the common mistakes and we can disable the lint warning as needed?
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.
I'm all for that. I just don't know which test hits this path so I can't help.
It should be. Maybe the additional runtime check was for calls that aren't checked by the lint rule. I can double check.
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.
We can use the usual
assertConsoleErrorDev
for now but probably want a specificassertConsoleBadgedErrorDev
in the future to encode the behavior clearer: sebmarkbage#6