Patch console to append component stacks #348

bvaughn · 2019-07-13T17:43:37Z

Auto-append component stack to console.error and console.warn calls.
Temporarily disable all console logging before re-running a function component to inspect hooks.
Make sure not to double append a stack to any warnings logged by React.
~~Test with multiple renderers~~ First injected renderer wins; console won't be patched for subsequent renderers

Resolves #347

Related React PR facebook/react#16133

Questions

~~Is the zero width space character (added in 6a196fd) reasonable or might it cause problems with any instrumented logging of warnings/errors?~~ This is out of DevTools hands since the component stack formatting is now being injected by React.
Is it okay to log owner stacks rather than full component stacks? Seems preferable to me, but it is different from the React official errors/warnings.

Demo

…atting

bvaughn · 2019-07-15T15:51:13Z

CI is failing because of a Yarn issue, yarnpkg/yarn#7403

bvaughn · 2019-07-15T22:17:33Z

~~At this point, the tests are expected to fail until facebook/react#16139 has landed.~~ This dependency has been removed.

…ions

gaearon · 2019-07-16T22:03:29Z

src/backend/console.js

+
+    try {
+      // $FlowFixMe property error|warn is not writable.
+      targetConsole[method] = overrideMethod;


One thing to be aware of is that this might cause the methods to lose location information displayed by the browser. Like when you warn/error and it says "foo.js:XX" in the upper right corner:

But now it would point inside DevTools.

People might find this very invasive, especially when we're not inside a React component. For example they may have been relying on clicking on those locations in their logs and warnings in the past.

I guess it's kind of unavoidable. Although would be nice to start a conversation with browsers if we can somehow specify to skip the topmost stack level when displaying it.

(We have a similar problem in CRA already because we also override it there.)

Yeah, I agree with this as a downside. I will reach out to some folks on Chrome's developer tools team to at least start that conversation.

One way to mitigate or limit impact from this would be to have DevTools call the Global Hook when it begins or exits the React work loop. Then you could only inject the overrides at that point, and restore them afterwards.

Hm... do you mean the work loop would call a function on the DevTools global hook (or otherwise notify it) any time it started/stopped work?

gaearon · 2019-07-16T22:08:45Z

src/hook.js

+        // because Webpack generates JS output that will runtime error with the typeof check.
+        // e.g. typeof patchConsole === 'function'
+        // becomes: typeof _backend_console__WEBPACK_IMPORTED_MODULE_0__["patch"] === 'function'
+        // and _backend_console__WEBPACK_IMPORTED_MODULE_0__ itself is undefined


Why is it undefined? I don't think I understand this.

There's a comment in the header about this too, but basically for the browser version- we inject the hook by to-stringing it.

react-devtools-experimental/shells/browser/shared/src/injectGlobalHook.js

Lines 85 to 89 in 0f2fb5b

// Inject a `__REACT_DEVTOOLS_GLOBAL_HOOK__` global so that React can detect that the

// devtools are installed (and skip its suggestion to install the devtools).

injectCode(

';(' + installHook.toString() + '(window))' + saveNativeValues + detectReact

);

Because of this, imports won't work- although they will for React Native.

If you have suggestions on the wording I would be happy to clean it up.

I pushed a wording improvement.

gaearon · 2019-07-16T22:10:05Z

src/backend/console.js

+            const ownerName =
+              owner != null ? getDisplayNameForFiber(owner) : null;
+
+            ownerStack += describeComponentFrame(


Is there any reason we wouldn't prefer to keep this logic in React, and inject a function that gives us a full owner stack instead?

We kind of talked about this 😄Maybe you missed my reply?

facebook/react#16139 (comment)

The relevant bit to this question is:

If React just injected ReactDebugCurrentFrame, then the only thing DevTools could use would be getStackAddendum()- which returns a component stack containing all ancestors, not just parents. By injecting a ref to the current Fiber, DevTools can choose what kind of component stack to generate (currently, owners-only).

I could inject a new method that gives an owners-only stack, or update the old one to accept a parameter- but I think it's kind of nice for DevTools to have a little more control over this in case we decide to change it later.

Yes, what I suggested is to add this owner-only function to ReactDebugCurrentFrame. :-) But fair enough.

bvaughn · 2019-07-17T18:07:18Z

I'm going to go ahead and make this a configurable preference, at least for now. It seems pretty invasive and might turn out to be annoying in ways I haven't accounted for.

bvaughn · 2019-07-17T18:11:50Z

I'm going to roll forward with this, since there didn't seem to be any strong concerns (just questions). Please feel free to follow up with me if there are concerns though.

Brian Vaughn added 2 commits July 13, 2019 10:43

Patch console to append component stacks

da7cdec

Updated auto-appended component stack to be owners-only

4654f11

bvaughn force-pushed the patch-console branch from e5d5639 to 4654f11 Compare July 15, 2019 01:10

Brian Vaughn added 2 commits July 15, 2019 08:33

Updated canary versions

5243ef4

Added zero-width space to component stack to prevent Chrome from form…

6a196fd

…atting

Brian Vaughn added 2 commits July 15, 2019 14:31

Updated react-is canary now that Yarn cache is busted

dcb7f28

Added test for console patch and component stacks

016afd5

Brian Vaughn added 6 commits July 16, 2019 08:39

Disable filters warning

97417dd

Ensure the console is only patched once

8293b34

Update tests to account for recent act changes

2ecdc60

Refactored console patching to not require additional renderer inject…

d9b3037

…ions

Destructure constants to remove property lookup in hot function

5daac23

Fixed console patching regression for DOM extension

aabc046

bvaughn requested a review from gaearon July 16, 2019 21:26

gaearon reviewed Jul 16, 2019

View reviewed changes

Brian Vaughn added 2 commits July 17, 2019 07:47

Improved clarity of inline comment

97a164a

Add preference to enable component stack overrides to be disabled

6b70d3c

bvaughn merged commit cb3fb42 into master Jul 17, 2019

bvaughn deleted the patch-console branch July 17, 2019 18:12

bvaughn mentioned this pull request Jul 18, 2019

Extending the console to improve developer experience whatwg/console#163

Open

	// Inject a `__REACT_DEVTOOLS_GLOBAL_HOOK__` global so that React can detect that the
	// devtools are installed (and skip its suggestion to install the devtools).
	injectCode(
	';(' + installHook.toString() + '(window))' + saveNativeValues + detectReact
	);

Patch console to append component stacks #348

Patch console to append component stacks #348

Uh oh!

Conversation

bvaughn commented Jul 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Questions

Demo

Uh oh!

bvaughn commented Jul 15, 2019

Uh oh!

bvaughn commented Jul 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gaearon Jul 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bvaughn Jul 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bvaughn commented Jul 17, 2019

Uh oh!

bvaughn commented Jul 17, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bvaughn commented Jul 13, 2019 •

edited

Loading

bvaughn commented Jul 15, 2019 •

edited

Loading

gaearon Jul 16, 2019 •

edited

Loading

bvaughn Jul 17, 2019 •

edited

Loading