Remove Warning: prefix and toString on console Arguments

[sebmarkbage]

Basically make console.error and console.warn behave like normal - when a component stack isn't appended. I need this because I need to be able to print rich logs with the component stack option and to be able to disable instrumentation completely in console.createTask environments that don't need it.

Currently we can't print logs with richer objects because they're toString:ed first. In practice, pretty much all arguments we log are already toString:ed so it's not necessary anyway. Some might be like a number. So it would only be a problem if some environment can't handle proper consoles but then it's up to that environment to toString it before logging.

The Warning: prefix is historic and is both noisy and confusing. It's mostly unnecessary since the UI surrounding console.error and console.warn tend to have visual treatment around it anyway. However, it's actively misleading when console.error gets prefixed with a Warning that we consider an error level. There's an argument to be made that some of our console.error don't make the bar for an error but then the argument is to downgrade each of those to console.warn - not to brand all our actual error logging with Warning: .

Apparently something needs to change in React Native before landing this because it depends on the prefix somehow which probably doesn't make sense already.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
react-compiler-playground	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jun 10, 2024 10:05pm

[react-sizebot]

The size diff is too large to display in a single comment. The CircleCI job contains an artifact called 'sizebot-message.md' with the full message.

Generated by 🚫 dangerJS against 1c844ea

[yungsters]

The historical reason for this was that some DevTools wouldn’t display the stack trace for console.warn.

React Native currently checks for the prefix and displays “Warning: “-prefixed errors as warnings. However, if we actually do believe that all current “Warning: “-prefixed errors should behave in DevTools like errors (and not be downgraded to warnings), then I think this is actually fine. It means that we will be “upgrading” the severity of all of these “Warning: “-prefixed errors in React Native (e.g. LogBox will automatically display maximized).

[sebmarkbage]

I'm guessing this is the issue in RN:

https://github.com/facebook/react-native/blob/7ce5e56f388a10787b74320a1d16271a90221fff/packages/polyfills/console.js#L425-L428

This doesn't make sense because user space console.error wouldn't have the prefix, and not even all React warnings go through this. Maybe all console.error shouldn't pop up.

However, it's also not correct that console.error shouldn't show log box. We treat console.error the same as throw and if you ignore console.error and especially if you instead show the thrown error you're like to get a bad error message. That's by far the biggest reason people complain about React error messages - that they see the thrown message but not the console.error.

More over, since the default for onCaughtError is console.error rather than reportError it means that any error boundary's errors are silenced.

React treats console.error the same as throw.

If there are some very common ones in the RN ecosystem or an app, then maybe those exact messages can be silenced or even downgraded upstream but it doesn't make sense that every new error that we rely on in React to provide a good DX is silenced.

[rickhanlonii]

The RN stuff is here: https://github.com/facebook/react-native/blob/main/packages/react-native/Libraries/LogBox/LogBox.js#L191-L224

I think the reason that redirect is needed was because we were adding the component stack in different places depending on whether it was a Warning: error or a user space console.error. So potentially we could clean that up with this PR, but we should have a branch that fixes that before merging this (cc @gaearon who has been very frustrated with RN errors breaking and a lag time of fixing them in RN).

[rickhanlonii]

But the bigger issue I'm not sure about is the WarningFilter and error reporting stuff, which I think depends on the Warning: prefix. I wonder if a root option can be a solution there, like onReactWarning. Don't love it.

[gaearon]

So potentially we could clean that up with this PR, but we should have a branch that fixes that before merging this (cc @gaearon who has been very frustrated with RN errors breaking and a lag time of fixing them in RN).

FWIW part of the reason it's broken in RN is because the system is so complicated — I'm very +1 on this change. I also don't think the onus for fixing it up should be on the React side — IMO whoever does the next sync to RN should generally be able to clean this up.

[sebmarkbage]

I don't know how the WarningFilter stuff works on RN if it exists but this doesn't affect www which has this fork which doesn't add the prefix nor toString.

https://github.com/facebook/react/blob/main/packages/shared/forks/consoleWithStackDev.www.js#L48-L50

(It does assume that it has a format as first argument which is already not true so something is probably already broken.)

[sebmarkbage]

I think the reason that redirect is needed was because we were adding the component stack in different places depending on whether it was a Warning: error or a user space console.error.

