Refactor Component Stack Traces #18495

sebmarkbage · 2020-04-04T23:54:05Z

I'm going to play with a different way to extract prod stack traces from native stack traces as well as building Hermes specific ones. This doesn't do that yet but it's setting up some refactoring for it.

Notably this no longer uses the getComponentName helper. This means that getComponentName is now only DEV only and can be safely be DCE. Component stacks are the only way we expose names in PROD, outside of DEV tools.

This approach also relies on tag instead of a .type field existing. Some fibers don't need that type field for anything except this use case.

I also ensured to carefully wrap things in __DEV__ that removes some of the paths related to "source info" that was also included in prod even though that's no longer possible in the jsxDEV transform.

sebmarkbage · 2020-04-04T23:55:19Z

packages/react-reconciler/src/ReactFiberComponentStack.js

+      : null
+    : null;
+  const source = __DEV__ ? fiber._debugSource : null;
+  switch (fiber.tag) {


The list of things that get included was kind of arbitrary and not fully consistent before because it was filtered in Fiber but not in SSR. This list seems like the most commonly useful.

codesandbox-ci · 2020-04-04T23:55:38Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 5ccb1d1:

Sandbox	Source
reverent-wildflower-tpdpf	Configuration

sebmarkbage · 2020-04-04T23:57:48Z

packages/shared/ReactComponentStackFrame.js

+  let sourceInfo = '';
+  if (source) {
+    const path = source.fileName;
+    let fileName = path.replace(BEFORE_SLASH_RE, '');


This should probably be wrapped in DEV.

sizebot · 2020-04-04T23:58:08Z

	Warnings
⚠️	Base commit is broken: `a387566`

Size changes (experimental)

Generated by 🚫 dangerJS against 5ccb1d1

sizebot · 2020-04-04T23:59:16Z

	Warnings
⚠️	Base commit is broken: `a387566`

Size changes (stable)

Generated by 🚫 dangerJS against 5ccb1d1

sebmarkbage · 2020-04-05T23:08:30Z

The "in StrictMode" approach doesn't really work because it doesn't apply to Concurrent and Batched mode environments which are also strict. Which we'll likely slowly adopt even in the legacy environments that are not considered modern. So I'm not sure what that is supposed to do exactly.

gaearon · 2020-04-06T01:41:40Z

We could read the current fiber’s mode and pass it from the fork.

bvaughn · 2020-04-06T01:55:07Z

Why should we expose any info about the mode though?

sebmarkbage · 2020-04-06T05:37:48Z

I started down an implementation like this but I don't think mode is really a generally applicable thing here. It's really arbitrary what kind of filtering you want based on your adoption strategy. Seems like some kind of explicit Context is a more appropriate approach here.

sebmarkbage · 2020-04-06T05:46:01Z

I guess you can just add a wrapper component of any unique name and use that name as the magic string. I don't know which internal uses of StrictMode actually matters, that are not concurrent mode, so I'll let someone else do that.

gaearon · 2020-04-06T16:01:07Z

D20870861

gaearon · 2020-04-06T16:59:47Z

packages/react/src/__tests__/forwardRef-test.js

+    );
+  });
+
+  it('should honor a displayName in stacks if set on the inner function', () => {


This will make some people unhappy. Can we make it work with the outer one too in DEV? For all the wrappers: lazy, memo, forwardRef.

What is worse is that Firefox doesn't respect displayName in stacks at all so none of them will be respected in Firefox. However, that's also true for regular error stacks so they should just fix that there. The goal is to be no better or worse than native stacks - and ideally just like native stacks.

I don't really want to add code to parse and try to piece together a line by trying to inject a name in the middle of the browser native format somehow.

What we could do is transfer the displayName onto the inner function if one is not already defined. This would be invasive in the sense that it's mutating an object not owned by React.

I added a forwarding setter in DEV. 9054ad2

Not for lazy though because you're suppose to use that on the consuming side, and that's not where you specify things like propTypes or displayName. Not sure they should really be used with forwardRef/memo neither. It should really be the inner function that you name using a function expression.

Ofc we had a test that reused a component. This trick doesn't work in this case. I think it's unusual though. 5449838

You can get stack from any fiber, not just current.

These should use fiber tags for switching. This also puts the relevant code behind DEV flags.

They're not super useful and will go away later anyway.

Context is no longer part of SSR stacks. This was already the case on the client. forwardRef no longer is wrapped on the stack. It's still in getComponentName but it's probably just noise in stacks. Eventually we'll remove the wrapper so it'll go away anyway. If we use native stack frames they won't have this extra wrapper. It also doesn't pick up displayName from the outer wrapper. We could maybe transfer it but this will also be fixed by removing the wrapper.

…n DEV This allows them to show up in stack traces. I'm not doing this for lazy because lazy is supposed to be called on the consuming side so you shouldn't assign it a name on that end. Especially not one that mutates the inner.

We mutate the inner component for its name so we need multiple copies.

bvaughn · 2020-04-07T16:23:07Z

packages/shared/ReactComponentStackFrame.js

+  source: void | null | Source,
+  ownerFn: void | null | Function,
+): string {
+  return describeFunctionComponentFrame(ctor, source, ownerFn);


This looks like a bug. ownerFn should not be passed directly. Why didn't Flow catch this?

Oh. Whoops. I misread this as calling describeComponentFrame. My bad.

Summary: In the next react sync we removed the `displayName` from forwardRef and useMemo components ([PR here](facebook/react#18495 )). This means we need to manually add the displayName in the mock. Changelog: [General] [Fixed] Fix test renderer mocks to use the displayName more often. Reviewed By: TheSavior Differential Revision: D22775470 fbshipit-source-id: 1390dc325e34f7ccea32bbdf1c6a8f6efea3a080

sebmarkbage requested review from bvaughn and gaearon April 4, 2020 23:54

facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Apr 4, 2020

sebmarkbage commented Apr 4, 2020

View reviewed changes

sebmarkbage force-pushed the proderrorstacks branch 2 times, most recently from 1ce4b9a to b1e878f Compare April 6, 2020 05:15

gaearon reviewed Apr 6, 2020

View reviewed changes

gaearon approved these changes Apr 6, 2020

View reviewed changes

sebmarkbage added 6 commits April 6, 2020 14:31

Add feature flag

4a7480d

Split stack from current fiber

394db23

You can get stack from any fiber, not just current.

Refactor description of component frames

35e1403

These should use fiber tags for switching. This also puts the relevant code behind DEV flags.

We no longer expose StrictMode in component stacks

1438716

They're not super useful and will go away later anyway.

sebmarkbage force-pushed the proderrorstacks branch 2 times, most recently from afb7bc5 to 5449838 Compare April 6, 2020 22:10

Use multiple instances of the fake component

5ccb1d1

We mutate the inner component for its name so we need multiple copies.

sebmarkbage force-pushed the proderrorstacks branch from 5449838 to 5ccb1d1 Compare April 6, 2020 22:30

sebmarkbage merged commit 4169420 into facebook:master Apr 6, 2020

bvaughn reviewed Apr 7, 2020

View reviewed changes

idmadj mentioned this pull request May 4, 2020

displayName from memoized components should be taken into account in DevTools #18026

Closed

bvaughn mentioned this pull request Jul 17, 2020

Conditionally enable profiler component stacks #19396

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Component Stack Traces #18495

Refactor Component Stack Traces #18495

sebmarkbage commented Apr 4, 2020

sebmarkbage Apr 4, 2020

codesandbox-ci bot commented Apr 4, 2020 •

edited

Loading

sebmarkbage Apr 4, 2020

sizebot commented Apr 4, 2020 •

edited

Loading

sizebot commented Apr 4, 2020 •

edited

Loading

sebmarkbage commented Apr 5, 2020

gaearon commented Apr 6, 2020

bvaughn commented Apr 6, 2020

sebmarkbage commented Apr 6, 2020

sebmarkbage commented Apr 6, 2020 •

edited

Loading

gaearon commented Apr 6, 2020

gaearon Apr 6, 2020

sebmarkbage Apr 6, 2020

sebmarkbage Apr 6, 2020 •

edited

Loading

sebmarkbage Apr 6, 2020

bvaughn Apr 7, 2020

bvaughn Apr 7, 2020

Refactor Component Stack Traces #18495

Refactor Component Stack Traces #18495

Conversation

sebmarkbage commented Apr 4, 2020

sebmarkbage Apr 4, 2020

Choose a reason for hiding this comment

codesandbox-ci bot commented Apr 4, 2020 • edited Loading

sebmarkbage Apr 4, 2020

Choose a reason for hiding this comment

sizebot commented Apr 4, 2020 • edited Loading

Size changes (experimental)

sizebot commented Apr 4, 2020 • edited Loading

Size changes (stable)

sebmarkbage commented Apr 5, 2020

gaearon commented Apr 6, 2020

bvaughn commented Apr 6, 2020

sebmarkbage commented Apr 6, 2020

sebmarkbage commented Apr 6, 2020 • edited Loading

gaearon commented Apr 6, 2020

gaearon Apr 6, 2020

Choose a reason for hiding this comment

sebmarkbage Apr 6, 2020

Choose a reason for hiding this comment

sebmarkbage Apr 6, 2020 • edited Loading

Choose a reason for hiding this comment

sebmarkbage Apr 6, 2020

Choose a reason for hiding this comment

bvaughn Apr 7, 2020

Choose a reason for hiding this comment

bvaughn Apr 7, 2020

Choose a reason for hiding this comment

codesandbox-ci bot commented Apr 4, 2020 •

edited

Loading

sizebot commented Apr 4, 2020 •

edited

Loading

sizebot commented Apr 4, 2020 •

edited

Loading

sebmarkbage commented Apr 6, 2020 •

edited

Loading

sebmarkbage Apr 6, 2020 •

edited

Loading