Fix devtools displaying Anonymous for memo of ref-forwarding components #17274

wsmd · 2019-11-05T01:17:02Z

Resolves #16722

This PR addresses the following issues:

react-devtools-shared not displaying the proper displayName for memo(forwardRef(C))
~~react-is's typeOf not accounting for forwardRef components/functions. It's currently accounting for forwardRef elements only.~~ (Fixed in Fix react-is memo and lazy type checks #17278 🎉)

Problem: devtools not displaying the proper displayName for `memo(forwardRef(C))`

I encountered an issue with React dev-tools where it no longer infers the proper display name of a component wrapped with both memo and forwardRef. The display name in this case is shown as "Anonymous".

Consider the following example:

const HelloWorld = () => <div>Hello, World!</div>;

const RefForwarding = React.forwardRef(HelloWorld);
const MemoizedRefForwarding = React.memo(RefForwarding);

When I render <MemoizedRefForwarding />, the React devtools shows the following:

Notice how the Memo component is displayed as "Anonymous"

What I would expect is the following (taken after using the attempted fix):

The Memo component is now properly displayed as "MemoizedRefForwarding"

Solution

I traced down the problem and found that react-devtools-shared has a shared helper function getDisplayName that expects a function. This getDisplayName function is being used by getDisplayNameForFiber in backend/renderer.js.

While this works for most fibers with their type as the component function itself, forwardRef and memo have a slightly different type.

In the case of memo(forwardRef(C)), the type of the memo element is the forwardRef object, not the component function itself (which getDisplayName expects), so we need to extract that type off of forwardRef.

I solved this problem by adding a resolveFiberType that will attempt to "resolve" the type value, if the type of the fiber node happens to be an object, before passing it to getDisplayName. Since this is doing recursion it will cover memo(C), forwardRef(C) , and memo(forwardRef(C)).

I'm not quite sure if that's the best possible way to approach this problem since it's my first time contributing to the React codebase 😬 so I'm not super familiar with it yet. I'd love some direction if there's a better way of handling this.

Problem (2) - `react-is`'s `typeOf` not accounting for `forwardRef` HOCs

Outdated Comment (Issue was resolved in in #17278)

To fix the devtools issue mentioned above, I wanted to use `react-is`'s helpers. Specifically, `isMemo`, and `isForwardRef`.

Looks like there's an outstanding issue with both of ReactIs.isForwardRef and ReactIs.typeOf where they don't respect a forwardRef HOC. It currently only works with elements created from a fowardRef component.

const RefForwardingComponent = React.forwardRef((props, ref) => null);

ReactIs.typeOf(RefForwardingComponent) // undefined
ReactIs.isForwardRef(RefForwardingComponent) // false

ReactIs.typeOf(<RefForwardingComponent />) // ReactIs.ForwardRef
ReactIs.isForwardRef(<RefForwardingComponent />) // true

I'm not sure if this is by design or intentional, but it does seem inconsistent with how isMemo works.

I made a 1-line change to react-is in this PR so that it respects forwardRef HOCs (the actual components/functions) not just the elements. Are there any pitfalls to know?

const RefForwardingComponent = React.forwardRef((props, ref) => null);
 
// notice how I'm not passing an actual element
ReactIs.typeOf(RefForwardingComponent) // ReactIs.ForwardRef
ReactIs.isForwardRef(RefForwardingComponent) // true

…mponent))

codesandbox-ci · 2019-11-05T01:18:50Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 6bcd757:

Sandbox	Source
focused-wescoff-5cluy	Configuration
React forwardRef and memo combo	Issue #16722

sizebot · 2019-11-05T01:21:29Z

Size changes (experimental)

No significant bundle size changes to report.

Generated by 🚫 dangerJS against 6bcd757

sizebot · 2019-11-05T01:21:54Z

Size changes (stable)

No significant bundle size changes to report.

Generated by 🚫 dangerJS against 6bcd757

packages/react-is/src/__tests__/ReactIs-test.js

…f memo recursively Resolving the fiber type of memo recursively before passing it to getDisplayName will prevent it from displaying "Anonymous" as displayName for components wrapped with both memo and forwardRef: memo(forwardRef(Component))

wsmd · 2019-11-05T21:37:07Z

packages/react-devtools-shared/src/backend/renderer.js

+    // This is to support lazy components with a Promise as the type.
+    // see https://github.com/facebook/react/pull/13397
+    if (typeof type.then === 'function') {
+      return type._reactResult;
+    }


I couldn't find any references to _reactResult in the codebase. Is this still relevant? Is it safe to remove?

I think it has sense been renamed to _result now that you mention it.

Someone can follow up with that in a separate PR.

cc @acdlite

I don't think this codepath is even used by DevTools anymore. By the time React DevTools gets called for the lazy component, it just gets the e.g. function.

bvaughn · 2019-11-05T15:17:22Z

packages/react-devtools-shared/src/backend/renderer.js

+    if (typeof type.then === 'function') {
+      return type._reactResult;
+    }
+    if (isForwardRef(type)) {


I don't think we should use react-is for this. Partly because the renderer already knows enough information to make this determination using the Fiber's tag and partly because react-is is meant to work with React elements and the renderer has a Fiber with a "type" (which is not an element).

Edit: This has already been addressed.

bvaughn · 2019-11-05T15:23:45Z

packages/react-devtools-shared/src/backend/renderer.js

+    if (isMemo(type)) {
+      return resolveFiberType(type.type);
+    }
+    return type;


Would be nice to add a new test or two to DevTools store-test for the types that were showing up as anonymous before that no longer are, to guard against a regression.

I can add that after merging the fix though :)

bvaughn · 2019-11-05T15:25:57Z

packages/react-is/src/__tests__/ReactIs-test.js

    const RefForwardingComponent = React.forwardRef((props, ref) => null);
+    expect(ReactIs.typeOf(RefForwardingComponent)).toBe(ReactIs.ForwardRef);
    expect(ReactIs.typeOf(<RefForwardingComponent />)).toBe(ReactIs.ForwardRef);
+    expect(ReactIs.isForwardRef(RefForwardingComponent)).toBe(true);


Hm. This change seems to mirror the current behavior of memo components, so I understand it from that aspect, but I don't think it's right. The typeOf method is meant to look at React element types (e.g. <MyComponent /> or React.createElement(MyComponent)) not component types (e.g. MyComponent).

I think the one exception for this that makes sense may be portals, since the value returned by React.createPortal() is meant to be rendered directly (and not wrapped in an element).

I'm going to file a separate PR for this particular issue since I don't think it's strictly related to this PR: #17278

packages/react-is/src/__tests__/ReactIs-test.js

bvaughn · 2019-11-05T21:34:59Z

packages/react-devtools-shared/src/backend/renderer.js

          return elementType.displayName;
        } else {
-          return getDisplayName(type, 'Anonymous');
+          return getDisplayName(resolvedType, 'Anonymous');


I had been thinking of suggesting a different approach that just kind of localized the check:

diff --git a/packages/react-devtools-shared/src/backend/renderer.js b/packages/react-devtools-shared/src/backend/renderer.js index 761c78935..284183f9f 100644 --- a/packages/react-devtools-shared/src/backend/renderer.js +++ b/packages/react-devtools-shared/src/backend/renderer.js @@ -365,6 +365,9 @@ export function getInternalReactConstants( case SimpleMemoComponent: if (elementType.displayName) { return elementType.displayName; + } else if (type.type != null && type.type.render != null) { + // Special case of a forwardRef() nested in a memo() + return getDisplayName(type.type.render, 'Anonymous'); } else { return getDisplayName(type, 'Anonymous'); }

I don't know that I really have a preference between the two approaches I guess. 😄 Yours is fine~

Tests added #17283

bvaughn · 2019-11-05T21:48:41Z

packages/react-devtools-shared/src/backend/renderer.js

+    // This is to support lazy components with a Promise as the type.
+    // see https://github.com/facebook/react/pull/13397
+    if (typeof type.then === 'function') {
+      return type._reactResult;
+    }


I think it has sense been renamed to _result now that you mention it.

Someone can follow up with that in a separate PR.

cc @acdlite

wsmd added 2 commits November 3, 2019 22:50

[react-is] return correct typeOf value of forwardRef

6f2849e

[react-devtools-shared] use correct displayName of memo(forwardRef(Co…

04341d4

…mponent))

facebook-github-bot added the CLA Signed label Nov 5, 2019

wsmd changed the title ~~Fix devtools displaying Anonymous for memo of forwardRef components~~ Fix devtools displaying Anonymous for memo of ref-forwarding components Nov 5, 2019

wsmd force-pushed the fix-react-is-and-devtools-forwardRef branch from 0591323 to fb63b81 Compare November 5, 2019 03:05

wsmd commented Nov 5, 2019

View reviewed changes

packages/react-is/src/__tests__/ReactIs-test.js Outdated Show resolved Hide resolved

wsmd force-pushed the fix-react-is-and-devtools-forwardRef branch from fb63b81 to c55be00 Compare November 5, 2019 03:54

wsmd mentioned this pull request Nov 5, 2019

Fix react-is memo and lazy type checks #17278

Merged

wsmd added 2 commits November 5, 2019 16:03

Merge branch 'master' into fix-react-is-and-devtools-forwardRef

5429c6e

rework resolveFiberType

6bcd757

wsmd commented Nov 5, 2019

View reviewed changes

bvaughn approved these changes Nov 5, 2019

View reviewed changes

bvaughn merged commit 4f02c93 into facebook:master Nov 5, 2019

bvaughn mentioned this pull request Nov 5, 2019

Added a DevTools store test for component names #17283

Merged

BPScott mentioned this pull request Jan 17, 2020

react-testing does not print name of component wrapped in memo correctly Shopify/quilt#1237

Open

gndplayground mentioned this pull request May 21, 2020

[SvgIcon] Add displayName in react-devtools mui/material-ui#21134

Merged

1 task

KevinGhadyani-minted mentioned this pull request May 28, 2020

Some memo components displaying as Anonymous bvaughn/react-devtools-experimental#372

Closed

This was referenced Mar 24, 2021

Fix false positives for display-name jsx-eslint/eslint-plugin-react#2399

Merged

display-name has false positive on nested use of memo and forwardRef jsx-eslint/eslint-plugin-react#2954

Closed

Fix devtools displaying Anonymous for memo of ref-forwarding components #17274

Fix devtools displaying Anonymous for memo of ref-forwarding components #17274

Uh oh!

Conversation

wsmd commented Nov 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem: devtools not displaying the proper displayName for memo(forwardRef(C))

Solution

Problem (2) - react-is's typeOf not accounting for forwardRef HOCs

Uh oh!

codesandbox-ci bot commented Nov 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sizebot commented Nov 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Size changes (experimental)

Uh oh!

sizebot commented Nov 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Size changes (stable)

Uh oh!

Uh oh!

wsmd Nov 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

wsmd commented Nov 5, 2019 •

edited

Loading

Problem: devtools not displaying the proper displayName for `memo(forwardRef(C))`

Problem (2) - `react-is`'s `typeOf` not accounting for `forwardRef` HOCs

codesandbox-ci bot commented Nov 5, 2019 •

edited

Loading

sizebot commented Nov 5, 2019 •

edited

Loading

sizebot commented Nov 5, 2019 •

edited

Loading

wsmd Nov 5, 2019 •

edited

Loading