Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

devtools: Remove ForwardRef/Memo from display name if displayName is set #21952

Merged
merged 2 commits into from
Jul 25, 2022
Merged

devtools: Remove ForwardRef/Memo from display name if displayName is set #21952

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
db8187d
feat(devtools): Remove ForwardRef/Memo from display name if `displayN…
eps1lon Jul 24, 2021
6f0ccb5
Avoid potentially wasting work by inlining `functionName`
eps1lon Jul 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 18 additions & 18 deletions packages/react-devtools-shared/src/__tests__/profilingCharts-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,8 +125,8 @@ describe('profiling charts', () => {
"actualDuration": 0,
"didRender": true,
"id": 5,
"label": "Child key=\\"third\\" (<0.1ms of <0.1ms)",
"name": "Child",
"label": "Memo(Child) key=\\"third\\" (<0.1ms of <0.1ms)",
"name": "Memo(Child)",
"offset": 15,
"selfDuration": 0,
"treeBaseDuration": 0,
Expand All @@ -135,8 +135,8 @@ describe('profiling charts', () => {
"actualDuration": 2,
"didRender": true,
"id": 4,
"label": "Child key=\\"second\\" (2ms of 2ms)",
"name": "Child",
"label": "Memo(Child) key=\\"second\\" (2ms of 2ms)",
"name": "Memo(Child)",
"offset": 13,
"selfDuration": 2,
"treeBaseDuration": 2,
Expand All @@ -145,8 +145,8 @@ describe('profiling charts', () => {
"actualDuration": 3,
"didRender": true,
"id": 3,
"label": "Child key=\\"first\\" (3ms of 3ms)",
"name": "Child",
"label": "Memo(Child) key=\\"first\\" (3ms of 3ms)",
"name": "Memo(Child)",
"offset": 10,
"selfDuration": 3,
"treeBaseDuration": 3,
Expand Down Expand Up @@ -176,8 +176,8 @@ describe('profiling charts', () => {
"actualDuration": 0,
"didRender": false,
"id": 5,
"label": "Child key=\\"third\\"",
"name": "Child",
"label": "Memo(Child) key=\\"third\\"",
"name": "Memo(Child)",
"offset": 15,
"selfDuration": 0,
"treeBaseDuration": 0,
Expand All @@ -186,8 +186,8 @@ describe('profiling charts', () => {
"actualDuration": 0,
"didRender": false,
"id": 4,
"label": "Child key=\\"second\\"",
"name": "Child",
"label": "Memo(Child) key=\\"second\\"",
"name": "Memo(Child)",
"offset": 13,
"selfDuration": 0,
"treeBaseDuration": 2,
Expand All @@ -196,8 +196,8 @@ describe('profiling charts', () => {
"actualDuration": 0,
"didRender": false,
"id": 3,
"label": "Child key=\\"first\\"",
"name": "Child",
"label": "Memo(Child) key=\\"first\\"",
"name": "Memo(Child)",
"offset": 10,
"selfDuration": 0,
"treeBaseDuration": 3,
Expand Down Expand Up @@ -267,20 +267,20 @@ describe('profiling charts', () => {
},
Object {
"id": 3,
"label": "Child (Memo) key=\\"first\\" (3ms)",
"name": "Child",
"label": "Memo(Child) (Memo) key=\\"first\\" (3ms)",
"name": "Memo(Child)",
"value": 3,
},
Object {
"id": 4,
"label": "Child (Memo) key=\\"second\\" (2ms)",
"name": "Child",
"label": "Memo(Child) (Memo) key=\\"second\\" (2ms)",
"name": "Memo(Child)",
"value": 2,
},
Object {
"id": 5,
"label": "Child (Memo) key=\\"third\\" (<0.1ms)",
"name": "Child",
"label": "Memo(Child) (Memo) key=\\"third\\" (<0.1ms)",
"name": "Memo(Child)",
"value": 0,
},
]
Expand Down
6 changes: 3 additions & 3 deletions packages/react-devtools-shared/src/__tests__/store-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1505,16 +1505,16 @@ describe('Store', () => {
<MyComponent> [ForwardRef]
▾ <Anonymous> [ForwardRef]
<MyComponent2>
<Custom> [ForwardRef]
<Custom>
<MyComponent4> [Memo]
▾ <MyComponent> [Memo]
<MyComponent> [ForwardRef]
<Baz> [withFoo][withBar]
<Baz> [Memo][withFoo][withBar]
<Baz> [ForwardRef][withFoo][withBar]
<Cache>
<memoRefOverride> [Memo]
<forwardRefOverride> [ForwardRef]
<memoRefOverride>
<forwardRefOverride>
`);
});

