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

allow react memo component can set displayName multiple times #20659

Closed
wants to merge 3 commits into from
Closed

allow react memo component can set displayName multiple times #20659

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
335f58f
allow react memo component can set displayName multiple times
iChenLei Jan 26, 2021
d38a651
Merge branch 'master' of https://github.com/iChenLei/react into memo-…
iChenLei Feb 4, 2021
44d947a
Merge branch 'master' of https://github.com/iChenLei/react into memo-…
iChenLei Apr 29, 2021
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
29 changes: 26 additions & 3 deletions packages/react-reconciler/src/__tests__/ReactMemo-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -516,7 +516,7 @@ describe('memo', () => {
);
});

it('should honor a inner displayName if set on the wrapped function', () => {
it('should honor a outter displayName when wrapped component and memo component set displayName at the same time.', () => {
function Component(props) {
return <div {...props} />;
}
Expand All @@ -532,8 +532,31 @@ describe('memo', () => {
ReactNoop.render(<MemoComponent optional="foo" />),
).toErrorDev(
'Warning: Failed prop type: The prop `required` is marked as required in ' +
'`Foo`, but its value is `undefined`.\n' +
' in Foo (at **)',
'`Bar`, but its value is `undefined`.\n' +
' in Bar (at **)',
);
});

it('can set react memo component displayName multiple times', () => {
function Component(props) {
return <div {...props} />;
}
Component.displayName = 'Foo';

const MemoComponent = React.memo(Component);
MemoComponent.displayName = 'MemoComp01';
MemoComponent.displayName = 'MemoComp02';
MemoComponent.displayName = 'MemoComp03';
MemoComponent.propTypes = {
required: PropTypes.string.isRequired,
};

expect(() =>
ReactNoop.render(<MemoComponent optional="foo" />),
).toErrorDev(
'Warning: Failed prop type: The prop `required` is marked as required in ' +
'`MemoComp03`, but its value is `undefined`.\n' +
' in MemoComp03 (at **)',
);
});
}
Expand Down
9 changes: 7 additions & 2 deletions packages/react/src/ReactMemo.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,14 @@ export function memo<Props>(
return ownName;
},
set: function(name) {
ownName = name;
if (type.displayName == null) {
if (typeof name === 'string') {
ownName = name;
type.displayName = name;
} else {
console.error(
"%s: is not valid displayName type, React memo's displayName should be a string",
typeof name,
);
Copy link
Contributor

@bvaughn bvaughn Apr 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

See #21392

}
},
});
Expand Down