displayName not working in React 17 with observer

[codeBelt]

Intended outcome:

In this old issue (#141) it states the below should work. Also In the docs it states it will be fixed in React 17.

export const MyComponent2 = observer(() => "hello2!");
MyComponent2.displayName = "MyComponent2";

Is this an issue with observer now and/or should the docs be updated to say displayName does not work with observer?

Actual outcome:
Viewing "React DevTools" in CodeSandbox notice _c3 for the MyComponent2 name:

How to reproduce the issue:
https://codesandbox.io/s/mobx-displayname-reat-17-z61pb?file=/src/MyComponent.jsx

Versions

• mobx: 6.0.4
• mobx-react: 7.0.5
• react: 17.0.1
• react-dom: 17.0.1

[mweststrate]

Looks like a bug indeed, setting a displayName on a memo should work nowadays: https://codesandbox.io/s/react-17-memo-displayname-forked-do95u?file=/src/index.js

[urugator]

Swap these?

mobx/packages/mobx-react-lite/src/observer.ts

Lines 75 to 76 in 680ad86

	copyStaticProperties(baseComponent, memoComponent)
	memoComponent.displayName = baseComponentName

[iChenLei]

TL;DR

React v17 support set memo displayName, but only can set displayName once!

Code analysis

packages/react/src/ReactMemo.js

  if (__DEV__) {
    let ownName;
    Object.defineProperty(elementType, 'displayName', {
      enumerable: false,
      configurable: true,
      get: function() {
        return ownName;
      },
      set: function(name) {
        ownName = name;
        if (type.displayName == null) {  👈🏻   // This is why !
          type.displayName = name;
        }
      },
    });
  }

You can only set displayName once.

      set: function(name) {
-        ownName = name;
-        if (type.displayName == null) {
-          type.displayName = name;
+        if (typeof name === 'string') {
+          ownName = name;
+          type.displayName = name; 
+        } else {
+          console.error(
+            'React component\'s displayName must be a string, %o is invalid',
+            name
          );
        }
      },

I rebuild react to solve it 👉 https://codesandbox.io/s/mobx-displayname-reat-17-forked-gfyx3

Expected Devtools Shown

React displayName

React version 17.0.1
👉 https://codesandbox.io/s/react-17-memo-displayname-forked-yk4u0

Can react component set displayName multiple times ?

1. React.forwardRef 👌
2. Normal Function Component 👌
3. Class Component 👌
4. React.memo 🙅‍♂️

React component and mobx-react

packages/mobx-react-lite/src/observer.ts

export function observer<P extends object, TRef = {}>(
    baseComponent: React.RefForwardingComponent<TRef, P> | React.FunctionComponent<P>,
    options?: IObserverOptions
) {
    if (isUsingStaticRendering()) {
        return baseComponent
    }
    const realOptions = {
        forwardRef: false,
        ...options
    }
    const baseComponentName = baseComponent.displayName || baseComponent.name
    const wrappedComponent = (props: P, ref: React.Ref<TRef>) => {
        return useObserver(() => baseComponent(props, ref), baseComponentName)
    }
    wrappedComponent.displayName = baseComponentName  👈🏻   // set displayName once
    if (realOptions.forwardRef) {
        memoComponent = memo(forwardRef(wrappedComponent))
    } else {
        memoComponent = memo(wrappedComponent)
    }
    copyStaticProperties(baseComponent, memoComponent)
    memoComponent.displayName = baseComponentName  👈🏻   // set displayName twice
    return memoComponent
}

So you can't change memo component's displayName now.

export const MyComponent2 = observer(() => "hello2!");
MyComponent2.displayName = "MyComponent2";  👈🏻   // not work

Previous Try

// swap these ?
- copyStaticProperties(baseComponent, memoComponent) 
+ copyStaticProperties(memoComponent, baseComponent) 
 memoComponent.displayName = baseComponentName 

This solution is not useful, cause mobx-react didn't do anything wrong. 😄

Final Conclusion

I will create a PR for facebook/react soon , but not sure it will be accepted.

Hope my analysis is useful for you.

[danielkcz]

Thanks a lot for the thorough investigation, looks solid. Please link the PR here later.

[iChenLei]

I mentioned this issue in facebook/react PR, we can discuss this issue in that PR.
@FredyC by the way, can you invite me to join mobxjs github organization, I wanna be a long term mobxjs contributor. Thanks

[danielkcz]

Thanks for creating the PR in facebook/react, hopefully, they will process it soon-ish.

by the way, can you invite me to join mobxjs github organization, I wanna be a long term mobxjs contributor.

We are generally promoting based on as-needed basis, not based on people desires :) Just continue to be active for now and when you need elevated permissions, you will get them.

[mweststrate]

Looks it is going to be addressed in facebook/react#21392

[inoyakaigor]

@mweststrate @FredyC Looks like problem has been solved

[iChenLei]

closed via facebook/react#21392 , thanks for check @inoyakaigor

[urugator]

@inoyakaigor which react version? They mentioned the fix won't likely make it into v17:
facebook/react#21644 (comment)

[inoyakaigor]

17.0.2

UPD: I checked more closely and it seems that this issue should be reopened. Maybe the fact that I have no problem with displayName is due to the fact that facebook/react#21392 affects not only react but also React Dev Tools? (I have version 4.21.0)

[urugator]

Dunno, but the OP codesandbox works the same even with updated deps...

[LandonSchropp]

I still have this issue as well. displayName never did properly work for me. If I'm trying to track down an error such as a vague PropTypes validation failure, I usually have to work around it by commenting out sections of my code until I find the culprit.

[urugator]

Attempt to improve the situation a bit with #3192.
I did not test it, so I would appreciate if someone would give it a try.

[LandonSchropp]

@urugator Sorry for the slow response. I'd be happy to give this a try, but I'm having a bit of trouble setting up a local project to use local versions of all of the MobX packages. I looked for a guide, but I didn't see one. Is there some documentation on how to accomplish this?

Thanks!

[urugator]

Good question, I don't know :D
Naively: clone the PR, run yarn install, remove mobx deps from your project, copy/move mobx/packages/* to node_modules of your project. Alternatively instead of moving: npm link foo/mobx/packages/mobx-react in your project.
https://docs.npmjs.com/cli/v8/commands/npm-link

[LandonSchropp]

Sorry again for the delay! Here's what I did to set things up.

1. Cloned the mobx repo using the fix-2721 branch.
2. lerna bootstrap.
3. lerna build.
4. cd node_modules/react
5. yarn link (This was necessary to avoid an Invalid Hook error.)
6. cd my-project
7. yarn link mobx-react-lite && yarn link mobx-react && yarn link mobx
8. Restart my server

At the end of this, I'm still seeing the same behavior with the display names in the console that I was before. To illustrate, I intentionally triggered a PropTypes error, and this is what it looks like.

This is what the component that triggers that error looks like:

const ErrorButtons = observer(({ error }) => {
  ...
});

ErrorButtons.displayName = "ErrorButtons";

ErrorButtons.propTypes = {
  error: PropTypes.instanceOf(ErrorStore).isRequired,
  example: PropTypes.string.isRequired
};

[urugator]

I will put the console.log('PR3192') to the observer impl so it's easily verifiable a correct imple is being used. I wil ping you when it's done.

The intention is to get the same behavior as if you would replace observable with React.memo:

// `React.memo` instead of `observer`
const ErrorButtons = React.memo(({ error }) => {
  ...
});

ErrorButtons.displayName = "ErrorButtons";

ErrorButtons.propTypes = {
  error: PropTypes.instanceOf(ErrorStore).isRequired,
  example: PropTypes.string.isRequired
};

Could you please just verify that you get a better warning when using React.memo instead of observer?

[urugator]

Actually, I can add a test for this. I was under the impression it's only about dev tools so I didn't know how to write a test.
However I would like to avoid the extra prop-types dependency, so I tried:

const ObserverCmp = observer(() => {})
ObserverCmp.displayName = 'Test';

and got:
Test(...): Nothing was returned from render. This usually means a return statement is missing. Or, to render nothing, return null
So it does print correct display name in this case, did not try prop-types ...
Do you know about other warning I could easily produce in unit test? Nothing else comes to my mind atm.

[LandonSchropp]

Yep! Here's what I get if I use React.memo with the ErrorButtons and ErrorPage (the parent) components.

Both components are now showing up in the stack trace.

---

Adding a console.log statement is a good idea. I went ahead and added console.log('PR3192') to line 59 of observer.ts locally and re-built the project, but I don't see the comment locally. So maybe the fix is working and the problem is on my end?

I double-checked my linked packages and they look correct:

I'll keep digging on this for a bit and see if I can diagnose the problem.

Thanks!

[LandonSchropp]

I think I've narrowed down the issue. I'm seeing my log statements from mobx-react in my local repo, but not from mobx-react-lite. I ran lerna boostrap in the mobx project, and I double-checked the links inside of mobx-react and they seem to be correct.

If I run jest packages/mobx-react/__tests__/observer.test.tsx, then I see the expected log statements from the mobx-react-lite package, so they must be working within the package.

Any ideas? I'm a little stumped.

[urugator]

I think the problem is that mobx-react-lite is direct (not peer) dependency of mobx-react, so if you import observer from mobx-react it still uses it's own mobx-react-lite version (not the linked one). Not sure. I think you can override it with "resolutions": { "mobx-react-lite": "file:node_modules/mobx-react-lite"} or import observer directly from mobx-react-lite (hard to do for a larger project I assume)
https://classic.yarnpkg.com/lang/en/docs/selective-version-resolutions/
In the end I think I will just merge it and we can always revert the change, right. I was more worried whether it doesn't break the displayName in some other cases...

[LandonSchropp]

Makes sense. I tried the resolutions approach, but unfortunately, I'm still seeing the same thing.

I agree merging might be easier if it's low-risk. 🙂