Expand Down
20 changes: 12 additions & 8 deletions packages/react-devtools-shared/src/backend/renderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import {
import {
deletePathInObject,
getDisplayName,
getWrappedDisplayName,
getDefaultComponentFilters,
getInObject,
getUID,
Expand Down Expand Up @@ -451,10 +452,11 @@ export function getInternalReactConstants(
case IndeterminateComponent:
return getDisplayName(resolvedType);
case ForwardRef:
bvaughn marked this conversation as resolved.
Show resolved Hide resolved
// Mirror https://github.com/facebook/react/blob/7c21bf72ace77094fd1910cc350a548287ef8350/packages/shared/getComponentName.js#L27-L37
bvaughn marked this conversation as resolved.
Show resolved Hide resolved
return (
(type && type.displayName) ||
getDisplayName(resolvedType, 'Anonymous')
return getWrappedDisplayName(
elementType,
resolvedType,
'ForwardRef',
'Anonymous',
);
case HostRoot:
const fiberRoot = fiber.stateNode;
Expand All @@ -475,10 +477,12 @@ export function getInternalReactConstants(
return 'Lazy';
case MemoComponent:
case SimpleMemoComponent:
return (
(elementType && elementType.displayName) ||
(type && type.displayName) ||
getDisplayName(resolvedType, 'Anonymous')
// Display name in React does not use `Memo` as a wrapper but fallback name.
Copy link
Collaborator Author

@eps1lon eps1lon Jul 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should sync the display names for memo as well. React will compute Component while devtools will display it as Memo(Component). No strong opinion whether we want to use Memo as a wrapper or fallback. For me personally React.memo is used like a higher-order component i.e. Memo as a wrapper is intuitive for me.

return getWrappedDisplayName(
elementType,
resolvedType,
'Memo',
'Anonymous',
);
case SuspenseComponent:
return 'Suspense';
Expand Down
25 changes: 11 additions & 14 deletions packages/react-devtools-shared/src/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,17 @@ export function getAllEnumerableKeys(
return keys;
}

// Mirror https://github.com/facebook/react/blob/7c21bf72ace77094fd1910cc350a548287ef8350/packages/shared/getComponentName.js#L27-L37
export function getWrappedDisplayName(
outerType: mixed,
innerType: any,
wrapperName: string,
fallbackName?: string,
): string {
const functionName = getDisplayName(innerType, fallbackName);
return (outerType: any).displayName || `${wrapperName}(${functionName})`;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Could just inline this

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea was to just copy the logic from whatever React packages are using so that it's easier to sync it. But then Devtools cover a bunch of React versions so it's probably fine to do things differently. Will check if this is trivial to do (i.e. do I remember how this one year old PR works ;D )

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. It actually save potentially wasted work if outerType.displayName is set.

export function getDisplayName(
type: Function,
fallbackName: string = 'Anonymous',
Expand Down Expand Up @@ -445,20 +456,6 @@ export function separateDisplayNameAndHOCs(
break;
}

if (type === ElementTypeMemo) {
if (hocDisplayNames === null) {
hocDisplayNames = ['Memo'];
} else {
hocDisplayNames.unshift('Memo');
}
} else if (type === ElementTypeForwardRef) {
if (hocDisplayNames === null) {
hocDisplayNames = ['ForwardRef'];
} else {
hocDisplayNames.unshift('ForwardRef');
}
}

return [displayName, hocDisplayNames];
}

Expand Down