allow react memo component can set displayName multiple times #20659

iChenLei · 2021-01-26T05:23:30Z

TL;DR

React memo can set displayName ( begin at React v17 displayName from memoized components should be taken into account in DevTools #18026 (comment)
React memo displayName's weight is lower than inner wrapped component's displayName Match react-devtools display name behavior for React.memo #15313 (comment)
Change the current behavior to hoist memo displayName's show weight and allow set memo displayName multiple times.

React displayName

question: can we set component displayName multiple times?

codesandbox link 👉 https://codesandbox.io/s/react-17-memo-displayname-forked-yk4u0

answer: ⬇️

React.forwardRef 👌
Normal Function Component 👌
Class Component 👌
React.memo 🙅‍♂️

Deep Investigation

React.forwardRef displayName behavior

 //  packages/react/src/ReactForwardRef.js
  const elementType = {
    $$typeof: REACT_FORWARD_REF_TYPE,
    render,    👈🏻   // render: render not type: type
  };
  if (__DEV__) {
    let ownName;
    Object.defineProperty(elementType, 'displayName', {
      enumerable: false,
      configurable: true,
      get: function() {
        return ownName;
      },
      set: function(name) {
        ownName = name;
        if (render.displayName == null) {
          render.displayName = name;  👈🏻   //  only effect render property
        }
      },
    });
  }
  return elementType;
}

React forwardRef Component actually

You can modilfy displayName multiple times, because it's standalone getter/setter property.

React devtools getDisplayName logic

export function getDisplayName(
  type: Function,
  fallbackName: string = 'Anonymous',
): string {
  const nameFromCache = cachedDisplayNames.get(type);
  if (nameFromCache != null) {
    return nameFromCache;
  }
  let displayName = fallbackName;
  // The displayName property is not guaranteed to be a string.
  // It's only safe to use for our purposes if it's a string.
  // github.com/facebook/react-devtools/issues/803
  if (typeof type.displayName === 'string') {    👈🏻   // type.displayName
    displayName = type.displayName;
  } else if (typeof type.name === 'string' && type.name !== '') {
    displayName = type.name;
  }
  cachedDisplayNames.set(type, displayName);
  return displayName;
}

PR for what ?

For react ecosystem third party library. displayName not working in React 17 with observer mobxjs/mobx#2721 (comment)
Make React.forwardRef and React.memo have same behavior

@eps1lon 's previous comment for this:
This test was added in PR 18925 but I don't think this should've been the expected behavior (at bvaughn.
For React.memo we ignore displayName on the wrapper if we have a displayName on the wrapped component.
For React.forwardRef we honor displayName on the wrapper even if we have displayName on the wrapped component.
I think it is confusing that React.memo and React.forwardRef behave differently with regard to displayName.

Test Plan

Add a new unit test and modify a exist unit test. Need @bvaughn and @eps1lon check it.

append comment

welcome any advices helpful, I'm not ReactJS expert so maybe I do something wrong.

sizebot · 2021-01-26T05:28:12Z

No significant bundle size changes to report.

Size changes (experimental)

Generated by 🚫 dangerJS against 335f58f

sizebot · 2021-01-26T05:29:29Z

Comparing: 3c21aa8...44d947a

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js	=	122.49 kB	122.49 kB	=	39.38 kB	39.38 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js	=	129.02 kB	129.02 kB	=	41.46 kB	41.46 kB
facebook-www/ReactDOM-prod.classic.js	=	406.89 kB	406.89 kB	=	75.31 kB	75.31 kB
facebook-www/ReactDOM-prod.modern.js	=	394.75 kB	394.75 kB	=	73.34 kB	73.35 kB
facebook-www/ReactDOMForked-prod.classic.js	=	406.89 kB	406.89 kB	=	75.31 kB	75.31 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against 44d947a

…displayname

iChenLei · 2021-02-06T14:28:13Z

@bvaughn @eps1lon Can someone review this pull request? If it's not ok I will close it , Thanks.

bvaughn · 2021-02-08T18:24:04Z

I'll add myself as a reviewer to reminder myself at some point in the future but I'm pretty swamped right now so I'm not committing to look any time soon.

It doesn't hurt anything to leave the PR open so I suggest you just do that. :)

iChenLei · 2021-02-08T22:46:56Z

Sorry, it's my fault. Thanks for your advices.

codeBelt · 2021-04-19T17:46:00Z

Is there a timeline when this might be merged?

ccheney

LGTM

bvaughn · 2021-04-28T14:43:40Z

packages/react/src/ReactMemo.js

+          console.error(
+            "%s: is not valid displayName type, React memo's displayName should be a string",
+            typeof name,
+          );


Why not follow the exact behavior of forwardRef here?

react/packages/react/src/ReactForwardRef.js

Lines 51 to 64 in a0d6b15

let ownName;

Object.defineProperty(elementType, 'displayName', {

enumerable: false,

configurable: true,

get: function() {

return ownName;

},

set: function(name) {

ownName = name;

if (render.displayName == null) {

render.displayName = name;

}

},

});

Specifically, only set type.displayName = name if it's not already type.displayName === null.

react/packages/react-devtools-shared/src/backend/renderer.js

Lines 379 to 392 in a0d6b15

function resolveFiberType(type: any) {

const typeSymbol = getTypeSymbol(type);

switch (typeSymbol) {

case MEMO_NUMBER:

case MEMO_SYMBOL_STRING:

// recursively resolving memo type in case of memo(forwardRef(Component))

return resolveFiberType(type.type);

case FORWARD_REF_NUMBER:

case FORWARD_REF_SYMBOL_STRING:

return type.render;

default:

return type;

}

}

If we set ownName we get the ownName(ownName) as displayName and otherwise we use type.render's displayName or name.

react/packages/react-devtools-shared/src/backend/renderer.js

Lines 414 to 419 in a0d6b15

case ForwardRef:

// Mirror https://github.com/facebook/react/blob/7c21bf72ace77094fd1910cc350a548287ef8350/packages/shared/getComponentName.js#L27-L37

return (

(type && type.displayName) ||

getDisplayName(resolvedType, 'Anonymous')

);

Yep, you are right, we need change the forwardRef logic as same as memoComponent.

We just want allow user set displayName multi time, the common scenario is that user use third party react library such as mobx-react, mobx-react use hoc to wrap component and use memo to optimize rende performance. and now user can't set custom displayName to debug . for example (mobxjs/mobx#2721

My point was that I don't understand why we were making the implementation of forwardRef and memo different.

TBH I'm not sure that either one modifying type.displayName is a good idea b'c that's a side effect. We do it because it helps with component stacks but I'm not sure that's the best workaround there either.

For instance:

function MyComponent() { // ... } const MyMemoizedComponent = React.memo(MyComponent); MyMemoizedComponent.displayName = 'Blah'; // Now if I render <MyComponent> it will also have a displayName of "blah" // That doesn't seem good.

It's true that forwardRef and memo currently have different behaviors in component stacks and that's not good either. To me, this indicates that the right place to make this change is where we read the names rather than here. So I'm talking about getComponentNameFromType 😄

Maybe it would be clearer for me to post a PR with an alternate idea for this.

…displayname

iChenLei · 2021-04-29T03:48:57Z

For example, you can't reset displayName for debug. But if we change the original behavior we will break should honor a inner displayName if set on the wrapped function from packages/react-reconciler/src/__tests__/ReactMemo-test.js. I am really confused now. so sad.

iChenLei · 2021-04-30T01:40:13Z

closed, see bvaughn's alternative PR #21392

allow react memo component can set displayName multiple times

335f58f

facebook-github-bot added the CLA Signed label Jan 26, 2021

rheh approved these changes Feb 3, 2021

View reviewed changes

Merge branch 'master' of https://github.com/iChenLei/react into memo-…

d38a651

…displayname

bvaughn self-requested a review February 8, 2021 18:23

bvaughn mentioned this pull request Feb 12, 2021

Bug: Anonymous component name in developer tool with run build but not run start #20805

Closed

ccheney approved these changes Apr 19, 2021

View reviewed changes

bvaughn reviewed Apr 28, 2021

View reviewed changes

Merge branch 'master' of https://github.com/iChenLei/react into memo-…

44d947a

…displayname

bvaughn mentioned this pull request Apr 29, 2021

Unify React.memo and React.forwardRef display name logic #21392

Merged

iChenLei closed this Apr 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow react memo component can set displayName multiple times #20659

allow react memo component can set displayName multiple times #20659

iChenLei commented Jan 26, 2021 •

edited

Loading

sizebot commented Jan 26, 2021

sizebot commented Jan 26, 2021 •

edited

Loading

iChenLei commented Feb 6, 2021

bvaughn commented Feb 8, 2021

iChenLei commented Feb 8, 2021

codeBelt commented Apr 19, 2021

ccheney left a comment

bvaughn Apr 28, 2021 •

edited

Loading

iChenLei Apr 29, 2021

iChenLei Apr 29, 2021

bvaughn Apr 29, 2021

bvaughn Apr 29, 2021

iChenLei commented Apr 29, 2021

iChenLei commented Apr 30, 2021

	let ownName;
	Object.defineProperty(elementType, 'displayName', {
	enumerable: false,
	configurable: true,
	get: function() {
	return ownName;
	},
	set: function(name) {
	ownName = name;
	if (render.displayName == null) {
	render.displayName = name;
	}
	},
	});

	function resolveFiberType(type: any) {
	const typeSymbol = getTypeSymbol(type);
	switch (typeSymbol) {
	case MEMO_NUMBER:
	case MEMO_SYMBOL_STRING:
	// recursively resolving memo type in case of memo(forwardRef(Component))
	return resolveFiberType(type.type);
	case FORWARD_REF_NUMBER:
	case FORWARD_REF_SYMBOL_STRING:
	return type.render;
	default:
	return type;
	}
	}

	case ForwardRef:
	// Mirror https://github.com/facebook/react/blob/7c21bf72ace77094fd1910cc350a548287ef8350/packages/shared/getComponentName.js#L27-L37
	return (
	(type && type.displayName) \|\|
	getDisplayName(resolvedType, 'Anonymous')
	);

allow react memo component can set displayName multiple times #20659

allow react memo component can set displayName multiple times #20659

Conversation

iChenLei commented Jan 26, 2021 • edited Loading

TL;DR

React displayName

Deep Investigation

React.forwardRef displayName behavior

React forwardRef Component actually

React devtools getDisplayName logic

PR for what ?

Test Plan

append comment

sizebot commented Jan 26, 2021

Size changes (experimental)

sizebot commented Jan 26, 2021 • edited Loading

Critical size changes

Significant size changes

iChenLei commented Feb 6, 2021

bvaughn commented Feb 8, 2021

iChenLei commented Feb 8, 2021

codeBelt commented Apr 19, 2021

ccheney left a comment

Choose a reason for hiding this comment

bvaughn Apr 28, 2021 • edited Loading

Choose a reason for hiding this comment

iChenLei Apr 29, 2021

Choose a reason for hiding this comment

iChenLei Apr 29, 2021

Choose a reason for hiding this comment

bvaughn Apr 29, 2021

Choose a reason for hiding this comment

bvaughn Apr 29, 2021

Choose a reason for hiding this comment

iChenLei commented Apr 29, 2021

iChenLei commented Apr 30, 2021

iChenLei commented Jan 26, 2021 •

edited

Loading

sizebot commented Jan 26, 2021 •

edited

Loading

bvaughn Apr 28, 2021 •

edited

Loading