Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Observer HOC is not getting displayName from forwardRef #3422

Closed
kazah96 opened this issue Jun 9, 2022 · 4 comments · Fixed by #3423
Closed

Observer HOC is not getting displayName from forwardRef #3422

kazah96 opened this issue Jun 9, 2022 · 4 comments · Fixed by #3423
Labels

Comments

@kazah96
Copy link
Contributor

kazah96 commented Jun 9, 2022

When i wrap forwardRef by observer, any infomation that can be used as display name is lost.

For example:

const ComponentWithForwardRef = forwardRef(function ComponentWithForwardRef(
  {},
  ref
) {
  return <div ref={ref} />;
});

const ObserverComponent = observer(ComponentWithForwardRef)

So ObserverComponent has name ObserverForwardRef instead ComponentWithForwardRef.

If i explicitly set

ComponentWithForwardRef.displayName = "ComponentWithForwardRef"
const ObserverComponent = observer(ComponentWithForwardRef)

i see same thing

The only variant ive found is to explicitly set displayName after wrapping in observer

const ObserverComponent = observer(ComponentWithForwardRef)
ObserverComponent .displayName = "ComponentWithForwardRef"

but it isnt look pretty

Also, mobx/no-anonymous-observer is not happy with all of listed variants.

Any ideas?

Checked for mobx-react-lite: 3.4.0
Here: https://codesandbox.io/s/condescending-glade-yx3qgy?file=/index.js

@urugator
Copy link
Collaborator

urugator commented Jun 9, 2022

Hm I don't think we can or should do something about this, because it's actually a problem of React.memo.
If you replace the observer with memo (which is what we do internally) you get the same result...
(EDIT: Note, you don't even have to use forwardRef)

Regarding eslint-plugin, IIRC the plan was to remove/deprecate no-anonymous-observer and suggest using
eslint-plugin-react using "componentWrapperFunctions": ["observer"] configuration + react/display-name rule, which should have similar, but more reliable effect).
I did not realize there is such option at the time of writing the no-anonymous-observer rule.

@kazah96
Copy link
Contributor Author

kazah96 commented Jun 10, 2022

@urugator Thank you for the reply.
Seems we dont got much to do, but ive checked source code of observer, and found
const baseComponentName = baseComponent.displayName || baseComponent.name,

and it looks like if we wrapping React.forwardRef, we can get its name from render.name of forwardRef component

"componentWrapperFunctions" + react/display-name works fine with explicit Component.displayName = "Component" setting. Should we add this workaround to mobx-eslint-plugin docs?

@urugator
Copy link
Collaborator

and it looks like if we wrapping React.forwardRef, we can get its name from render.name of forwardRef component

Yea, I dunno what would be a proper thing to do here - whether we should get the name from ref wrapper as now or rather from wrapped component. Ideally it should behave as with react alone. The whole displayName business is a bit magical and can even differ from version to version. I would have to investigate, however I don't think it's worth it atm because once we apply memo, the displayName seems lost anyway. Or perhaps I am missing something?

Should we add this workaround to mobx-eslint-plugin docs?

Yes and deprecate and remove it from recommended. PR welcome.

@kazah96
Copy link
Contributor Author

kazah96 commented Jun 10, 2022

@urugator, if we set displayName after wrapping in memo, things gonna work (at first glance). But this looks a bit hacky
EDIT: we can set MemoComponent.displayName only once? If so, it really seems there is no other way)

Btw, i made a PR with docs edit and rule removal, check please

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants