allow react memo component can set displayName multiple times

[iChenLei]

TL;DR

1. React memo can set displayName ( begin at React v17 displayName from memoized components should be taken into account in DevTools #18026 (comment)
2. React memo displayName's weight is lower than inner wrapped component's displayName Match react-devtools display name behavior for React.memo #15313 (comment)
3. 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: ⬇️

1. React.forwardRef 👌
2. Normal Function Component 👌
3. Class Component 👌
4. 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 ?

1. For react ecosystem third party library. displayName not working in React 17 with observer mobxjs/mobx#2721 (comment)
2. 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]

No significant bundle size changes to report.

Size changes (experimental)

Generated by 🚫 dangerJS against 335f58f

[sizebot]

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

[iChenLei]

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

[bvaughn]

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]

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

[codeBelt]

Is there a timeline when this might be merged?

[ccheney]

LGTM

[bvaughn]

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.

[iChenLei]

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.

[iChenLei]

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

[bvaughn]

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.

[bvaughn]

See #21392

[iChenLei]

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]

closed, see bvaughn's alternative PR #21392