DisplayName not being forwarded when wrapped in

[jtwigg]

When I receive a printed call stack from React, the components wrapped in observer show up as observerComponent

    at observerComponent (http://localhost:3000/static/js/bundle.js:119423:69)
    at div
    at observerComponent (http://localhost:3000/static/js/bundle.js:119423:69)
    at PrimaryWindow (http://localhost:3000/static/js/bundle.js:4346:5)

and in the callstack in the debugger shows up as anonymous

<anonymous> (/Users/johntwigg/development/crypto/loopNFT/ui/src/components/Cinema/CinemaMain.tsx:35)
<anonymous> (/Users/johntwigg/development/crypto/loopNFT/ui/node_modules/mobx-react-lite/src/observer.ts:104)
<anonymous> (/Users/johntwigg/development/crypto/loopNFT/ui/node_modules/mobx-react-lite/src/useObserver.ts:115)

Intended outcome:
Ideally, the underlying React Name would be maintained. CinemaMain in this case.

Actual outcome:
anonymous and observerComponent

Looking at issue #3422 doesn't seem to resolve or change the behavior for me.
For example

const FocusLane : React.FC = observer(() => {...})
FocusLane.displayName = "FocusLane"

has no impact.

Any ideas? I'm open to workarounds.

Versions

➜  ui git:(mobx) ✗ npm list --depth 2 | grep mobx
├─┬ [email protected]
│ ├── [email protected]

[mweststrate]

Did you search for existing issues first?

…

On Thu, 30 Jun 2022, 23:00 jtwigg, ***@***.***> wrote: When I receive a printed call stack from React, the components wrapped in observer show up as observerComponent at observerComponent (http://localhost:3000/static/js/bundle.js:119423:69) at div at observerComponent (http://localhost:3000/static/js/bundle.js:119423:69) at PrimaryWindow (http://localhost:3000/static/js/bundle.js:4346:5) and in the callstack in the debugger shows up as anonymous <anonymous> (/Users/johntwigg/development/crypto/loopNFT/ui/src/components/Cinema/CinemaMain.tsx:35) <anonymous> (/Users/johntwigg/development/crypto/loopNFT/ui/node_modules/mobx-react-lite/src/observer.ts:104) <anonymous> (/Users/johntwigg/development/crypto/loopNFT/ui/node_modules/mobx-react-lite/src/useObserver.ts:115) *Intended outcome:* Ideally, the underlying React Name would be maintained. CinemaMain in this case. *Actual outcome:* anonymous and observerComponent Looking at issue #3422 <#3422> doesn't seem to resolve or change the behavior for me. For example const FocusLane : React.FC = observer(() => {...}) FocusLane.displayName = "FocusLane" has no impact. Any ideas? I'm open to workarounds. *Versions* ➜ ui git:(mobx) ✗ npm list --depth 2 | grep mobx ├─┬ ***@***.*** │ ├── ***@***.*** — Reply to this email directly, view it on GitHub <#3438>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAN4NBFZQ2IROOO2ODL4GODVRYKG5ANCNFSM52KZEXGA> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[jtwigg]

Did you search for existing issues first?

Yes. I referenced the one here

Looking at issue https://github.com/mobxjs/mobx/issues/3422 doesn't seem to resolve or change the behavior for me.

It claims to be fixed.

Additionally, this describes behavior as "printing correctly" but its not, at least in the scenarios I described.
#2721 (comment)

[urugator]

Have you tried it without observer? I don't think displayName has any effect on callstacks, it's only relevant for dev tools or error messages.

[mdeem]

I can confirm that setting displayName on the return value of observe as imported from mobx-react-lite 3.4.0 doesn't work with the lasted react memo code. This code is now checking for name as well as displayName, and no longer forwards displayName changes if name is set, which is the case for observerComponent.

https://github.com/facebook/react/blob/cae635054e17a6f107a39d328649137b83f25972/packages/react/src/ReactMemo.js#L48

Changed 15 months ago in: facebook/react@d1542de#diff-38bea54d83c3ea71b17acbd041cbe83ef053a57e54311943cb50268e03d06fcdR40.

Also, it looks like if observerComponent were an anonymous functions, it would actually be replaced by displayName in the component stack trace built by react. https://github.com/facebook/react/blob/9abe745aa748271be170c67cc43b09f62ca5f2dc/packages/shared/ReactComponentStackFrame.js#L179

[urugator]

@mdeem thank you for the investigation.

If I follow correctly, following could suffice?

Object.defineProperty(observerComponent, "name", {
  name: baseComponent.name
})

Or should we do this only if (!baseComponent.name) ?

[urugator]

Any idea how to write a test for this? If I

const MemoCmp = React.memo(() => { 
  throw new Error('HERE');                
  return '';
})

The function name (second line) is just empty:

at performUnitOfWork (redacted\mobx\node_modules\react-dom\cjs\react-dom.development.js:26395:12) Error: HERE
at redacted\mobx\packages\mobx-react-lite\__tests__\observer.test.tsx:1102:15

[mweststrate]

I think in our code we do return an anonymous function? https://github.com/mobxjs/mobx/blob/main/packages/mobx-react-lite/src/observer.ts#L103. However, iirc either babel or the js engine implicitly renames some anonymous functions based on the variable name they are assigned to. Maybe we can work around that by either using function() ... instead of an arrow function, explicitly unsetting the name, or make the syntax confusingly enough to stop this effect from happening? (e.g. let x = 3 - 2 === 1 ? function() {} : null), dunno, something black magic like that to trick the engine.

[urugator]

@mweststrate Redefining name prop should be fine I think. Ideally we don't want to make the observerComponent "nameless" unless the original component is also "nameless" (inlined).
We just have to decide if observerComponent should always have the same name as original component or if it just should be nameless when original is nameless - the difference is what you will see in the callstack in a situation when original has a name - either you will see OriginalComponentName twice or you will see observerComponent + OriginalComponentName.
One way or another I dunno how to write a test for it.

[abrindam]

I just spent a good couple of hours debugging this problem, and here's what I've found.

The biggest issue is that React doesn't respect the displayName property any more, which appears to be an intentional change as part of what they call Native Component Stacks, in which they use an actual stack trace to generate their stacks. IMHO, this is a mistake as it makes debugging with higher order components like observer very difficult. I have weighed in on the relevant issue here: facebook/react#22315 (don't let the name fool you, it happens in every browser).

To work around this, as suggested above, you can instead redefine name instead of displayName. Note this must be done via Object.defineProperty and not a vanilla assignment. Unfortunately, this doesn't work in Firefox, because Firefox stacks ignore name, and that's where React is getting its info from. Still, as I will grudgingly admit to Chrome's overwhelming popularity, that might be a workaround for now.

On top of all this, if you're using the "set the display name after defining the component" strategy (option 2 on what I consider the definite source on this, #141 (comment)), this still doesn't work. The reason for this is that you're changing the displayName (or if working around, the name) of the memo which wraps the actual component, and React, through some black magic, unwraps the actual component when computing the stack trace, so the name is still missing.

To work around this, I had to author the following dubious code:

export function addDisplayName(component, displayName) {
    if (component.$$typeof.toString() === "Symbol(react.memo)") {
        // Memo objects have a `type` underlying property which is the thing they're wrapping
        component.type.displayName = displayName        
    }

    component.displayName = displayName
}

Would it be possible to define displayName on the memo as a bidirectional computed property (set and get) which would just proxy the underlying wrappedComponent's displayName? I haven't investigate this yet but maybe it would be possible.

[urugator]

Would it be possible to define displayName on the memo as a bidirectional computed property (set and get) which would just proxy the underlying wrappedComponent's displayName?

Unless something changed, React propagates the displayName to inner component, but only if it doesn't have a displayName already, in which case React basically assumes the inner component is inlined in HOC and therefore it's safe to have a sideffect that sets displayName - you have to consider the inner component can be used in other places without the HOC, so the name must be preserved if that's the case. Please check: #3192

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

Lines 155 to 157 in cfcf3c6

	// Don't redefine `displayName`,
	// it's defined as getter-setter pair on `memo` (see #3192).
	displayName: true

[abrindam]

If you look at the latest version React's ReactMemo.js which you cited in #3192 , you'll see that they sneakily added another check: the component must also not have a name already.

As previously established in this thread, wrappedComponent does indeed have a name, which is why this doesn't work right now.

See https://github.com/facebook/react/blob/main/packages/react/src/ReactMemo.js#L50-L52

[urugator]

I see, well that settles my concern above - we will just respect the original component name - if it's nameless we keep wrapper nameless as well, so that displayName always propagates, otherwise we redefine it. Thank you for pointing it out. I will try to incorporate the change into #3590 .

[urugator]

f239cb4 feedback welcome

[jer-sen]

@urugator it doesn't work for me ([email protected] [email protected]), changing observerComponent.name does not change the component name displayed in errorInfo.componentStack passed to an ErrorBoundary component. Also, the hack here doesn't work either for me in __DEV__ https://github.com/facebook/react/blob/main/packages/react/src/ReactMemo.js#L50-L52. It seems that only the real original name of the function is used so I found a fix, if you want to give it a try...:

    const tmpObject = {
        [baseComponent.name]: function (props, ref) {
        return useObserver(function () { return render(props, ref); }, baseComponentName);
    }
    }
    let observerComponent = tmpObject[baseComponent.name];
    observerComponent.displayName = baseComponent.displayName;

[urugator]

IIRC the name/displayName handling of observer(Cmp) should be currently the same as React.memo(Cmp), so you can only change the name if the original component is inlined, like observer/memo(() => {})
But tbh I don't remember why we/me settled on this behavior/impl, I think there should be no issue with doing it like this:

Object.defineProperty(memoObserverCmp, 'displayName', {
  set(name) {
    // Propagate name to created observerCmp
    observerCmp.displayName = name;
    // Modify original component name if it was inlined
    if (!originalCmp.name && !originalCmp.displayName) {
      originalCmp.displayName = name;
    } 
  },
  get() {
    return observerCmp.displayName;    
  }
})

@abrindam Does that check out?

[jer-sen]

For me, setting observerComponent.displayName doesn't change the component name in componentStack (still observerComponent). I let you check it on your own.

[mweststrate]

Afaik this issue is stale / no longer an issue with modern versions. If it is, please open a new issue