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 v6 #581

Closed
markerikson opened this Issue Dec 6, 2018 · 11 comments

Comments

Projects
None yet
5 participants
@markerikson
Copy link

markerikson commented Dec 6, 2018

Do you want to request a feature or report a bug?

Bug / compatibility issue.

What is the current behavior?

FirestoreConnect currently tries to access the Redux store out of legacy context:

[storeKey]: PropTypes.object.isRequired

That's never been part of the React-Redux public API. React-Redux v6 now uses new context internally, so that breaks this library.

You'll need to rework the internals to access the Redux store a different way, likely using the ReactReduxContext instance exported from React-Redux.

See supasate/connected-react-router#191 for an example of how connected-react-router is handling the change, and some related discussion at reduxjs/react-redux#1083 .

What is the expected behavior?

Should render okay with no errors.

Which versions of dependencies, and which browser and OS are affected by this issue? Did this work in previous versions or setups?

Fails with React-Redux v6.0.0, works with v5.x and earlier.

@geminiyellow

This comment has been minimized.

Copy link

geminiyellow commented Dec 6, 2018

👍 thank you @markerikson , that help me

@prescottprue

This comment has been minimized.

Copy link
Owner

prescottprue commented Dec 6, 2018

@markerikson Thank you for reporting this! I am adding it to the proposed features in the v3.0.0 roadmap. That said, I wonder if this is a big enough change/rewrite to excuse trying out the move to the single repo for all of the redux-firebase tools 👀 (been wanting to do this for a long while for a number of reasons).

@Fandekasp

This comment has been minimized.

Copy link

Fandekasp commented Dec 6, 2018

@prescottprue can you add a timeline estimate to the roadmap ?
react-redux v6 also breaks a lot of stuff in my project (not a good idea to write unittests with store as a props), and I'd love to organize myself as to match with the release of react-redux-firebase v3 (or redux-firebase v1).

@prescottprue

This comment has been minimized.

Copy link
Owner

prescottprue commented Dec 6, 2018

I avoided that initially since it is hard to say accurately for a number of the features, especially this one since it is such a re-write. I will try to add timelines as soon as I can get a better understanding of scope though, so thanks for the suggestion.

A good way to go for now is to assume that you should build for how things are currently. I try to keep a clear/visible update path for major breaking changes since there are a number of production applications that myself and others I work with will have to upgrade. If certain features need to be moved to other future versions or redux-firebase v1, then that will happen.

@prescottprue

This comment has been minimized.

Copy link
Owner

prescottprue commented Dec 9, 2018

Started looking into this, and I was able to get a change working with firebaseConnect in the simple example. It was a pretty major change like we were thinking, and actually doesn't use store for the firebase instance at all:

NOTE: This is just a prototype I threw together, and is not an available API. Just showing how I got things working

// Initialize Firebase instance
firebase.initializeApp(fbConfig)

const App = () => (
  <Provider store={store}>
    <ReactReduxFirebaseProvider firebase={firebase} config={fbConfig} dispatch={store.dispatch}>
      <Home />
    </ReactReduxFirebaseProvider>
  </Provider>
)

Then it doesn't even require the store enhancer anymore! I also exposed ReactReduxFirebaseConsumer (used in firebaseConnect to get the enhanced instance from context).

The main downside being that it requires react 16 since it uses createContext.

Its not available on npm yet, but I'm going to work on getting this into a v3.0.0 pre-release version as soon as possible. The API may change, so please share your input on naming or patterns. Since this is such a drastic change in architecture I want to make sure there is a path forward for upgrading from previous versions.

@markerikson

This comment has been minimized.

Copy link

markerikson commented Dec 9, 2018

Since React-Redux v6 requires a minimum of React 16.4, I don't think that's going to be an issue. In fact, I'd suggest you do the same.

@prescottprue

This comment has been minimized.

Copy link
Owner

prescottprue commented Dec 12, 2018

Just started the provider for firestore.

When using just Firestore, the provider setup is like so:

import firebase from 'firebase/app'
import 'firebase/auth'
import 'firebase/database'
import 'firebase/firestore' // make sure you add this for firestore
import { Provider } from 'react-redux'
import { ReactReduxFirebaseProvider } from 'react-redux-firebase';
import { createFirestoreInstance } from 'redux-firestore';
import { firebase as fbConfig, reduxFirebase as rrfConfig } from './config'

// Initialize Firebase instance
firebase.initializeApp(fbConfig)

const App = () => (
  <Provider store={store}>
    <ReduxFirestoreProvider
      firebase={firebase}
      config={rrfConfig}
      dispatch={store.dispatch}
      createFirestoreInstance={createFirestoreInstance}>
      <Home />
    </ReduxFirestoreProvider>
  </Provider>
)

Having both providers can be done through the main provider like so:

<ReactReduxFirebaseProvider
  firebase={firebase}
  config={rrfConfig}
  dispatch={store.dispatch}
  createFirestoreInstance={createFirestoreInstance}>
    <Home />
</ReactReduxFirebaseProvider>

Equivalent to:

<ReactReduxFirebaseProvider firebase={firebase} config={fbConfig} dispatch={store.dispatch}>
    <ReduxFirestoreProvider
        firebase={firebase}
        config={fbConfig}
        dispatch={store.dispatch}
        createFirestoreInstance={createFirestoreInstance}>
        <Home />
    </ReduxFirestoreProvider>
</ReactReduxFirebaseProvider>

That said, what are thoughts on the names (everyone feel free to answer)? I was going to do FirebaseProvider or ReduxFirebaseProvider, but went with the long name for clarity. Not as sold on the long name now that I see the example code, but maybe that is just me.

Still haven't released it to npm, but now that all of the HOCs use this new pattern on my branch I am planning on getting it published into an alpha version. Don't want to speak too soon, but hopefully to be released sometime this week.

prescottprue added a commit that referenced this issue Dec 13, 2018

Support react-redux v6 and React 16 context API - #581 (#584)
* feat(core): New react context API working with HOCs (`firebaseConnect`, `firestoreConnect` `withFirebase`, and `withFirestore`) - #581
* feat(core): support `react-redux` v6 by removing usage of `context.store` (used to be added by `Provider` from `react-redux`)
* feat(core): Added `ReactReduxFirebaseContext` and `ReduxFirestoreContext` (with associated providers `ReactReduxFirebaseProvider` `ReduxFirestoreProvider` respectively)

Breaking Change: react 16.4 is now a requirement due to using new context api

@prescottprue prescottprue referenced this issue Dec 13, 2018

Merged

v3.0.0-alpha #566

3 of 3 tasks complete

prescottprue added a commit that referenced this issue Dec 15, 2018

v3.0.0-alpha (#566)
* feat(HOCs): switch `firestoreConnect` and `firebaseConnect` to use `componentDidMount` over `componentWillMount` - #564 
* feat(core): New react context API working with HOCs (`firebaseConnect`, `firestoreConnect` `withFirebase`, and `withFirestore`) - #581
* feat(core): support `react-redux` v6 by removing usage of `context.store` (used to be added by `Provider` from `react-redux`)
* feat(core): Added `ReactReduxFirebaseContext` and `ReduxFirestoreContext` (with associated providers `ReactReduxFirebaseProvider` `ReduxFirestoreProvider` respectively)
* fix(core): shrink build sizes considerably by adding `babel-preset-minify` - #573
* feat(deps): update `hoist-non-react-statics` to `^3.1.0`
* feat(deps): upgrade to `babel^7`
* feat(deps): Update to `webpack^4` (along with `webpack-cli`)
* feat(deps): update react peer dependency to `^16.4` for new Context API (matches `react-redux`)
@prescottprue

This comment has been minimized.

Copy link
Owner

prescottprue commented Dec 15, 2018

Released v3.0.0-alpha to the next tag (it can be installed with npm i --save react-redux-firebase@next). Also published the v3.0.0 section of the docs which explains how to install and includes a migration guide.

It is all still young, so be sure to reach out if things don't work as expected or if you have any input on a more clear API. I want to get things ironed out in the pre-release versions as soon as possible.

Thanks to to all for reporting!

@salmazov

This comment has been minimized.

Copy link

salmazov commented Dec 17, 2018

screenshot 2018-12-17 at 13 00 15

Please correct the docs
@prescottprue

This comment has been minimized.

Copy link
Owner

prescottprue commented Dec 17, 2018

@salmazov Fixed. Thanks for reaching out. Feel free to open a new issue if anything is not working as you expect or if you find other typos in the docs.

@salmazov

This comment has been minimized.

Copy link

salmazov commented Dec 18, 2018

@prescottprue Okey! Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment