Refactored display name logic for memo to be consistent with forwardRef

facebook · Apr 29, 2021 · 3af3f6a · 3af3f6a
1 parent 546c243
commit 3af3f6a
Show file tree

Hide file tree

Showing 8 changed files with 133 additions and 67 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
@@ -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>
-`;
diff --git 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 = () => (
       <React.Fragment>
         <MyComponent />
@@ -891,6 +902,8 @@ describe('Store', () => {
         <MemoizedFakeHigherOrderComponent />
         <ForwardRefFakeHigherOrderComponent />
         <React.unstable_Cache />
+        <MemoizedFakeHigherOrderComponentWithDisplayNameOverride />
+        <ForwardRefFakeHigherOrderComponentWithDisplayNameOverride />
       </React.Fragment>
     );
 
@@ -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', () => {

diff --git 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
@@ -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 <div {...props} />;
-      });
-      MemoComponent.displayName = 'Foo';
+    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,
       };
@@ -511,19 +508,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`.',
+        // 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.', () => {
-      function Component(props) {
+    it('should honor a displayName if set on the memo wrapper in warnings', () => {
+      const MemoComponent = React.memo(function Component(props) {
         return <div {...props} />;
-      }
-      Component.displayName = 'Foo';
-
-      const MemoComponent = React.memo(Component);
-      MemoComponent.displayName = 'Bar';
+      });
+      MemoComponent.displayName = 'Outer';
       MemoComponent.propTypes = {
         required: PropTypes.string.isRequired,
       };
@@ -532,21 +529,19 @@ describe('memo', () => {
         ReactNoop.render(<MemoComponent optional="foo" />),
       ).toErrorDev(
         'Warning: Failed prop type: The prop `required` is marked as required in ' +
-          '`Bar`, but its value is `undefined`.\n' +
-          '    in Bar (at **)',
+          '`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 <div {...props} />;
       }
-      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 +550,8 @@ describe('memo', () => {
         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 **)',
+          '`Outer`, but its value is `undefined`.\n' +
+          '    in Inner (at **)',
       );
     });
   }

diff --git a/packages/react/src/ReactForwardRef.js b/packages/react/src/ReactForwardRef.js
@@ -57,9 +57,6 @@ export function forwardRef<Props, ElementType: React$ElementType>(
       },
       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
@@ -36,15 +36,7 @@ export function memo<Props>(
         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
@@ -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((props, ref) => (
+      <Component {...props} forwardedRef={ref} />
+    ));
+    RefForwardingComponent.displayName = 'Outer';
 
     RefForwardingComponent.propTypes = {
       optional: PropTypes.string,
@@ -241,17 +270,48 @@ 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 **)',
+        '`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},
     );
   });
 
   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,
@@ -268,8 +328,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 **)',
     );
   });
 

diff --git 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<any, any> = (type: any);
         const payload = lazyComponent._payload;