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

Consolidate behavior of React.forwardRef & observer #2527

Closed
O4epegb opened this issue Oct 7, 2020 · 11 comments
Closed

Consolidate behavior of React.forwardRef & observer #2527

O4epegb opened this issue Oct 7, 2020 · 11 comments
Labels
🍗 enhancement 🎁 mobx-react-lite Issue or PR related to mobx-react-lite package

Comments

@O4epegb
Copy link

O4epegb commented Oct 7, 2020

Basically code says it all:

const Component = React.forwardRef((props, ref) => {
  return <div ref={ref}>Component text</div>;
});

// Same outcome with or without { forwardRef: true }
// Error: baseComponent is not a function
const LiteObserverComponent = observerLite(Component);

// Works fine
const ObserverComponent = observer(Component);

Intended outcome

No error, same behavior between mobx-react and mobx-react-lite

Actual outcome

observer decorator from mobx-react-lite fails with an Error

How to reproduce the issue

Use code from above or this codesandbox https://codesandbox.io/s/mobx-react-lite-observer-bug-57hv6?file=/src/App.js

Versions

mobx 6.0.1
mobx-react 7.0.0
mobx-react-lite 3.0.0
react 16.13.1
react-dom 16.13.1

@O4epegb
Copy link
Author

O4epegb commented Oct 7, 2020

I am not sure if this is a bug or intended behavior?

@danielkcz
Copy link
Contributor

I guess we should have changed this with a major release, but it evaded our attention. For now, you have to use observer(comp, { forwardRef: true }).

@O4epegb
Copy link
Author

O4epegb commented Oct 7, 2020

By changed you mean add this functionality? I think it is viable use case.

@O4epegb
Copy link
Author

O4epegb commented Oct 7, 2020

I understand that mobx-react-lite only supports functional components, but React.forwardRef is a weird case because as far as I understand it does not return function, but at the same time it only used and supposed to be used for functional components

@danielkcz
Copy link
Contributor

danielkcz commented Oct 7, 2020

Well, it would of course require to do something like React.forwardRef(observer(comp)). The other way around would be a problem. The mobx-react is prepared for that and unwraps the forwarded refs, but this was never transported back to the lite.

https://github.com/mobxjs/mobx-react/blob/3d5439118400b43cafc7d52987322519d7b9e55a/src/observer.tsx#L34-L45

@O4epegb
Copy link
Author

O4epegb commented Oct 7, 2020

Yep, I also just checked it and it seems like it would be enough to just copy-paste this 10 lines of code?

If so I could make PR and update README a little bit maybe

@danielkcz
Copy link
Contributor

Well, we should have some tests for it too and need to consider how it will behave together with forwardRef option (ideally deprecate it). Also what happens when someone would use older mobx-react with the same logic.

But PR for starting the path would be certainly welcome.

@danielkcz danielkcz reopened this Oct 7, 2020
@danielkcz danielkcz transferred this issue from mobxjs/mobx-react-lite Oct 18, 2020
@danielkcz danielkcz added 🍗 enhancement 🎁 mobx-react-lite Issue or PR related to mobx-react-lite package labels Oct 18, 2020
@danielkcz danielkcz changed the title Wrapping React.forwardRef component with observer causes Error: baseComponent is not a function Consolidate behavior of React.forwardRef & observer Oct 18, 2020
@guhyeon
Copy link

guhyeon commented Dec 6, 2020

#2527 (comment)

I guess we should have changed this with a major release, but it evaded our attention. For now, you have to use observer(comp, { forwardRef: true }).

After that, the parent component throws an error.
JSX element type'PlaySetting Dialog' has no configuration or call signature.
It seems to hide the prop definition

@danielkcz
Copy link
Contributor

@guhyeon Please, if you have encountered a bug, open the new issue with a proper template. Such a shout in the dark is not helpful at all.

@patternleaf
Copy link

For internet people who find this as an open issue, note that it has been resolved as of mobx-react-lite 3.3.0: #3290, per issue #3267. Using observer(MyRefForwardingComponent) seems to work as expected. 🥳

@kubk
Copy link
Collaborator

kubk commented May 26, 2022

@patternleaf Thank you! I'll close the issue then.

@kubk kubk closed this as completed May 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🍗 enhancement 🎁 mobx-react-lite Issue or PR related to mobx-react-lite package
Projects
None yet
Development

No branches or pull requests

5 participants