Open main-- opened 3 years ago
Note how the component wrapped in
view()
is shown asReactiveComp
instead of either the function name or the explicitly assigneddisplayName
.
Just to be clear, ignoring the displayName
attribute is (I'm pretty sure) an intentional part of the "native component stacks" - which are supposed to exactly mimic the format of native stacks (Error().stack
) which also don't respect displayName
. The major benefits of this approach are:
displayName
in production builds.I appreciate the repro! Would it be possible to get one that doesn't include any external packages (like react-easy-state)? (It's always nice for the repro to be as small and self-contained as possible.)
Digging into the third party library you mentioned, I don't think the problem is actually "inheritance" at all but rather the way the component is being rendered in the HOC:
// create a memoized reactive wrapper of the original component (render)
// at the very first run of the component function
const render = useMemo(
() =>
observe(Comp, {
scheduler: () => setState({}),
lazy: true,
}),
// Adding the original Comp here is necessary to make React Hot Reload work
// it does not affect behavior otherwise
[Comp],
);
For example, this simplified repro shows the behavior you describe: https://codesandbox.io/s/condescending-cartwright-qn8c2?file=/src/App.tsx
But this one does not: https://codesandbox.io/s/exciting-pasteur-p98fn?file=/src/App.tsx
That's because the latter approach is not actually rendering Comp
as a React element but rather as a "render prop", which breaks our stack detection logic (since "render props" aren't React elements).
Thank you for the fast response!
Digging into the third party library you mentioned, I don't think the problem is actually "inheritance" at all but rather the way the component is being rendered in the HOC:
I think you're spot on that this is about render prop vs React element. Sorry, I confused the terminology.
So it looks like moving away from render props could be a workaround for now. I'm not sure if this is actually possible for react-easy-state though, but I can try.
One unfortunate aspect about this issue is that the docs still imply that displayName
can be used to specify what name should be used in component stack traces. Since displayName
used to override the inferred function name previously (and is still respected by the React Developer Tools extension), I think that ignoring it for component stack traces may be a bug.
I think that ignoring it for component stack traces may be a bug.
Can you share a reduced repro that shows this behavior specifically?
Looking at the code, it seems like the displayName
attribute should actually be respected (at least in DEV builds):
https://github.com/facebook/react/blob/50263d3273b6fc983acc5b0fd52e670399b248b1/packages/shared/ReactComponentStackFrame.js#L260-L262
Can you share a reduced repro that shows this behavior specifically?
Here you go: https://codesandbox.io/s/naughty-austin-jk28f?file=/src/App.tsx
It's basically the renderprop example you linked above, but instead of react-error-boundary I'm using a custom ErrorBoundary component (based on example code from somewhere in the docs).
We still have the two components BadGuy
and WrappedBadGuy
, the latter of which is wrapped in the dummy view()
HOC from your example that just passes the component as a render prop. For WrappedBadGuy
we have both an underlying function name (_WrappedBadGuy
) as well as an explicitly assigned displayName
(WrappedBadGuyDisplayName
). But still, the errorInfo.componentStack
just shows the second component as ReactiveComp
.
Ah, sorry. To be clear that is the expected behavior for the ReactiveComp
because WrappedBadGuy
is not being called as a React element but rather as a regular function (so React doesn't know anything about it / can't access / can't see it to read the displayName
prop).
I think there is a miscommunication here. WrappedBadGuy
is in fact called as a React element below:
<ErrorBoundary>
<WrappedBadGuy />
</ErrorBoundary>
What's being passed as a render prop is the inner function _WrappedBadGuy
(perhaps I should have picked a different name to make the difference clearer). But the displayName
is being assigned directly to the element, and not the inner function:
function view(Comp: any) {
let ReactiveComp = (props: any) => {
return Comp(props);
};
return ReactiveComp;
}
const WrappedBadGuy = view(function _WrappedBadGuy() {
throw Error("sorry");
});
(WrappedBadGuy as any).displayName = "WrappedBadGuyDisplayName";
Ah, you are correct. I missed this line:
(WrappedBadGuy as any).displayName = "WrappedBadGuyDisplayName";
I don't think I see how the view()
wrapper is relevant to this repro then :smile:
Seems like what you're really reporting is that a component like this:
function BadGuy() {
throw Error("sorry");
}
BadGuy.displayName = 'BadGuyDisplayName';
Will show up as BadGuy
in a component stack rather than BadGuyDisplayName
. Is that right?
In other words, it will show this:
The above error occurred in the <BadGuyDisplayName> component:
at BadGuy (https://jk28f.csb.app/src/App.tsx:27:9)
at ErrorBoundary
...
Here is a reduced repro: https://codesandbox.io/s/festive-ritchie-webcl?file=/src/App.tsx
Somehow it never occurred to me to test this behavior without some kind of HOC, but I think it really is that simple! :smile:
Okay! Glad we're on the same page. :)
I'm on-call for DevTools this week so I don't really have the bandwidth to dig in any further, but I think we have enough info now for this to be discussed/decided.
I'm not positive but I don't think what you're looking for is possible (reading the displayName
for the "native stack") because we use Error
to construct these stacks (so they'll be in the exact same format) and Error
doesn't care about the displayName
convention.
You can Object.defineProperty
instead for name
. This works: https://codesandbox.io/s/tender-easley-vck0q?file=/src/App.tsx
function BadGuy() {
throw Error("sorry");
}
Object.defineProperty(BadGuy, "name", { value: "BadGuyDisplayName" });
Oh that's a nice point, Dan. I had forgotten about that.
Using Object.defineProperty
changes the name used in the error message, but not in the component stack trace: https://codesandbox.io/s/jolly-sea-s7tl4
That's not the behavior I see in Chrome on OSX:
Interesting! I can confirm that it works on Chromium (93.0.4577.63 Arch Linux
) but breaks on Firefox (91.0.2 Arch Linux
), where my screenshot was taken.
Firefox (left) vs Chromium (right)
Seems like overriding Function.name
using Object.defineProperty
changes the name shown in native stack traces on Chromium but not on Firefox.
Just to be clear, displayName
is broken (at least for me) in all browsers. The problem looks like this: Native Component Stacks appear to just ignore displayName
right now. Instead, it always prints the native function name. Using the Object.defineProperty
hack, it is possible to override the native function name in some browsers (Chrome), but this approach seems very hacky and fragile. It also does not work with the library that's triggering my issue (react-easy-state) - not sure why, might be related to memoization.
Native Component Stacks appear to just ignore
displayName
right now.
PR #22477 may fix the issue being reported (without requiring the defineProperty
hack).
Looks like this issue has been inactive for awhile - if you'd prefer I open a new issue, let me know.
The following issue still exists:
Just to be clear, displayName is broken (at least for me) in all browsers. The problem looks like this: Native Component Stacks appear to just ignore displayName right now. Instead, it always prints the native function name.
From reading the underlying React Source code, this behavior appears to be intentional, with the idea being native component stacks will give you the most accurate information for debugging. I agree with this to a point, but as noted earlier in the thread, this is very problematic for Higher Order Components which want to preserve the name of the component they're wrapping.
In fact, this is such a primary use case that it is covered specifically in the React docs: https://reactjs.org/docs/higher-order-components.html#convention-wrap-the-display-name-for-easy-debugging
As it stands currently, that documentation is not accurate, because defining displayName
doesn't do anything.
This is all independent of alternate mechanisms such as the previously mentioned defineProperty
hack. Regardless of whether this hack works or not, the lack of support for displayName
should be considered a bug, in my opinion, until the documentation is updated and/or a new convention is provided for how to make higher order components debuggable.
Accordingly, I would at least petition to drop "in Firefox" from this issue title, as the displayName
issue is browser independent - it is only the workaround hack that is related to Firefox.
My personal suggestion is to override a portion of the stack frame with the displayName
if it is set. This would require only a slight alteration to the code introduced by PR #22477 mentioned above, by adding the following else if
clause:
if (fn.displayName && _frame.includes('<anonymous>')) {
_frame = _frame.replace('<anonymous>', fn.displayName);
} else if (fn.displayName) {
_frame = _frame.replace(fn.name, fn.displayName);
}
I am not certain if this is robust enough for general use, but it solved the problem for me and is at least a step in the right direction.
Given the age of this issue, it seems like it won't be fixed any time soon. However, I did find a workaround!
To recap, the problem is that given a snippet like this:
function higherOrderComponent(wrapped) {
const wrappedComponent = (props) => {
return wrapped(props);
};
return wrappedComponent
}
the closure will simply be named wrappedComponent
and so the original name of the wrapped component (wrapped.name
) is lost. While the most convenient solution here would undoubtedly be if React had a way to just override that name, we have to assume for now that this feature is not coming back.
So we need a way to set the actual name (and not just a shadowed "name" property like the hack for Chrome) of our closure to whatever wrapped.name
says. It seems impossible - after all, the closure is named by the variable, and variable names are source code literals. So we can't just create a variable with a dynamic name, can we? In fact, we can do just that:
const wrappedComponent = { [wrapped.name](props) {
return wrapped(props);
} }[wrapped.name];
Also works for class components:
const WrappedClassComp = { [Wrapped.name]: class extends Wrapped {
// ...
} }[Wrapped.name];
Implemented in react-easy-state here: https://github.com/RisingStack/react-easy-state/commit/458e4bb8b9367a082f80d9ce15a45d58b06c7333
Edit See this comment for the key part of what's being reported/discussed on this issue: https://github.com/facebook/react/issues/22315#issuecomment-920061727
Original bug report below
As of #18561 component stacks are generated from native stack frames. This is problematic with HOCs that inherit from the input component in order to change its behavior. The somewhat popular @risingstack/react-easy-state package is one example of such a component. While it does assign a
displayName
, the new Native Component Stacks appear to ignore this. Instead, components wrapped inview()
(from react-easy-state) are always shown with the name of the wrapper class, i.e.,ReactiveComp
orReactiveClassComp
.This is especially catastrophic in the case of react-easy-state, where one is supposed to wrap essentially all components in the entire codebase in the
view()
HOC. The result is that component stacks become unusable for debugging.Is there perhaps a way to work around this (e.g. disable native component stacks, or some new way to explicitly provide a component name like
displayName
)?React version: 17.0.1
Steps To Reproduce
ReactiveComp
orReactiveClassComp
in component stack traces.Link to code example: https://codesandbox.io/s/rough-tdd-wqepe?file=/src/App.tsx
The current behavior
Note how the component wrapped in
view()
is shown asReactiveComp
instead of either the function name or the explicitly assigneddisplayName
.The expected behavior
The name of the
ReactiveComp
wrapper should never appear in component stacks.