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

Issue with composed HOC and forwarding Ref #44

Closed
wants to merge 1 commit into from

Conversation

popzelife
Copy link

I had an issue while composing HOC with handleViewport and withRouterAndRef, being a HOC for react-router-dom to support forwarding Ref (see issue here remix-run/react-router#6056)

import { compose, setDisplayName } from 'recompose';
{...}

export default compose(
  setDisplayName('Component'),
  withRouterAndRef,
  handleViewport,
)(Component);

React was complaining about functional component and the way it attempts to access Ref:

Warning: Function components cannot be given refs. Attempts to access this ref will fail. Did you mean to use React.forwardRef()?

Check the render method of `RouterAndRefHoc`.
    in InViewport (created by RouterAndRefHoc)

So when investigating, I change your HOC to a way I'm used to and it finally worked. I ended not using hoist-non-react-statics. Not sure if it's still needed this way.
Maybe I've misused your package ? React.forwardRef tends to be a challenge sometimes.

* FIX change way to forwardRef

* TYPO trailing comma
@roderickhsiao
Copy link
Owner

Thanks for the pull request and investigation!
Would you mind to share about some code snippet about how you use the package?

Would love to address the issue and test out on different example to see what I'm missing here.

Thanks!

@roderickhsiao
Copy link
Owner

I'll create an example this week and test it out

@roderickhsiao
Copy link
Owner

@popzelife I made some changes and publish a new version, would you mind to give a try to see if that fixes your issues? (some edge cases are not handled in your PR and could break other use case)

Thanks again

@popzelife
Copy link
Author

popzelife commented Nov 15, 2019

@roderickhsiao thanks for your update. Sorry for the delay, just came back from holidays.

I tested the new version and I still have the issue. I'll have a look and send you an example project (with withRouterAndRef file) - so you can test on your own. I'm wondering if there is an issue with hoist-non-react-statics, forwardRef and React-Router v4 ?

In this order, I will have a warning but handleViewport will work:

export default compose(
  setDisplayName('Component'),
  withRouterAndRef,
  handleViewport,
)(Component);

But if I switch order, handleViewport just won't work (with still a warning of course):

export default compose(
  setDisplayName('Component'),
  handleViewport,
  withRouterAndRef,
)(Component);

@roderickhsiao
Copy link
Owner

@popzelife I created one example #49 in this branch but I couldn't reproduce the issue, I didn't see error when running storybook and also changing both order seems fine.

@roderickhsiao roderickhsiao changed the base branch from master to main October 16, 2020 03:11
@popzelife
Copy link
Author

Hi @roderickhsiao, I hadn't time to investigate more and I believe the issue was due to a bad use of forwardedRef on our side. I moved to another project last year and now re-using this library I don't have this issue anymore. So I can close this PR.
Thank you for your time investigating.

@popzelife popzelife closed this Jan 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants