Skip to content

Commit

Permalink
Unify React.memo and React.forwardRef display name logic (#21392)
Browse files Browse the repository at this point in the history
Co-authored-by: iChenLei <[email protected]>
  • Loading branch information
Brian Vaughn and iChenLei authored May 4, 2021
1 parent d19257b commit d1542de
Show file tree
Hide file tree
Showing 8 changed files with 220 additions and 42 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -670,20 +670,3 @@ exports[`Store should properly serialize non-string key values: 1: mount 1`] = `
[root]
<Child key="123">
`;

exports[`Store should show the right display names for special component types 1`] = `
[root]
▾ <App>
<MyComponent>
<MyComponent> [ForwardRef]
▾ <Anonymous> [ForwardRef]
<MyComponent2>
<Custom> [ForwardRef]
<MyComponent4> [Memo]
▾ <MyComponent> [Memo]
<MyComponent> [ForwardRef]
<Baz> [withFoo][withBar]
<Baz> [Memo][withFoo][withBar]
<Baz> [ForwardRef][withFoo][withBar]
<Cache>
`;
32 changes: 31 additions & 1 deletion packages/react-devtools-shared/src/__tests__/store-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -879,6 +879,17 @@ describe('Store', () => {
FakeHigherOrderComponent,
);

const MemoizedFakeHigherOrderComponentWithDisplayNameOverride = React.memo(
FakeHigherOrderComponent,
);
MemoizedFakeHigherOrderComponentWithDisplayNameOverride.displayName =
'memoRefOverride';
const ForwardRefFakeHigherOrderComponentWithDisplayNameOverride = React.forwardRef(
FakeHigherOrderComponent,
);
ForwardRefFakeHigherOrderComponentWithDisplayNameOverride.displayName =
'forwardRefOverride';

const App = () => (
<React.Fragment>
<MyComponent />
Expand All @@ -891,6 +902,8 @@ describe('Store', () => {
<MemoizedFakeHigherOrderComponent />
<ForwardRefFakeHigherOrderComponent />
<React.unstable_Cache />
<MemoizedFakeHigherOrderComponentWithDisplayNameOverride />
<ForwardRefFakeHigherOrderComponentWithDisplayNameOverride />
</React.Fragment>
);

Expand All @@ -904,7 +917,24 @@ describe('Store', () => {
// Render again after it resolves
act(() => ReactDOM.render(<App />, container));

expect(store).toMatchSnapshot();
expect(store).toMatchInlineSnapshot(`
[root]
▾ <App>
<MyComponent>
<MyComponent> [ForwardRef]
▾ <Anonymous> [ForwardRef]
<MyComponent2>
<Custom> [ForwardRef]
<MyComponent4> [Memo]
▾ <MyComponent> [Memo]
<MyComponent> [ForwardRef]
<Baz> [withFoo][withBar]
<Baz> [Memo][withFoo][withBar]
<Baz> [ForwardRef][withFoo][withBar]
<Cache>
<memoRefOverride> [Memo]
<forwardRefOverride> [ForwardRef]
`);
});

describe('Lazy', () => {
Expand Down
8 changes: 6 additions & 2 deletions packages/react-devtools-shared/src/backend/renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -393,7 +393,7 @@ export function getInternalReactConstants(

// NOTICE Keep in sync with shouldFilterFiber() and other get*ForFiber methods
function getDisplayNameForFiber(fiber: Fiber): string | null {
const {type, tag} = fiber;
const {elementType, type, tag} = fiber;

let resolvedType = type;
if (typeof type === 'object' && type !== null) {
Expand Down Expand Up @@ -432,7 +432,11 @@ export function getInternalReactConstants(
return 'Lazy';
case MemoComponent:
case SimpleMemoComponent:
return getDisplayName(resolvedType, 'Anonymous');
return (
(elementType && elementType.displayName) ||
(type && type.displayName) ||
getDisplayName(resolvedType, 'Anonymous')
);
case SuspenseComponent:
return 'Suspense';
case LegacyHiddenComponent:
Expand Down
71 changes: 63 additions & 8 deletions packages/react-reconciler/src/__tests__/ReactMemo-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -498,11 +498,66 @@ describe('memo', () => {
});
});

it('should fall back to showing something meaningful if no displayName or name are present', () => {
const MemoComponent = React.memo(props => <div {...props} />);
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 ' +
'`Memo`, but its value is `undefined`.',
// There's no component stack in this warning because the inner function is anonymous.
// If we wanted to support this (for the Error frames / source location)
// we could do this by updating ReactComponentStackFrame.
{withoutStack: true},
);
});

it('should honor a displayName if set on the inner component in warnings', () => {
function Component(props) {
return <div {...props} />;
}
Component.displayName = 'Inner';
const MemoComponent = React.memo(Component);
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 ' +
'`Inner`, but its value is `undefined`.\n' +
' in Inner (at **)',
);
});

it('should honor a displayName if set on the memo wrapper in warnings', () => {
const MemoComponent = React.memo(function Component(props) {
return <div {...props} />;
});
MemoComponent.displayName = 'Foo';
MemoComponent.displayName = 'Outer';
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 ' +
'`Outer`, but its value is `undefined`.\n' +
' in Component (at **)',
);
});

it('should pass displayName to an anonymous inner component so it shows up in component stacks', () => {
const MemoComponent = React.memo(props => {
return <div {...props} />;
});
MemoComponent.displayName = 'Memo';
MemoComponent.propTypes = {
required: PropTypes.string.isRequired,
};
Expand All @@ -511,19 +566,19 @@ 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 **)',
'`Memo`, but its value is `undefined`.\n' +
' in Memo (at **)',
);
});

it('should honor a inner displayName if set on the wrapped function', () => {
it('should honor a outer displayName when wrapped component and memo component set displayName at the same time.', () => {
function Component(props) {
return <div {...props} />;
}
Component.displayName = 'Foo';
Component.displayName = 'Inner';

const MemoComponent = React.memo(Component);
MemoComponent.displayName = 'Bar';
MemoComponent.displayName = 'Outer';
MemoComponent.propTypes = {
required: PropTypes.string.isRequired,
};
Expand All @@ -532,8 +587,8 @@ 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 **)',
'`Outer`, but its value is `undefined`.\n' +
' in Inner (at **)',
);
});
}
Expand Down
10 changes: 9 additions & 1 deletion packages/react/src/ReactForwardRef.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,15 @@ export function forwardRef<Props, ElementType: React$ElementType>(
},
set: function(name) {
ownName = name;
if (render.displayName == null) {

// The inner component shouldn't inherit this display name in most cases,
// because the component may be used elsewhere.
// But it's nice for anonymous functions to inherit the name,
// so that our component-stack generation logic will display their frames.
// An anonymous function generally suggests a pattern like:
// React.forwardRef((props, ref) => {...});
// This kind of inner function is not used elsewhere so the side effect is okay.
if (!render.name && !render.displayName) {
render.displayName = name;
}
},
Expand Down
10 changes: 9 additions & 1 deletion packages/react/src/ReactMemo.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,15 @@ export function memo<Props>(
},
set: function(name) {
ownName = name;
if (type.displayName == null) {

// The inner component shouldn't inherit this display name in most cases,
// because the component may be used elsewhere.
// But it's nice for anonymous functions to inherit the name,
// so that our component-stack generation logic will display their frames.
// An anonymous function generally suggests a pattern like:
// React.memo((props) => {...});
// This kind of inner function is not used elsewhere so the side effect is okay.
if (!type.name && !type.displayName) {
type.displayName = name;
}
},
Expand Down
99 changes: 92 additions & 7 deletions packages/react/src/__tests__/forwardRef-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -217,14 +217,43 @@ describe('forwardRef', () => {
);
});

it('should honor a displayName if set on the forwardRef wrapper in warnings', () => {
it('should fall back to showing something meaningful if no displayName or name are present', () => {
const Component = props => <div {...props} />;

const RefForwardingComponent = React.forwardRef((props, ref) => (
<Component {...props} forwardedRef={ref} />
));

RefForwardingComponent.displayName = 'Foo';
RefForwardingComponent.propTypes = {
optional: PropTypes.string,
required: PropTypes.string.isRequired,
};

RefForwardingComponent.defaultProps = {
optional: 'default',
};

const ref = React.createRef();

expect(() =>
ReactNoop.render(<RefForwardingComponent ref={ref} optional="foo" />),
).toErrorDev(
'Warning: Failed prop type: The prop `required` is marked as required in ' +
'`ForwardRef`, but its value is `undefined`.',
// There's no component stack in this warning because the inner function is anonymous.
// If we wanted to support this (for the Error frames / source location)
// we could do this by updating ReactComponentStackFrame.
{withoutStack: true},
);
});

it('should honor a displayName if set on the forwardRef wrapper in warnings', () => {
const Component = props => <div {...props} />;

const RefForwardingComponent = React.forwardRef(function Inner(props, ref) {
<Component {...props} forwardedRef={ref} />;
});
RefForwardingComponent.displayName = 'Custom';

RefForwardingComponent.propTypes = {
optional: PropTypes.string,
Expand All @@ -241,17 +270,73 @@ describe('forwardRef', () => {
ReactNoop.render(<RefForwardingComponent ref={ref} 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 **)',
'`Custom`, but its value is `undefined`.\n' +
' in Inner (at **)',
);
});

it('should pass displayName to an anonymous inner component so it shows up in component stacks', () => {
const Component = props => <div {...props} />;

const RefForwardingComponent = React.forwardRef((props, ref) => (
<Component {...props} forwardedRef={ref} />
));
RefForwardingComponent.displayName = 'Custom';

RefForwardingComponent.propTypes = {
optional: PropTypes.string,
required: PropTypes.string.isRequired,
};

RefForwardingComponent.defaultProps = {
optional: 'default',
};

const ref = React.createRef();

expect(() =>
ReactNoop.render(<RefForwardingComponent ref={ref} optional="foo" />),
).toErrorDev(
'Warning: Failed prop type: The prop `required` is marked as required in ' +
'`Custom`, but its value is `undefined`.\n' +
' in Custom (at **)',
);
});

it('should honor a displayName in stacks if set on the inner function', () => {
const Component = props => <div {...props} />;

const inner = (props, ref) => <Component {...props} forwardedRef={ref} />;
inner.displayName = 'Foo';
inner.displayName = 'Inner';
const RefForwardingComponent = React.forwardRef(inner);

RefForwardingComponent.propTypes = {
optional: PropTypes.string,
required: PropTypes.string.isRequired,
};

RefForwardingComponent.defaultProps = {
optional: 'default',
};

const ref = React.createRef();

expect(() =>
ReactNoop.render(<RefForwardingComponent ref={ref} optional="foo" />),
).toErrorDev(
'Warning: Failed prop type: The prop `required` is marked as required in ' +
'`ForwardRef(Inner)`, but its value is `undefined`.\n' +
' in Inner (at **)',
);
});

it('should honor a outer displayName when wrapped component and memo component set displayName at the same time.', () => {
const Component = props => <div {...props} />;

const inner = (props, ref) => <Component {...props} forwardedRef={ref} />;
inner.displayName = 'Inner';
const RefForwardingComponent = React.forwardRef(inner);
RefForwardingComponent.displayName = 'Outer';

RefForwardingComponent.propTypes = {
optional: PropTypes.string,
Expand All @@ -268,8 +353,8 @@ describe('forwardRef', () => {
ReactNoop.render(<RefForwardingComponent ref={ref} optional="foo" />),
).toErrorDev(
'Warning: Failed prop type: The prop `required` is marked as required in ' +
'`ForwardRef(Foo)`, but its value is `undefined`.\n' +
' in Foo (at **)',
'`Outer`, but its value is `undefined`.\n' +
' in Inner (at **)',
);
});

Expand Down
15 changes: 10 additions & 5 deletions packages/shared/getComponentNameFromType.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,12 @@ function getWrappedName(
innerType: any,
wrapperName: string,
): string {
const displayName = (outerType: any).displayName;
if (displayName) {
return displayName;
}
const functionName = innerType.displayName || innerType.name || '';
return (
(outerType: any).displayName ||
(functionName !== '' ? `${wrapperName}(${functionName})` : wrapperName)
);
return functionName !== '' ? `${wrapperName}(${functionName})` : wrapperName;
}

// Keep in sync with react-reconciler/getComponentNameFromFiber
Expand Down Expand Up @@ -90,7 +91,11 @@ export default function getComponentNameFromType(type: mixed): string | null {
case REACT_FORWARD_REF_TYPE:
return getWrappedName(type, type.render, 'ForwardRef');
case REACT_MEMO_TYPE:
return getComponentNameFromType(type.type);
const outerName = (type: any).displayName || null;
if (outerName !== null) {
return outerName;
}
return getComponentNameFromType(type.type) || 'Memo';
case REACT_LAZY_TYPE: {
const lazyComponent: LazyComponent<any, any> = (type: any);
const payload = lazyComponent._payload;
Expand Down

0 comments on commit d1542de

Please sign in to comment.