-
Notifications
You must be signed in to change notification settings - Fork 49k
Closed
Labels
Component: Developer ToolsStatus: UnconfirmedA potential issue that we haven't yet confirmed as a bugA potential issue that we haven't yet confirmed as a bugType: Discussion
Description
Given
const StyleDiv = forwardRef(function Component({ children }, ref) {
return <div ref={ref}>{children}</div>;
});
StyleDiv.displayName = `styled(connected(div))`;
-- https://codesandbox.io/s/little-sky-y8h1b?file=/src/App.js
I would expect that the badges from the display name are prioritized in the component tree.
However, devtools currently displays the ForwardRef
badge first:
Oddly enough, the inline devtools in codesandbox do prioritize the badge from the displayName
(maybe this regressed?):
There's also an argument to be made that devtools should not display the ForwardRef
badge to begin with (since we explicitly omitted it in displayName
). That can be discussed separately but would solve the issue entirely.
Metadata
Metadata
Assignees
Labels
Component: Developer ToolsStatus: UnconfirmedA potential issue that we haven't yet confirmed as a bugA potential issue that we haven't yet confirmed as a bugType: Discussion
Type
Projects
Milestone
Relationships
Development
Select code repository
Activity
styled
component ifname
isn't set mui/material-ui#27401bvaughn commentedon Jul 22, 2021
Just an FYI, the
HocBadges
component (the one that displays badges for the inspected component, in the right panel) just iterates over the list of display names:react/packages/react-devtools-shared/src/devtools/views/Components/HocBadges.js
Lines 26 to 35 in ae5d261
This list of names is calculated using this helper util which first parses the
displayName
and then unshifts special caseforwardRef
andmemo
names:react/packages/react-devtools-shared/src/utils.js
Lines 339 to 381 in ae5d261
The badge shown in the tree, beside the name, uses this same value and just displays the first name:
react/packages/react-devtools-shared/src/devtools/views/Components/Element.js
Lines 162 to 172 in ae5d261
Code Sandbox uses a pretty old version of DevTools b'c no one has updated it anytime recently and the one time I tried, I was unable to get the project running locally.
Regardless, what order would you expect them to be in? This order kind of seemed reasonable to me but I'm open for discussion. (Let's account for both
forwardRef
andmemo
in whatever proposal we discuss.)eps1lon commentedon Jul 23, 2021
I've thought about this more and the order does make sense generally e.g.
styled(connected(fancy(logged(Component))))
should display asComponent Styled Connected Fancy Logged
I think my issue is more that
ForwardRef
andmemo
are displayed even though I've set an explicitdisplayName
. Conceptually inthe
forwardRef
is part ofstyled
. So they're not independent higher-order components but a single one.The current devtools behavior is confusing for another reason:
shared/getComponentNameFromType
ignoresforwardRef
andmemo
as well if we set adisplayName
e.g. inComponent
will not have the computed display nameForwardRef(Component)
(e.g. inpropType
warnings) butComponent
. However, devtools will display it asComponent ForwardRef
: https://codesandbox.io/s/displayname-devtools-vs-react-itself-if6dgSo I think this is less about the order (otherwise where do we splice in
ForwardRef
if not at the start?) but more about not displayingForwardRef
at all if we explicitly set adisplayName
.Because right now most modern higher-order components (
styled-components
,react-redux
,emotion
) do return aforwardRef
component but their badge will not appear in the component tree. All of these components are displayed either asforwardRef
ormemo
and I don't find that information helpful. I'd rather see the badges of the higher-order components:-- https://codesandbox.io/s/displayname-of-popular-higher-order-components-l7yw3
bvaughn commentedon Jul 23, 2021
That's an interesting POV.
Seems like a pretty compelling argument. Want to post a PR?
markerikson commentedon Jul 24, 2021
Yeah, I'd echo this. For libs like these,
forwardRef
is an implementation detail of the actual HOC, not a primary aspect by itself. Checking for adisplayName
seems like a reasonable way to figure that out.(technically
connect
doesn't actually add aforwardRef
unless users specifically opt into that by passing an option, but close enough)eps1lon commentedon Jul 24, 2021
That's how I see it as well.
My pleasure.
displayName
is set #21952bvaughn commentedon Jul 26, 2021
I think the linked PR isn't quite what I thought we'd settled on in the discussion above.
I don't think we should change e.g. memo from a badge (
Component [memo]
) to a wrapper string (Memo(Component)
). I thought we said we'd with the badge, except for when adisplayName
is specified, in which case we should use it instead?I think the comparison to
packages/shared/getComponentNameFromType
is a little bit of an apples-to-oranges comparison, since that's a plain text warning and the DevTools Components tree is a rich UI.That being said, I can also see an argument for
React.memo
andReact.forwardRef
being substantially different from HOCs and needing a different visual treatment as a result. Maybe the solution would be to not show a [memo] or [forwardRef] badge in the tree view at all, but show it in the inspected props on the right hand side when selected? (It may be an "implementation detail" but that can still be useful information when debugging things, no?)How would you feel about doing this instead?
eps1lon commentedon Jul 26, 2021
That was the goal of the PR. I think it's just not immediately obvious that that's the change. But we can discuss this in the PR.
The problem is that these things can usually be mapped onto another. And how each treats
memo
/forwardRef
right now makes the mapping harder than it could be.For library authors maybe but then we could always add the
memo(...)
ourselves.Though I wouldn't mind having an extra signifier that a certain component is wrapped in
memo
orforwardRef
(or even rendered vialazy
?).bvaughn commentedon Jul 26, 2021
I think it could be useful info for more than just library authors (e.g. a "memo" indicator would explain why a component isn't re-rendering when things around it are). Probably not useful enough to be in the main tree view, but in the inspected panel on the side?
I think we could use the same visual area, but just style the memo/forwardRef badges differently (different color).