pgarciacamou / react-context-consumer-hoc

HOC that consumes multiple contexts and pass as props to components
MIT License
18 stars 3 forks source link

Does not work with react-redux connect. #6

Closed pgarciacamou closed 6 years ago

pgarciacamou commented 6 years ago

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

See: https://github.com/reduxjs/react-redux/issues/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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

Is that related to the this.state.props?

pgarciacamou commented 6 years ago

@daviscabral I'm not sure I follow

daviscabral commented 6 years ago

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 commented 6 years ago

@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 https://github.com/reduxjs/react-redux/pull/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 commented 6 years ago

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. :+1:

pgarciacamou commented 6 years ago

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.