From 81fc1caf504e6bc2552fa49fcf592ece6bbcee7a Mon Sep 17 00:00:00 2001 From: iChenLei <2470828450@qq.com> Date: Tue, 26 Jan 2021 11:30:39 +0800 Subject: [PATCH 1/3] allow react memo component can set displayName multiple times --- .../src/__tests__/ReactMemo-test.js | 29 +++++++++++++++++-- packages/react/src/ReactMemo.js | 9 ++++-- 2 files changed, 33 insertions(+), 5 deletions(-) diff --git a/packages/react-reconciler/src/__tests__/ReactMemo-test.js b/packages/react-reconciler/src/__tests__/ReactMemo-test.js index 5be63d65a37..18fa2a55668 100644 --- a/packages/react-reconciler/src/__tests__/ReactMemo-test.js +++ b/packages/react-reconciler/src/__tests__/ReactMemo-test.js @@ -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
; } @@ -532,8 +532,31 @@ describe('memo', () => { ReactNoop.render(), ).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
; + } + 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(), + ).toErrorDev( + 'Warning: Failed prop type: The prop `required` is marked as required in ' + + '`MemoComp03`, but its value is `undefined`.\n' + + ' in MemoComp03 (at **)', ); }); } diff --git a/packages/react/src/ReactMemo.js b/packages/react/src/ReactMemo.js index 2531c1a1879..796ba2e393c 100644 --- a/packages/react/src/ReactMemo.js +++ b/packages/react/src/ReactMemo.js @@ -36,9 +36,14 @@ export function memo( 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, + ); } }, }); From b668b124f714f2440cbc851eeda9c80d934674a2 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Thu, 29 Apr 2021 17:57:42 -0400 Subject: [PATCH 2/3] Unify React.memo and React.forwardRef display name logic --- .../__snapshots__/store-test.js.snap | 17 ----- .../src/__tests__/store-test.js | 32 +++++++- .../src/backend/renderer.js | 8 +- .../src/__tests__/ReactMemo-test.js | 54 +++++++++----- packages/react/src/ReactForwardRef.js | 3 - packages/react/src/ReactMemo.js | 10 +-- .../react/src/__tests__/forwardRef-test.js | 74 +++++++++++++++++-- packages/shared/getComponentNameFromType.js | 15 ++-- 8 files changed, 149 insertions(+), 64 deletions(-) diff --git a/packages/react-devtools-shared/src/__tests__/__snapshots__/store-test.js.snap b/packages/react-devtools-shared/src/__tests__/__snapshots__/store-test.js.snap index 5ff48f317f4..eedcaa33fc0 100644 --- a/packages/react-devtools-shared/src/__tests__/__snapshots__/store-test.js.snap +++ b/packages/react-devtools-shared/src/__tests__/__snapshots__/store-test.js.snap @@ -670,20 +670,3 @@ exports[`Store should properly serialize non-string key values: 1: mount 1`] = ` [root] `; - -exports[`Store should show the right display names for special component types 1`] = ` -[root] - ▾ - - [ForwardRef] - ▾ [ForwardRef] - - [ForwardRef] - [Memo] - ▾ [Memo] - [ForwardRef] - [withFoo][withBar] - [Memo][withFoo][withBar] - [ForwardRef][withFoo][withBar] - -`; diff --git a/packages/react-devtools-shared/src/__tests__/store-test.js b/packages/react-devtools-shared/src/__tests__/store-test.js index f971946b686..b63b8f088e8 100644 --- a/packages/react-devtools-shared/src/__tests__/store-test.js +++ b/packages/react-devtools-shared/src/__tests__/store-test.js @@ -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 = () => ( @@ -891,6 +902,8 @@ describe('Store', () => { + + ); @@ -904,7 +917,24 @@ describe('Store', () => { // Render again after it resolves act(() => ReactDOM.render(, container)); - expect(store).toMatchSnapshot(); + expect(store).toMatchInlineSnapshot(` + [root] + ▾ + + [ForwardRef] + ▾ [ForwardRef] + + [ForwardRef] + [Memo] + ▾ [Memo] + [ForwardRef] + [withFoo][withBar] + [Memo][withFoo][withBar] + [ForwardRef][withFoo][withBar] + + [Memo] + [ForwardRef] + `); }); describe('Lazy', () => { diff --git a/packages/react-devtools-shared/src/backend/renderer.js b/packages/react-devtools-shared/src/backend/renderer.js index a45f510ff23..18fa9d7efdd 100644 --- a/packages/react-devtools-shared/src/backend/renderer.js +++ b/packages/react-devtools-shared/src/backend/renderer.js @@ -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) { @@ -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: diff --git a/packages/react-reconciler/src/__tests__/ReactMemo-test.js b/packages/react-reconciler/src/__tests__/ReactMemo-test.js index 18fa2a55668..910dbee7bed 100644 --- a/packages/react-reconciler/src/__tests__/ReactMemo-test.js +++ b/packages/react-reconciler/src/__tests__/ReactMemo-test.js @@ -498,11 +498,8 @@ describe('memo', () => { }); }); - it('should honor a displayName if set on the memo wrapper in warnings', () => { - const MemoComponent = React.memo(function Component(props) { - return
; - }); - MemoComponent.displayName = 'Foo'; + it('should fall back to showing something meaningful if no displayName or name are present', () => { + const MemoComponent = React.memo(props =>
); MemoComponent.propTypes = { required: PropTypes.string.isRequired, }; @@ -511,19 +508,20 @@ describe('memo', () => { ReactNoop.render(), ).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`.', + // 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 outter displayName when wrapped component and memo component set displayName at the same time.', () => { + it('should honor a displayName if set on the inner component in warnings', () => { function Component(props) { return
; } - Component.displayName = 'Foo'; - + Component.displayName = 'Inner'; const MemoComponent = React.memo(Component); - MemoComponent.displayName = 'Bar'; MemoComponent.propTypes = { required: PropTypes.string.isRequired, }; @@ -532,21 +530,37 @@ describe('memo', () => { ReactNoop.render(), ).toErrorDev( 'Warning: Failed prop type: The prop `required` is marked as required in ' + - '`Bar`, but its value is `undefined`.\n' + - ' in Bar (at **)', + '`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
; + }); + MemoComponent.displayName = 'Outer'; + MemoComponent.propTypes = { + required: PropTypes.string.isRequired, + }; + + expect(() => + ReactNoop.render(), + ).toErrorDev( + 'Warning: Failed prop type: The prop `required` is marked as required in ' + + '`Outer`, but its value is `undefined`.\n' + + ' in Component (at **)', ); }); - it('can set react memo component displayName multiple times', () => { + it('should honor a outer displayName when wrapped component and memo component set displayName at the same time.', () => { function Component(props) { return
; } - Component.displayName = 'Foo'; + Component.displayName = 'Inner'; const MemoComponent = React.memo(Component); - MemoComponent.displayName = 'MemoComp01'; - MemoComponent.displayName = 'MemoComp02'; - MemoComponent.displayName = 'MemoComp03'; + MemoComponent.displayName = 'Outer'; MemoComponent.propTypes = { required: PropTypes.string.isRequired, }; @@ -555,8 +569,8 @@ describe('memo', () => { ReactNoop.render(), ).toErrorDev( 'Warning: Failed prop type: The prop `required` is marked as required in ' + - '`MemoComp03`, but its value is `undefined`.\n' + - ' in MemoComp03 (at **)', + '`Outer`, but its value is `undefined`.\n' + + ' in Inner (at **)', ); }); } diff --git a/packages/react/src/ReactForwardRef.js b/packages/react/src/ReactForwardRef.js index d76310ac74c..7bb46419ccd 100644 --- a/packages/react/src/ReactForwardRef.js +++ b/packages/react/src/ReactForwardRef.js @@ -57,9 +57,6 @@ export function forwardRef( }, set: function(name) { ownName = name; - if (render.displayName == null) { - render.displayName = name; - } }, }); } diff --git a/packages/react/src/ReactMemo.js b/packages/react/src/ReactMemo.js index 796ba2e393c..618dafe6a7a 100644 --- a/packages/react/src/ReactMemo.js +++ b/packages/react/src/ReactMemo.js @@ -36,15 +36,7 @@ export function memo( return ownName; }, set: function(name) { - 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, - ); - } + ownName = name; }, }); } diff --git a/packages/react/src/__tests__/forwardRef-test.js b/packages/react/src/__tests__/forwardRef-test.js index 4dfb4a3f9cd..b995cd8d804 100644 --- a/packages/react/src/__tests__/forwardRef-test.js +++ b/packages/react/src/__tests__/forwardRef-test.js @@ -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 =>
; const RefForwardingComponent = React.forwardRef((props, ref) => ( )); - RefForwardingComponent.displayName = 'Foo'; + RefForwardingComponent.propTypes = { + optional: PropTypes.string, + required: PropTypes.string.isRequired, + }; + + RefForwardingComponent.defaultProps = { + optional: 'default', + }; + + const ref = React.createRef(); + + expect(() => + ReactNoop.render(), + ).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 =>
; + + const RefForwardingComponent = React.forwardRef((props, ref) => ( + + )); + RefForwardingComponent.displayName = 'Outer'; RefForwardingComponent.propTypes = { optional: PropTypes.string, @@ -241,8 +270,11 @@ describe('forwardRef', () => { ReactNoop.render(), ).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`.', + // 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}, ); }); @@ -250,8 +282,36 @@ describe('forwardRef', () => { const Component = props =>
; const inner = (props, 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(), + ).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 =>
; + + const inner = (props, ref) => ; + inner.displayName = 'Inner'; const RefForwardingComponent = React.forwardRef(inner); + RefForwardingComponent.displayName = 'Outer'; RefForwardingComponent.propTypes = { optional: PropTypes.string, @@ -268,8 +328,8 @@ describe('forwardRef', () => { ReactNoop.render(), ).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 **)', ); }); diff --git a/packages/shared/getComponentNameFromType.js b/packages/shared/getComponentNameFromType.js index f0f51d852f8..7428f8c11d7 100644 --- a/packages/shared/getComponentNameFromType.js +++ b/packages/shared/getComponentNameFromType.js @@ -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 @@ -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 = (type: any); const payload = lazyComponent._payload; From a6f42c72d22b8e036ac73ba3499eba95c6921084 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Tue, 4 May 2021 10:11:01 -0400 Subject: [PATCH 3/3] Only anonymous inner functions of memo/forwardRef inherit displayName --- .../src/__tests__/ReactMemo-test.js | 18 +++++++++ packages/react/src/ReactForwardRef.js | 11 ++++++ packages/react/src/ReactMemo.js | 11 ++++++ .../react/src/__tests__/forwardRef-test.js | 37 ++++++++++++++++--- 4 files changed, 71 insertions(+), 6 deletions(-) diff --git a/packages/react-reconciler/src/__tests__/ReactMemo-test.js b/packages/react-reconciler/src/__tests__/ReactMemo-test.js index 910dbee7bed..89bf6e2cd4e 100644 --- a/packages/react-reconciler/src/__tests__/ReactMemo-test.js +++ b/packages/react-reconciler/src/__tests__/ReactMemo-test.js @@ -553,6 +553,24 @@ describe('memo', () => { ); }); + it('should pass displayName to an anonymous inner component so it shows up in component stacks', () => { + const MemoComponent = React.memo(props => { + return
; + }); + MemoComponent.displayName = 'Memo'; + MemoComponent.propTypes = { + required: PropTypes.string.isRequired, + }; + + expect(() => + ReactNoop.render(), + ).toErrorDev( + 'Warning: Failed prop type: The prop `required` is marked as required in ' + + '`Memo`, but its value is `undefined`.\n' + + ' in Memo (at **)', + ); + }); + it('should honor a outer displayName when wrapped component and memo component set displayName at the same time.', () => { function Component(props) { return
; diff --git a/packages/react/src/ReactForwardRef.js b/packages/react/src/ReactForwardRef.js index 7bb46419ccd..796571d51f6 100644 --- a/packages/react/src/ReactForwardRef.js +++ b/packages/react/src/ReactForwardRef.js @@ -57,6 +57,17 @@ export function forwardRef( }, set: function(name) { ownName = name; + + // 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; + } }, }); } diff --git a/packages/react/src/ReactMemo.js b/packages/react/src/ReactMemo.js index 618dafe6a7a..d742948bf5e 100644 --- a/packages/react/src/ReactMemo.js +++ b/packages/react/src/ReactMemo.js @@ -37,6 +37,17 @@ export function memo( }, set: function(name) { ownName = name; + + // 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; + } }, }); } diff --git a/packages/react/src/__tests__/forwardRef-test.js b/packages/react/src/__tests__/forwardRef-test.js index b995cd8d804..0c2ee9aa4ea 100644 --- a/packages/react/src/__tests__/forwardRef-test.js +++ b/packages/react/src/__tests__/forwardRef-test.js @@ -250,10 +250,38 @@ describe('forwardRef', () => { it('should honor a displayName if set on the forwardRef wrapper in warnings', () => { const Component = props =>
; + const RefForwardingComponent = React.forwardRef(function Inner(props, ref) { + ; + }); + RefForwardingComponent.displayName = 'Custom'; + + RefForwardingComponent.propTypes = { + optional: PropTypes.string, + required: PropTypes.string.isRequired, + }; + + RefForwardingComponent.defaultProps = { + optional: 'default', + }; + + const ref = React.createRef(); + + expect(() => + ReactNoop.render(), + ).toErrorDev( + 'Warning: Failed prop type: The prop `required` is marked as required in ' + + '`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 =>
; + const RefForwardingComponent = React.forwardRef((props, ref) => ( )); - RefForwardingComponent.displayName = 'Outer'; + RefForwardingComponent.displayName = 'Custom'; RefForwardingComponent.propTypes = { optional: PropTypes.string, @@ -270,11 +298,8 @@ describe('forwardRef', () => { ReactNoop.render(), ).toErrorDev( 'Warning: Failed prop type: The prop `required` is marked as required in ' + - '`Outer`, 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}, + '`Custom`, but its value is `undefined`.\n' + + ' in Custom (at **)', ); });