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

Does not work with react-redux connect. #6

Closed
pgarciacamou opened this issue Jul 11, 2018 · 8 comments · Fixed by #12
Closed

Does not work with react-redux connect. #6

pgarciacamou opened this issue Jul 11, 2018 · 8 comments · Fixed by #12
Labels
bug Something isn't working

Comments

@pgarciacamou
Copy link
Owner

pgarciacamou commented Jul 11, 2018

With the new React.forwardRef added, the library does no longer works with react-redux.

See: reduxjs/react-redux#914 for more information.

We need to:

  1. fix the issue
  2. add test cases to make sure this works fine all the time and it doesn't break again
@pgarciacamou pgarciacamou added the bug Something isn't working label Jul 11, 2018
@pgarciacamou
Copy link
Owner Author

pgarciacamou commented Jul 11, 2018

The current implementation does not allow:

connect()(
  ContextConsumerHOC(SomeContext)(SomeComponent)
)

the current workaround is as follows:

ContextConsumerHOC(SomeContext)(
  connect()(SomeComponent)
)

but the functionality to pass the reference down doesn't work anymore.

@pgarciacamou
Copy link
Owner Author

pgarciacamou commented Jul 29, 2018

I'm working with react-redux to get this fix, it is going slower than I expected because the fix requires a feature implemented in react, and that is taking longer.

@daviscabral
Copy link

Is that related to the this.state.props?

@pgarciacamou
Copy link
Owner Author

@daviscabral I'm not sure I follow

@daviscabral
Copy link

daviscabral commented Aug 23, 2018

There was some on going changes related to it that made me work with redux and react-redux from master. So, right now, to have it working in my project, I have this on my package.json:

{
...
  "scripts": {
    ...
    "build:redux": "cd node_modules/redux/ && npm install --ignore-scripts && cross-env NODE_ENV=cjs rollup -c -o lib/redux.js",
    "build:react-redux": "cd node_modules/react-redux/ && npm install --ignore-scripts && cross-env BABEL_ENV=commonjs babel src --out-dir lib",
    "preinstall": "run-p build:redux build:react-redux"
  },
...
  "dependencies": {
    "react-redux": "reduxjs/react-redux",
    "redux": "reduxjs/redux",
...
  }
}

@pgarciacamou
Copy link
Owner Author

pgarciacamou commented Aug 23, 2018

@daviscabral

I hope this helps:

Using the latest version of redux and react-redux (which I'm not sure I would recommend) should not have anything to do with this issue.

To answer your question,

Is that related to the this.state.props?

This issue is not related to this.state.props (I'm not even sure that is in their codebase).

The bug described in this issue is related to an invariant in react-redux codebase that checks if the composed component passed to connect() is a function (rather than any valid React element type). Because React.forwardRef returns an object, react-redux throws an error.

If you are not interested in the React.forwardRef functionality for now, a simple solution is to install react-context-consumer-hoc v1.0.3. React.forwardRef was added in react-context-consumer-hoc v1.0.4.

npm install --save --save-exact react-context-consumer-hoc@1.0.3

NOTE: All this should be fixed as soon as react-redux updates the invariant, which is what reduxjs/react-redux#971 is trying to do.


Because you might be looking into why this bug is going on, here is some side context:

When React collaborators released React.forwardRef, they suggested that adding it to any library should be reflected in a major version increase. When I found this out, I should have reverted the change and release it into a patch version and then release a new major version with this functionality. But, I decided NOT to do that because I do not know if some people are already expecting this behavior in their codebase.

@daviscabral
Copy link

Cool, thank you for the "context" (lol). So, that thing about this.props.state that I was talking is because the way forwarding it - and it was not being available sometimes - causing connect to be broken and not refreshing components (not sure if was just with me - I need to revisit it later to be sure).

But again, thanks for taking the time to give me context - I hope to contribute here later. 👍

@pgarciacamou
Copy link
Owner Author

Because of the time that has taken for react-redux to implement the solution, I've decided to add a simple workaround.

Sadly, this workaround won't be implemented in version 1 of this library. It will be added to version 2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants