Match react-devtools display name behavior for React.memo #15313

eps1lon · 2019-04-03T22:19:57Z

// anonymous render
React.memo(() => null)
- Component
+ Memo
// named render
React.memo(function Inner() { return null })
- Inner
+ Memo(Inner)

Showcase to verify matching behavior with react-devtools: https://codesandbox.io/s/lpx7k7wv4z

acdlite

Can we make getComponentName accept a fiber instead of an element type? Then we don't have to redo all the logic that we did to create the fiber in the first place.

acdlite · 2019-04-03T22:36:25Z

Oh haha I thought you were @bvaughn. Sorry :D If you want to change the signature of getComponentType then that's fine, but what you have is fine, too.

acdlite · 2019-04-03T22:37:06Z

packages/react-reconciler/src/__tests__/ReactLazy-test.internal.js

    Add.defaultProps = {
      innerWithDefault: 42,
    };
+    Add.displayName = 'Add';


Let's leave these tests as-is and write a separate one that tests displayName.

This is only there because the test function matches the display name. Since not every component is a memo component the test would fail.

i.e. this is not there to test displayName but to prevent refactoring the test.

The alternative would be to use a wildcard matcher in

react/packages/react-reconciler/src/__tests__/ReactLazy-test.internal.js

Line 681 in 4aa2241

'Invalid prop `inner` of type `string` supplied to `Add`, expected `number`.',

instead of expecting an exact component name.

Let's leave these tests as-is and write a separate one that tests displayName.

Added an explicit test for memo and displayName in 012af53.

I could not use displayName in this test but then I would have to change the test to match * as the displayName (some components have the memo wrapper some don't). I don't think that's a good idea.

Not sure what else I should do here.

eps1lon · 2019-04-03T22:39:12Z

I think the devtools get the name from the fiber as well. If we can use a fiber instead we can we get rid of the component name fork in react-devtools as well?

eps1lon · 2019-04-03T22:41:07Z

packages/shared/getComponentName.js

  wrapperName: string,
 ): string {
-  const functionName = innerType.displayName || innerType.name || '';
+  const innerName = getComponentName(innerType) || '';


This may be a bit overkill for render functions of forwardRef. We could extract the logic of innerName to the call-site of getComponentName.

bvaughn · 2019-04-11T15:19:44Z

If we can use a fiber instead we can we get rid of the component name fork in react-devtools as well?

Not any time soon, since DevTools source lives in a different repo and (more importantly) needs to support multiple versions of React- unlike this help function, which only needs to support the current version at any given time.

eps1lon · 2019-04-14T11:26:49Z

packages/react-reconciler/src/__tests__/ReactIncrementalPerf-test.internal.js

      const MemoFoo = React.memo(function Foo() {
        return <div />;
      });
+      MemoFoo.displayName = 'Foo';


beginFiberMark will be called with the initial Fiber. However, in updateMemoComponent we will upgrade the Fiber to a SimpleMemoComponent at which point the Fiber is indistinguishable for getComponentName from a function component because with mutated the type. endFiberMark will receive a SimpleMemoComponent and getComponentName assumes it's a plain function component.

Not sure if this is considered a defect (assuming that the fiber type in beginFiberWork is the same we get in endFiberWork). Should we we add something _debugOriginalType field to the Fiber or rather change react-devtools to display Foo rather than Memo(Foo)? ~~Mutating the displayName of the type when upgrading the Fiber might also work.~~ Mutating the display name was a terrible idea.

Any thoughts @bvaughn ?

eps1lon · 2019-04-14T13:32:27Z

packages/react-reconciler/src/ReactDebugFiberPerf.js


+const getFiberName = (fiber: Fiber): string | null => {
+  if (fiber.tag === SimpleMemoComponent) {
+    return getComponentName(fiber.elementType);


Tested

-getComponentName(someFiber.type) +getComponentName(someFiber.elementType)

on all source files in master and no test broke. This may be the better option if other types of Fibers change their type during work i.e. getComponentName should only be called with element types not fiber types (enforceable with types?).

#15638 would remove the necessity for this branch.

sizebot · 2019-08-29T15:09:47Z

No significant bundle size changes to report.

Generated by 🚫 dangerJS against 2c6088e

Airkro · 2019-09-12T05:16:51Z

forwardRef should do the same.

stale · 2020-01-09T20:47:27Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution.

eps1lon · 2020-01-10T16:20:54Z

I'll resolve merge conflicts.

alflennik · 2020-02-27T17:42:07Z

I just ran into this issue and I would love to see this PR make it in! I see that it's been sitting here for some time now.

eps1lon · 2020-02-27T18:49:37Z

I'll resolve the conflicts. Maybe this'll help.

codesandbox-ci · 2020-03-01T17:29:45Z

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 3893791:

Sandbox	Source
hungry-murdock-13ddl	Configuration
React.memo display name in react and react-devtools	PR

sizebot · 2020-03-01T17:32:42Z

No significant bundle size changes to report.

Size changes (experimental)

Generated by 🚫 dangerJS against 3893791

sizebot · 2020-03-01T17:34:04Z

No significant bundle size changes to report.

Size changes (stable)

Generated by 🚫 dangerJS against 3893791

eps1lon · 2020-03-05T23:30:46Z

@bvaughn Could you take a look at this?

eps1lon · 2020-05-25T07:18:14Z

packages/react-reconciler/src/__tests__/ReactMemo-test.js

      ).toErrorDev(
        'Warning: Failed prop type: The prop `required` is marked as required in ' +
-          '`Foo`, but its value is `undefined`.\n' +
+          '`Bar`, but its value is `undefined`.\n' +


This test was added in #18925 but I don't think this should've been the expected behavior @bvaughn.

For React.memo we ignore displayName on the wrapper if we have a displayName on the wrapped component.
For React.forwardRef we honor displayName on the wrapper even if we have displayName on the wrapped component.

I think it is confusing that React.memo and React.forwardRef behave differently with regard to displayName.

stale · 2020-08-23T07:46:48Z

This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated.

stale · 2020-08-30T23:32:32Z

Closing this pull request after a prolonged period of inactivity. If this issue is still present in the latest release, please ask for this pull request to be reopened. Thank you!

facebook-github-bot added the CLA Signed label Apr 3, 2019

eps1lon force-pushed the feat/memo-component-name branch from 48d8e0a to 4aa2241 Compare April 3, 2019 22:23

eps1lon mentioned this pull request Apr 3, 2019

Memoized components should forward displayName #15207

Open

acdlite reviewed Apr 3, 2019

View reviewed changes

eps1lon commented Apr 3, 2019

View reviewed changes

eps1lon force-pushed the feat/memo-component-name branch from 4aa2241 to 012af53 Compare April 9, 2019 18:07

eps1lon commented Apr 14, 2019

View reviewed changes

eps1lon force-pushed the feat/memo-component-name branch from 012af53 to 6d9c392 Compare April 14, 2019 11:33

eps1lon commented Apr 14, 2019

View reviewed changes

eps1lon mentioned this pull request May 13, 2019

refactor(getComponentName): Use element types instead of fiber types #15638

Closed

eps1lon mentioned this pull request Jun 15, 2019

memo components show as Unknown in the timings section of Chrome #15897

Closed

eps1lon force-pushed the feat/memo-component-name branch from 4192198 to 2c6088e Compare August 29, 2019 15:05

stale bot added the Resolution: Stale Automatically closed due to inactivity label Jan 9, 2020

stale bot removed the Resolution: Stale Automatically closed due to inactivity label Jan 10, 2020

eps1lon force-pushed the feat/memo-component-name branch from 2c6088e to de7878e Compare March 1, 2020 17:27

eps1lon force-pushed the feat/memo-component-name branch from de7878e to 8f816bc Compare April 14, 2020 19:54

eps1lon force-pushed the feat/memo-component-name branch from 8f816bc to 3e6f7f8 Compare May 14, 2020 18:16

Match react-devtools display name behavior for React.memo

f30ca94

eps1lon force-pushed the feat/memo-component-name branch from 3e6f7f8 to f30ca94 Compare May 25, 2020 04:33

fix failing test

3893791

eps1lon commented May 25, 2020

View reviewed changes

stale bot added the Resolution: Stale Automatically closed due to inactivity label Aug 23, 2020

stale bot closed this Aug 30, 2020

iChenLei mentioned this pull request Jan 26, 2021

allow react memo component can set displayName multiple times #20659

Closed

eps1lon deleted the feat/memo-component-name branch June 30, 2021 12:16

Match react-devtools display name behavior for React.memo #15313

Match react-devtools display name behavior for React.memo #15313

Uh oh!

Conversation

eps1lon commented Apr 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

acdlite left a comment

Choose a reason for hiding this comment

Uh oh!

acdlite commented Apr 3, 2019

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!

eps1lon commented Apr 3, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bvaughn commented Apr 11, 2019

Uh oh!

eps1lon Apr 14, 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!

sizebot commented Aug 29, 2019

Uh oh!

Airkro commented Sep 12, 2019

Uh oh!

stale bot commented Jan 9, 2020

Uh oh!

eps1lon commented Jan 10, 2020

Uh oh!

alflennik commented Feb 27, 2020

Uh oh!

eps1lon commented Feb 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codesandbox-ci bot commented Mar 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sizebot commented Mar 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Size changes (experimental)

Uh oh!

sizebot commented Mar 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Size changes (stable)

Uh oh!

eps1lon commented Mar 5, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stale bot commented Aug 23, 2020

Uh oh!

stale bot commented Aug 30, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

eps1lon commented Apr 3, 2019 •

edited

Loading

eps1lon Apr 14, 2019 •

edited

Loading

eps1lon commented Feb 27, 2020 •

edited

Loading

codesandbox-ci bot commented Mar 1, 2020 •

edited

Loading

sizebot commented Mar 1, 2020 •

edited

Loading

sizebot commented Mar 1, 2020 •

edited

Loading