The Wayback Machine - https://web.archive.org/web/20220725165235/https://github.com/facebook/react/pull/21952
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.

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

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

Conversation

eps1lon
Copy link
Collaborator

@eps1lon eps1lon commented Jul 24, 2021

Summary

The displayed name of e.g.

const Component = forwardRef(function Component() {
  return null;
});
Component.displayName = `Component`;

will no longer include the ForwardRef badge in the element tree. This also applies to React.memo.

This matches how display names are computed in getComponentNameFromType e.g. in warnings.

Closes #21939

Test Plan

  • CI green
  • linked codesandboxes produce desired display name See ForwardRefComponentWithCustomDisplayName in react-devtools-shell
    Before:
    devtools-memo-custom-name-before
    After:
    devtools-memo-custom-name-after
    -Custom ForwardRef
    +Custom 
(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

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.

@eps1lon eps1lon marked this pull request as ready for review Jul 24, 2021
@eps1lon eps1lon requested a review from bvaughn Jul 24, 2021
@bvaughn
Copy link
Collaborator

@bvaughn bvaughn commented Jul 26, 2021

@ryota-murakami
Copy link
Contributor

@ryota-murakami ryota-murakami commented Aug 2, 2021

I think, DevTools should be there based on @bvaughn decided in previous conversation.
Even though I know that feeling @eps1lon mentioned situations are not good experience with DevTools by default.

But I think problem root is library implementation. most of React library overused HOC Context value injection layer, ReactDevTools right.
And more, React is only provided view as a library so external library tend to providing invisible component(Called HOC) as a API of their business logic.

As a result, causing noises against actual view component by invisible component that's just a library interface doesn't appear screen but existing on screen as a empty layer.

DevTools Hide components is right solution i think, finally my opinion about a badge that should't hide it because display component information is DevTools must role certainly.

@stale
Copy link

@stale stale bot commented Jan 8, 2022

This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated.

@stale stale bot added the Resolution: Stale label Jan 8, 2022
@eps1lon
Copy link
Collaborator Author

@eps1lon eps1lon commented Jan 9, 2022

Bump

@stale stale bot removed the Resolution: Stale label Jan 9, 2022
@eps1lon eps1lon force-pushed the feat/devtools/displayname-overrides-built-in-badges branch from 8789464 to d3bd393 Compare Jan 9, 2022
@inoyakaigor
Copy link

@inoyakaigor inoyakaigor commented May 18, 2022

@eps1lon any updates for this?

@eps1lon eps1lon force-pushed the feat/devtools/displayname-overrides-built-in-badges branch from d3bd393 to db8187d Compare Jul 2, 2022
@eps1lon
Copy link
Collaborator Author

@eps1lon eps1lon commented Jul 2, 2022

@eps1lon any updates for this?

Rebased and added screenshots and a digest of the diff in display names in devtools.

@bvaughn bvaughn requested review from mondaychen and lunaruan Jul 2, 2022
Copy link
Contributor

@lunaruan lunaruan left a comment

LGTM. Thanks for doing this!

): string {
const functionName = getDisplayName(innerType, fallbackName);
return (outerType: any).displayName || `${wrapperName}(${functionName})`;
}

Copy link
Contributor

@lunaruan lunaruan Jul 12, 2022

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

@eps1lon eps1lon Jul 12, 2022

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

@eps1lon eps1lon Jul 16, 2022

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.

@eps1lon eps1lon force-pushed the feat/devtools/displayname-overrides-built-in-badges branch from aa717ab to 6f0ccb5 Compare Jul 17, 2022
@eps1lon eps1lon merged commit b66936e into facebook:main Jul 25, 2022
36 checks passed
@eps1lon eps1lon deleted the feat/devtools/displayname-overrides-built-in-badges branch Jul 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment