DevTools does not eagerly patch console for RN

[bvaughn]

Previously, the DevTools backend eagerly patched the console (to append "component stacks" to warnings and errors) even if no DevTools frontend was running. I did this because I thought the components stacks would be very helpful, and a lot of RN developers don't even run the DevTools UI to otherwise get them. (Also there would be no other way to get them for errors logged during mount.)

Unfortunately this had the observable effect of changing the source code line shown for the errors/warnings in debugger tooling (where the actual call to the native console.error is made) from YellowBox.js (the RN override) to the DevTools backend.js script. Some people complained, so this PR removes that default behavior¹.

This should not affect the browser/DOM extension. I have tested to confirm this in the browser. I tested the change in RN by building the react-native-core backend and patching my local version of React Native.

Resolves facebook/react-native#26788

¹ I'm not sure I understand why the previous behavior (of showing the YellowBox.js override location) was preferable to showing the DevTools backend. Neither is the location of the original caller. I've asked for clarification on the RN issue. If we decide to remove the DevTools override though, this is the way to do it.

[sizebot]

No significant bundle size changes to report.

Generated by 🚫 dangerJS against 56b2771

[gaearon]

I'm not sure I understand why the previous behavior (of showing the YellowBox.js override location) was preferable to showing the DevTools backend

Huh, that's not at all what I thought the behavior was! What do you mean by this?

I was looking at this screenshot:

It has real filenames for all logs except the warn one, not the YellowBox override location. Maybe you were looking at different lines?

The thing I'd like to see fixed is the regular console.logs losing their filename. Not the warns.

[gaearon]

To clarify, the relevant part is the red rectangle on the blurred screenshot.

[bvaughn]

Ahh, I see. I misunderstood this to be about the component stack logs, but it seems this is actually about the fact that DevTools patches console.log too so it can disable logging when re-rendering to inspect hooks.

Hmm...maybe we should separate those.

[gaearon]

Ohh I didn’t realize that’s what it is. Yeah. I think it’s OK to keep hijacking warn/error but probably not log by default.

[gaearon]

Silly question. Can’t we only override console log when we re evaluate Hooks? And restore it right after.

[bvaughn]

Yeah, maybe that override could be deferred until the first time a component was actually inspected (which would never happen unless someone was actually using the DevTools frontend).

So then the open question would be if we wanted to still keep eagerly overriding console.warn, console.error for component stacks (given that YellowBox already overrides those methods)

[bvaughn]

Duh. I'm having trouble remembering my own code. We already do delay patching console.log until we're re-rendering:

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

Lines 2291 to 2313 in 4cb399a

	// Temporarily disable all console logging before re-running the hook.
	for (let method in console) {
	try {
	originalConsoleMethods[method] = console[method];
	// $FlowFixMe property error|warn is not writable.
	console[method] = () => {};
	} catch (error) {}
	}
	try {
	hooks = inspectHooksOfFiber(
	fiber,
	(renderer.currentDispatcherRef: any),
	);
	} finally {
	// Restore original console functionality.
	for (let method in originalConsoleMethods) {
	try {
	// $FlowFixMe property error|warn is not writable.
	console[method] = originalConsoleMethods[method];
	} catch (error) {}
	}
	}

I am confused about the originally reported issue now.

[gaearon]

facebook/react-native#26788 (comment)