Badge Environment Name on Thrown Errors from the Server #29846
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
facebook-github-bot
added
CLA Signed
React Core Team
| try { | ||
| if (error instanceof Error) { | ||
| // eslint-disable-next-line react-internal/safe-string-coercion | ||
| message = String(error.message); | ||
| stack = getStack(error); | ||
| const errorEnv = (error: any).environmentName; | ||
| if (typeof errorEnv === 'string') { | ||
| // This probably came from another FlightClient as a pass through. |
There was a problem hiding this comment.
| // This probably came from another FlightClient as a pass through. | |
| // This probably came from another FlightServer as a pass through. |
There was a problem hiding this comment.
Well, technically what I mean is that the FlightClient is what creates the object on the parsing side. In theory it wasn't necessarily even from a FlightServer that generated the payload.
Conceptually it's a bit unclear who hides the original thrown/rejected value (which might not be an Error) and who creates the new Error - FlightServer or FlightClient? I guess FlightServer. We don't technically serialize Error objects - we serialize the throw.
There was a problem hiding this comment.
Well, technically what I mean is that the FlightClient is what creates the object on the parsing side.
Do you mean this line? How would this end up back in a Flight Server?
There was a problem hiding this comment.
In the server-to-server case where an RSC server consumes the payload from another RSC server (not the binary pass-though case). I.e. the case we have in our tests.
There was a problem hiding this comment.
Oh, of course, I forgot that there must be a client in-between in the server-to-server case. 🤦
| args, | ||
| expectedArgCount: argIndex, | ||
| }); | ||
| if (format.includes('%c%s')) { |
There was a problem hiding this comment.
Can this mirror the check in shouldIgnoreConsoleError i.e.
| if (format.includes('%c%s')) { | |
| if (format.startsWith('%c%s%c')) { |
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.
In shouldIgnoreConsoleError the more specific one was ok because we didn't test any paths that used the server printer:
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.
But tbh, none of this makes any sense. We should find a way to remove shouldIgnoreConsoleError
I'm all for that. I just don't know which test hits this path so I can't help.
Isn't the lint warning enough for the common mistakes and we can disable the lint warning as needed?
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.
We can use the usual assertConsoleErrorDev for now but probably want a specific assertConsoleBadgedErrorDev in the future to encode the behavior clearer: sebmarkbage#6
| @@ -94,13 +96,33 @@ export function defaultOnCaughtError( | |||
| }.`; | |||
|
|
|||
| if (enableOwnerStacks) { | |||
There was a problem hiding this comment.
I only added this feature to the enableOwnerStacks flag because we the other branch uses the uninstrumented console['error'](...) and we don't have a version of printToConsole that excludes adding a component stack. So to avoid forking all that behavior for the old branch I just made this a "new world" feature.
| if (format.startsWith('%c%s')) { | ||
| // Looks like a badged error message | ||
| args.splice(0, 3); | ||
| } |
There was a problem hiding this comment.
This is ensuring we're filtering the same as before. No change in what gets filtered.
This is attached to our custom Error "sub class" on the receiving side which also includes digest. DEV-only.
This way this helper can use component stacks when available.
That way your typical log of an error that originated on the Server gets a `[Server]` badge. Unfortunately because defaultOnUncaughtError and defaultOnRecoverableError doesn't use custom logging but goes through reportError. There's no place for us to add this kind of formatted badging to the logs. It's also unfortunate that you'd have to replicate this in user space.
|
Felt cute. Might delete later. This is inline with the current strategy already in main but another strategy might be to rely on Meta Frameworks to add this badge. |
When we replay logs we badge them with e.g. `[Server]`. That way it's easy to identify that the source of the log actually happened on the Server (RSC). However, when we threw an error we didn't have any such thing. The error was rethrown on the client and then handled just like any other client error. This transfers the `environmentName` in DEV to our restored Error "sub-class" (conceptually) along with `digest`. That way you can read `error.environmentName` to print this in your own UI. I also updated our default for `onCaughtError` (and `onError` in Fizz) to use the `printToConsole` helper that the Flight Client uses to log it with the badge format. So by default you get the same experience as console.error for caught errors: <img width="810" alt="Screenshot 2024-06-10 at 9 25 12 PM" src="https://github.com/facebook/react/assets/63648/8490fedc-09f6-4286-9332-fbe6b0faa2d3"> <img width="815" alt="Screenshot 2024-06-10 at 9 39 30 PM" src="https://github.com/facebook/react/assets/63648/bdcfc554-504a-4b1d-82bf-b717e74975ac"> Unfortunately I can't do the same thing for `onUncaughtError` nor `onRecoverableError` because they use `reportError` which doesn't have custom formatting (unless we also prevented default on window.onerror). However maybe that's ok because 1) you should always have an error boundary 2) it's not likely that an RSC error can actually recover because it's not going to be rendered again so shouldn't really happen outside some parent conditionally rendering maybe. The other problem with this approach is that the default is no longer trivial - so reimplementing the default in user space is trickier and ideally we shouldn't expose our default to be called. DiffTrain build for [349a99a](349a99a)
When we replay logs we badge them with e.g.
[Server]. That way it's easy to identify that the source of the log actually happened on the Server (RSC). However, when we threw an error we didn't have any such thing. The error was rethrown on the client and then handled just like any other client error.This transfers the
environmentNamein DEV to our restored Error "sub-class" (conceptually) along withdigest. That way you can readerror.environmentNameto print this in your own UI.I also updated our default for
onCaughtError(andonErrorin Fizz) to use theprintToConsolehelper that the Flight Client uses to log it with the badge format. So by default you get the same experience as console.error for caught errors:Unfortunately I can't do the same thing for
onUncaughtErrornoronRecoverableErrorbecause they usereportErrorwhich doesn't have custom formatting (unless we also prevented default on window.onerror). However maybe that's ok because 1) you should always have an error boundary 2) it's not likely that an RSC error can actually recover because it's not going to be rendered again so shouldn't really happen outside some parent conditionally rendering maybe.The other problem with this approach is that the default is no longer trivial - so reimplementing the default in user space is trickier and ideally we shouldn't expose our default to be called.