I'm not sure what this means but it does seem like something similar to what React DevTools has to do. It adds a component stack if one doesn't already exist in the message. That might be what we want to copy here. (I don't love that neither because it gets disabled with console.createTask but at least it brings it in line and we can fix it once console.createTask is available in RN.)

[sebmarkbage]

This is the code in DevTools that checks if there's a component stack before adding another one.

react/packages/react-devtools-shared/src/backend/console.js

Lines 206 to 207 in d172bda

	const alreadyHasComponentStack =
	typeof lastArg === 'string' && isStringComponentStack(lastArg);

Would a check like that be sufficient?

[rickhanlonii]

Yeah, I think LogBox could be updated to check for the presence of component stacks instead of the Warning: prefix. But if it's ever possible that a react warning could be logged without a component stack, that warning would never be shown in logbox. Really the thing to fix is the fall through that is needed, so the logbox patching is always after react and devtools patching.

And are we sure it is always the last arg? To @gaearon's point, the reason this is so fragile in RN is yes the complexity, but also the implicit assumptions being mad (for example, this is assuming the the component stack is always the last arg, but is that always the case?)

[sebmarkbage]

It doesn't have to always be last if something else patches between React and DevTools so it assumes that like nothing adds another suffix.

[rickhanlonii]

I also don't think the onus for fixing it up should be on the React side

fwiw I'm not really concerned with who actually has to do the work, but I think having one person doing it on both sides gives you the full context of the system so you can reduce the complexity all the way down. and if you assume it works one way and someone without much context on the change finds out it works another way, there could be a lot more work involved than expected being done by the person with the least amount of context on the change

[rickhanlonii]

@sebmarkbage yeah, that's why in the non-Warning case, logbox searches all the args for a component stack. We may actually be able to just remove this check from LogBox and let them always fall through to console.error, I think that warning check was inherited from the YellowBox days, but someone would need to check to verify it doesn't break react warnings

[rickhanlonii]

The place where we patch console.error with some docs on the indirection that can happen is here: https://github.com/facebook/react-native/blob/main/packages/react-native/Libraries/Core/ExceptionsManager.js#L165

[sebmarkbage]

there could be a lot more work involved than expected being done by the person with the least amount of context on the change

There is a barrier here that we expect console.error to behave like the Web standard and the browser DevTools. That's what creates a contract that minimizes the scope.

Work on React can't be expected to know how every possible downstream works including every meta-framework. In fact, while we test like Firefox and Safari we don't really even care to make those work when they diverge from Chrome in weird ways. E.g. you don't get stack traces for uncaught errors in Safari - the fix is for Safari to make reportError behave like Chrome.

So the assumption from the RN side must be that any sync of React can have a new callsite to console.error that behaves like it would in Chrome. It doesn't need any more context than that.

[rickhanlonii]

Yeah I mean there's no standard for patching?

What would be great is a standard way to capture errors/warnings after React has finished it's patching, maybe with root options like onCapturedError and onUncapturedError` with go to the console methods be default:

• onConsoleError(...args, {componentStack})
• onConsoleWarning(...args, {componentStack})

The other thing I was talking with @acdlite about was that we really need different levels for warnings that react emits. Some of them are informative, many or most of them are SEV preventative. Maybe React warnings would have another option or a errorInfo level.

[rickhanlonii]

It would be nice to move the settings like showDialog from the warningfilter into React too

[rickhanlonii]

LGTM, I'll fix the RN sync when this lands

[sebmarkbage]

Note that React doesn't patch the console. (At least not on the client - FlightServer does.) React DevTools does and the timing for that has to do with the Chrome Extensions plugin which ensures that it is last. Meaning it should be unobservable to the page whether it was or not. This does mean that a web page can't show the component stacks added by React DevTools. We need some way to implement custom dialogs over console.error that includes the stack for those. With createTask they go away even for warnings issued from React itself.

What would be great is a standard way to capture errors/warnings after React has finished it's patching, maybe with root options like onCapturedError and onUncapturedError` with go to the console methods be default: onConsoleError(...args, {componentStack}) onConsoleWarning(...args, {componentStack})

Since we don't currently patch this wouldn't really capture anything. I considered this as one of the options. It's tricky though because we'd need to start patching and then redirect them to this callback which would then start silencing any warnings but only if they're inside a React scope which can be tricky because that's a lot of scopes.

Currently I'm leaning towards root.getCurrentOwnerStack() to get the currently executing owner stack for the root. That can then be used by Error Dialogs - including Log Box to show component stacks on console.error. I think most of this is a symptom of not having an API like that.

we really need different levels for warnings that react emits.

The standard already has so many levels. log, info, warn, error (+ more for like profiling and other special logs). After some point there's only so much more value you can gain from more levels. Maybe our usage of the current levels are inflated though and we should downgrade some to info and some to warn.

[sebmarkbage]

LGTM, I'll fix the RN sync when this lands

❤️ u