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

Support react-redux 6 with reduxContext option #125

Closed

Conversation

philip-peterson
Copy link

Hey Ryan. This PR should resolve issue #124.

@justanotherjones
Copy link

👋 thanks for the PR!

@preactive
Copy link

preactive commented Feb 15, 2019

@ryanashcraft when merged when will this hit npm? Thanks in advance! :)

@ryanashcraft
Copy link
Contributor

@preactive Yes we'll ship this to npm when we get the chance to review and test. ETA sometime this week.

@preactive
Copy link

Awesome sauce! thanks @ryanashcraft!

@trashstack
Copy link
Contributor

Hey @philip-peterson,

Thanks for the PR!

Sorry it's taken us a bit of time to review it. After taking a look there are a couple of changes we'd like to propose/discuss.

  1. We think it will make the code cleaner to make this a backwards incompatible change and cut a new major version of redux-query that depends on react-redux 6+.
  2. How would you feel about breaking this PR down into two separate stages?
    One PR for the react-redux 6 compatibility just using the default ReactReduxContext.
    And another PR for the custom store context API.
    I think @ryanashcraft would like to have a longer think about the custom store API change. But we don't want that to be a blocker for the first part.

Let us know what you think.

And if you don't have time to work on this anymore we'd be happy to pick it up from where you left it.

@philip-peterson
Copy link
Author

Hey Amplitude peeps,

Those sound like reasonable changes. Only thing is to be wary of is people using this library on older installations of react-redux who may start asking for backports to bugfixes. Personally I'm not in that situation but we were only just recently able to upgrade our react-redux version, and React 15 obviously doesn't have the new context API.

Likely won't have a whole lot of bandwidth these coming weeks (things are pretty busy), so it would be best if y'all could pick this up.

Thanks <3

@trashstack
Copy link
Contributor

Only thing is to be wary of is people using this library on older installations of react-redux who may start asking for backports to bugfixes.

Yep, that's a good point.

We're not currently planning any other major changes so hopefully this won't come up. If it does then we can always consider adding back backwards compatibility.

@trashstack
Copy link
Contributor

Hey @philip-peterson,

Sorry that I didn't get to this sooner. I spent a bit of time this weekend working on the first stage of our proposed changes - making redux-query depend on the default ReactReduxContext.

Unfortunately I haven't been able to get that figured out. Given the current state of react-redux (reduxjs/react-redux#1177) I don't think I'm going to invest any more time in attempting this upgrade.

@preactive if you need support for react-redux 6 right now I'd suggest using @philip-peterson's branch for now.

When the "fixed" react-redux 6, or react-redux 7 comes out we'll revisit this.

@preactive
Copy link

@ManThursday Thanks for the update and info!

@salvoravida
Copy link

you shouldtry this https://github.com/salvoravida/react-redux-fork 6.5.0

reading from <ReactReduxContext.Consumer > is very slow.
Regards

@jesnil01
Copy link

react-redux 7 is now out

@ryanashcraft
Copy link
Contributor

Closing this in favor of #129. Thanks again @philip-peterson for the initial implementation.

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

Successfully merging this pull request may close these issues.

None yet

7 participants