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

feat(core): new pattern for firebase instance in thunks/sagas (getFirebase not part of v3 API) #635

Closed
ljacho opened this issue Feb 10, 2019 · 29 comments

Comments

@ljacho
Copy link

ljacho commented Feb 10, 2019

getFirebase is not returning firebase instance. Getting undefined.

@prescottprue
Copy link
Owner

@sbtgE What version are you using? How/when are you using getFirebase. It depends on when your instance is initialized.

@ljacho
Copy link
Author

ljacho commented Feb 10, 2019

@prescottprue trying "3.0.0-alpha.9" and I want to access firebase instance in thunk:

...
import { getFirebase } from 'react-redux-firebase';
import { getFirestore } from 'redux-firestore';
...

export default function configureStore(intState = {}, history) {
  const middlewares = [
    routerMiddleware(history),
    thunk.withExtraArgument({getFirebase, getFirestore}),
    logger
  ];

  const enhancers = [
    applyMiddleware(...middlewares),
  ];

  const store = createStore(
    createReducer(),
    intState,
    compose(...enhancers));

  // Hot reloading
  if (module.hot) {
    // Enable Webpack hot module replacement for reducers
    module.hot.accept('./reducers', () => {
      store.replaceReducer(createReducer());
    });
  }

  return store;
}

actions.js

export function register(newUser) {
  return (dispatch, getState, { getFirebase, getFirestore }) => {
    dispatch({ type: ACTION_LOADING_SET })

    const firebase = getFirebase()
    const firestore = getFirestore()

 ...

@prescottprue
Copy link
Owner

prescottprue commented Feb 11, 2019

@sbtgE getFirebase is not part of the v3.0.0 api (mentioned in the v3.0.0 roadmap, but looking to make this more clear). The new API uses the stable React context api and does not have any store enhancers. The way that context is now passed means that the wrapped firebase instance, which was kept on the store (store.firebase), is no longer necessary. That said, there was already is an open question of best pattern within thunks/sagas, so I'll put this ticket next to it.

I have personally been leaving the business logic within handlers (thunks were most often not needed in my cases), or where necessary passed the wrapped method through (i.e. push or update). This is because the wrapped instance can be passed using React's stable context api and pulled with a Provider (ReactReduxFirebaseProvider) and used right within your component's handlers. The goal was to actually get folks to move towards doing that by providing the instance as props in withFirebase and firebaseConnect.

Wondering for your case:

  • Are you dispatching other custom actions in your thunk?
  • Is there a reason not to call the wrapped method then whatever is in your thunk?
  • Is logic within thunks an organizational thing or is there another specific reason for using them?
  • Are you on a new enough version of react to be using the context API?

The reason I ask is that the v3 API is by no means stable, so I am trying to understand as many use cases as possible. Especially since there has been a middleware planned for a while that will handle calling async methods (like push/set) through dispatching an action like CALL_FIREBASE (not decided on yet) - your use case may be a good fit to test this pattern out.

@ljacho
Copy link
Author

ljacho commented Feb 12, 2019

Thank you for the comprehensive answer. First of all, I have to admit, that I am not so experienced in the topic but I will do my best to describe the whole thing. According to my current experience context API is great, but I don't see an easy way to switch from using the store and thunk with a lot of custom actions to using context actually. Imho the best way for this moment would be to support both solutions at least for the next major version - dispatching e.g. CALL_FIREBASE could be helpful. I would like to follow this topic and see what others think about that - still learning - I think that slack/discord channel would help speed up discussion. Appreciate your help @prescottprue.

@prescottprue
Copy link
Owner

@sbtgE No problem at all, glad to talk out the options. I like the idea of keeping the API similar for this version, but still need to look into how that would be done. As for a chat line, there is the the gitter channel.

@prescottprue prescottprue changed the title getFirebase returns undefined feat(core): new pattern for firebase instance in thunks/sagas (getFirebase not part of v3 API) Feb 12, 2019
@prescottprue prescottprue added this to the v3.0.* milestone Feb 12, 2019
@prescottprue
Copy link
Owner

#607 was another duplicate of this question. Clearly something that will needs to be addressed.

@kelset
Copy link

kelset commented Mar 5, 2019

Related to this, I've hit myself this issue and I feel it's also caused by the fact that neither the release tabs nor the docs for v3.0 mention this (it's actually still there) - even the migration guide doesn't mention it.

@wen-l
Copy link

wen-l commented Mar 13, 2019

@sbtgE getFirebase is not part of the v3.0.0 api (mentioned in the v3.0.0 roadmap, but looking to make this more clear). The new API uses the stable React context api and does not have any store enhancers. The way that context is now passed means that the wrapped firebase instance, which was kept on the store (store.firebase), is no longer necessary. That said, there was already is an open question of best pattern within thunks/sagas, so I'll put this ticket next to it.

I have personally been leaving the business logic within handlers (thunks were most often not needed in my cases), or where necessary passed the wrapped method through (i.e. push or update). This is because the wrapped instance can be passed using React's stable context api and pulled with a Provider (ReactReduxFirebaseProvider) and used right within your component's handlers. The goal was to actually get folks to move towards doing that by providing the instance as props in withFirebase and firebaseConnect.

Wondering for your case:

  • Are you dispatching other custom actions in your thunk?
  • Is there a reason not to call the wrapped method then whatever is in your thunk?
  • Is logic within thunks an organizational thing or is there another specific reason for using them?
  • Are you on a new enough version of react to be using the context API?

The reason I ask is that the v3 API is by no means stable, so I am trying to understand as many use cases as possible. Especially since there has been a middleware planned for a while that will handle calling async methods (like push/set) through dispatching an action like CALL_FIREBASE (not decided on yet) - your use case may be a good fit to test this pattern out.

So I've also ran into this issue and only after a few hours of trying to fix this, did I stumble upon this thread. I am still quite new to react, redux and firestore but if you still require any insight into use cases for thunks, my thoughts are that the logic put in thunks are more so for organisational and re-usability purposes as action creators can be reused by multiple components.

But again I am still very new to react, redux and firebase, and for the time being I'm using probably a really bad workaround where I take the firebase/firestore instance from the current prop (which is connected to firestore using "withFirestore"), and then pass the firebase/firestore instance into the dispatched function so It can be accessed in the action creator. Obviously this is not at all ideal since you need to get and pass the firestore instance everytime you call the action creator and have no clue as to the performance or structural implications and impacts of this workaround.

@prescottprue
Copy link
Owner

prescottprue commented Mar 14, 2019

@wen-l Your logic can be separated into actionCreators as you mentioned without needing to use thunks - thunks are only needed when dispatching in async.

Dispatching Actions (Async & Sync)

Since mapDispatchToProps from connect accepts a function or an object (as described in their docs) you can do either of the following:

As an object (each item is assumed to be a named action creator, so it is wrapped in dispatch):

const mapDispatchToProps = {
  someActionCreator: () => {
    return { type: 'SOME_ACTION_TYPE' }
  }
}

export default connect(null, mapDispatchToProps)

or as a function which receives dispatch and ownProps (props of the component) as arguments. This is great for handling the async stuff folks often "need" thunks for:

function mapDispatchToProps(dispatch, ownProps) {
  return {
    someActionCreator: () => {
      ownProps.firebase.push('some/path', { example: 'data' })
       .then(() => {
          dispatch({ type: 'SOME_ACTION_TYPE' })
       })
    }
  }
}

const enhance = compose(
  withFirebase, // add props.firebase
  connect(null, mapDispatchToProps)
)

export default enhance(SomeComponent)

Separating Logic

Since we can pass an object full of handlers in this way, it is easy to move them all to another file then just import. This can be done a number of ways including directly with connect and using withHandlers from recompose (both shown below):

Directly With Connect

handlers.js

// handler name is used to denote "props" being passed and it returns a function
// 
export function someHandler(props) {
  return () => {
      ownProps.firebase.push('some/path', { example: 'data' })
       .then(() => {
          dispatch({ type: 'SOME_ACTION_TYPE' })
       })
  }
}

SomeComponent.js

import { someHandler } from './handlers'

function mapDispatchToProps(dispatch, ownProps) {
  return {
     someAction: someHandler(ownProps, dispatch)
  }
}

export default connect(null, mapDispatchToProps)(SomeComponent)

With Recompose

import * as handlers from './handlers'

const enhance = compose(
  withFirebase,
  connect(), // add props.dispatch
  withHandlers(handlers)
)

export default enhance(SomeComponent)

@wen-l
Copy link

wen-l commented Mar 15, 2019

Ok thanks for this, I'm learning new things about react 😄
There's only one other thing I can think of regarding the separation of logic into other files and that is probably that thunk has the added bonus of automatically passing the dispatch method to the external handler so that we do not need to manually pass the dispatch method to every external handler as well. Your above examples are nice and have helped me better understand redux, however it's not so different to how I've described my workaround. Let me illustrate:

Without Thunk

actionCreator.js

export const doSometing = (props, dispatch, data) => {    // Need: props and dispatch as well as data
    return () => {
        props.firestore.collection('dataStore').add({
            ...data
        }).then(() => {
            dispatch({ type: 'DONE_SUCCESS', data });
        }).catch((err) => {
            dispatch({ type: 'DONE_FAILED', err });
        })
    }
}

SomeComponent.js

import { doSomething } from '/somewhere'

function mapDispatchToProps(dispatch, ownProps) {
    return {
        doAction: doSomething(ownProps, dispatch, data) // Must pass props, dispatch alongside data
    }
}

export default connect(null, mapDispatchToProps)(SomeComponent)

With Thunk

actionCreator.js

export const doSometing = (props, data) => {
    return (dispatch, getState) => {    // We get these from thunk
        props.firestore.collection('dataStore').add({
            ...data
        }).then(() => {
            dispatch({ type: 'DONE_SUCCESS', data });
        }).catch((err) => {
            dispatch({ type: 'DONE_FAILED', err });
        })
    }
}

SomeComponent.js

import { doSomething } from '/somewhere'

function mapDispatchToProps(dispatch, ownProps) {
    return {
        doAction: doSomething(ownProps, dispatch, data) // Must pass props, dispatch alongside data
    }
}

export default connect(null, mapDispatchToProps)(SomeComponent)

So from this example, they are practically the same except without thunk means we need to for each reference to an external handler/actioncreator we must pass along with it the dispatch, whereas with thunk, dispatch (as well as reducer state(s)) are provided to external handlers/actioncreators (Not sure whats difference between handler/action creator just heard these phrases being used sometimes 😆).

So with the previous version of this react-redux-firebase, we were able to add an extra argument to get an instance of firebase or firestore using thunk so that we did not need to pass around the props either(to fetch the firestore/firebase instance from those props). But I'm not sure how the new (Not new to me, just started learning react so everything is new) context system would allow access to these firebase/firestore instances outside of passing props around.

So yeah, thats the only other reason I can think of for the use of thunk in this case, feel free to correct any mistake as Im still learning. 😄.

@wen-l
Copy link

wen-l commented Mar 17, 2019

Also further from this, since we do not use the reactReduxFirebase method to attach the firebase instance to the store, we are not able to use the "attachAuthIsReady" setting as this config is not passed to the store and hence no function firebaseAuthIsReady is attached to the store object. Is there another way to access this promise, or for now do we need to create our own promises?

@opula
Copy link

opula commented Mar 17, 2019

I cannot speak on thunk, but I have been using redux-saga for the past two years. This is my first attempt at creating an app using firebase. In my opinion, running firebase commands prior to dispatching redux actions isn't a viable solution. The point of saga is to manage async and sync operations in a reasonable fashion. Imagine the following scenarios: updating / pushing multiple documents at the same time, calling an external api prior to making an update, debouncing / throttling calls, etc. Given the async nature of firebase, it makes sense to handle those calls inside a saga flow.

It looks like the previous version created a singleton of the firebase instance. Now that instance is create within the provider. I can create a second instance within the store creation method and pass that as the second argument of sagaMiddleware.run. Would this be okay? Or would there be an adverse reaction?

const fbInstance = createFirebaseInstance(firebase, rrfConfig, store.dispatch); sagaMiddleware.run(rootSaga, fbInstance);

@prescottprue
Copy link
Owner

@opula That was actually what I was thinking would be in the docs for the new version - that should be fine. Still need to investigate if there are issues with sharing listener count across instances, but other than that things should work as expected.

What you are bringing up with saga may fit better with a middleware I have been proposing for a while which would allow dispatching an action (something like CALL_FIREBASE) that would then call the async action.

@Akash187
Copy link

Akash187 commented Apr 10, 2019

@prescottprue I am using original firebase instance. Is this correct way.
`import {firebase } from './../../config/fbConfig';

export const signIn = (credentials) => {
return (dispatch, getState) => {
firebase.auth().signInWithEmailAndPassword(
credentials.email,
credentials.password
).then(() => {
dispatch({type: 'LOGIN_SUCCESS'})
}).catch((err) => {
dispatch({type: 'LOGIN_ERROR', err})
})
}
};
`

@prescottprue
Copy link
Owner

prescottprue commented Apr 18, 2019

Yes, that should work just fine @Akash187. That said, if you are doing email login and want to dispatch an action, you can use login (a method which dispatches actions on success/error of logins of all types, including email).

@atultherajput
Copy link

If getFirebase is not part of the v3.0.0 api then why it is mentioned here: http://docs.react-redux-firebase.com/history/v3.0.0/docs/api/get-firebase.html ?

@prescottprue
Copy link
Owner

prescottprue commented Apr 29, 2019

@atultherajput That is because it was copied over from the v2 docs. I will remove it from there, thanks for posting.

All of that said, we may end up having to keep getFirebase in the v3 api since there is so many folks looking to keep using it. Personally, I have been trying to remove this from the API for a while since I don't find it to be very clear.

If folks could post their reason for still wanting to use it, that would be a great way to gauge if it should be kept. Currently in v3, there is not an enhancer, which means that the firebase instance is not strapped to store.firebase, which therefore means that there is no existing instance to load - this is because the instance is available through the new context API.

@alex-r89
Copy link

alex-r89 commented May 8, 2019

Hi,

I was wondering if anyone could advise:

Due to getFirebase not working in V3, and therefore it not being possible to do something like thunk.withExtraArgument(getFirebase) , could you not simply define your own getFirebase method and pass that into the thunk, for example:

import firebase from './Firebase/firebase'

...

const getFirebase = () => firebase

const store = createStore(rootReducer, compose(applyMiddleware(thunk.withExtraArgument(getFirebase))))


...

And then later on, in say a Redux action, you could do something like this

export const signIn = (data) => {
  return (dispatch, getState, { getFirebase }) => {
    //Async call
    const firebase = getFirebase()
    firebase
      .auth()
      .signInWithEmailAndPassword(data.email, data.password)
      .then(() => {
        dispatch({ type: 'LOGIN_SUCCESS' })
      })
      .catch((err) => {
        dispatch({ type: 'LOGIN_ERROR', err })
      })
  }
}

is there anything wrong with that? It seems too simple?

prescottprue added a commit that referenced this issue May 10, 2019
* fix(docs): remove remaining instances of getFirebase from docs - #635 (#694)
* feat(core): Start React hook API - #684 - @illuminist
@samsoul16
Copy link

samsoul16 commented May 10, 2019

Hello Guys, i would like to share a working solution for getting redux, react-redux, react-redux-firebase, redux-firestore, redux-thunk and redux-devtools-extension completely working back up.

The below code uses the latest version of all the libraries so please look at the version of dependencies on the left first and to get it working in your project.

Please take a look at this code-pen : https://codesandbox.io/s/8423o1m529

Here i have created separate actions file which has (actions / actionCreator as some say) and call firestore functions from there, which is one of the most common use case from what i have seen.

I have organized everything as i have done in my original project just not added files to folders.

Also it would be great if you could just have a glimpse through it @prescottprue.

The reason for me not posting everything here is that it would take too much space and this will not be a great place to do it.

@omern92
Copy link

omern92 commented May 13, 2019

I have struggled with the same issue.

The solution is to import the firebase instance you created and use it.

Example:
I had this simple action, which used the thunk "getFirebase()".

export const signInAsync = (creds) => {
  return async (dispatch, getState, {getFirebase}) => {
    const firebase = getFirebase();
    try {
      await firebase.auth().signInWithEmailAndPassword(creds.email, creds.password);
      dispatch(closeModal());

    } catch (error) {
      console.log(error);
    }
  };
};

Now, in the v3 version, you access your "firebase" instance directly, by importing it:

import firebase from '../../app/config/firebase';  // Import it.

export const signInAsync = (creds) => {
  return async (dispatch) => {
    try {
      await firebase.auth().signInWithEmailAndPassword(creds.email, creds.password);
      dispatch(closeModal());

    } catch (error) {
      console.log(error);
    }
  };
};

I hope it was plain enough.

@samsoul16
Copy link

samsoul16 commented May 14, 2019

I have struggled with the same issue.

The solution is to import the firebase instance you created and use it.

Example:
I had this simple action, which used the thunk "getFirebase()".

export const signInAsync = (creds) => {
  return async (dispatch, getState, {getFirebase}) => {
    const firebase = getFirebase();
    try {
      await firebase.auth().signInWithEmailAndPassword(creds.email, creds.password);
      dispatch(closeModal());

    } catch (error) {
      console.log(error);
    }
  };
};

Now, in the v3 version, you access your "firebase" instance directly, by importing it:

import firebase from '../../app/config/firebase';  // Import it.

export const signInAsync = (creds) => {
  return async (dispatch) => {
    try {
      await firebase.auth().signInWithEmailAndPassword(creds.email, creds.password);
      dispatch(closeModal());

    } catch (error) {
      console.log(error);
    }
  };
};

I hope it was plain enough.

Helllo @omern92 Using firebase instance directly might not be a good strategy going ahead, instead you should firestore instance in props as a parameter to your action creator which is signInAsync.
Declare your action creator as such. NOTE : (creds, props) instead of (creds)

export const signInAsync = (creds, props) => {
  return async (dispatch) => {
    try {
      await props.firebase.auth().signInWithEmailAndPassword(creds.email, creds.password);
      dispatch(closeModal());

    } catch (error) {
      console.log(error);
    }
  };
};

Now you can pass firebase as a prop using 2 ways from your component

  1. use withFirestore HOC which will automatically add firebase in the props of your component.
  2. or you can if you are using the next version of react-redux-firebase i.e. react-redux-firebase@next then you use useFirebase Hook instead and get firebase instance and then pass it during dispatch.
const firebase = useFirebase();
dispatch(signInAsync(creds, {firebase})

@thibautvdu
Copy link

thibautvdu commented Jun 3, 2019

I'm currently facing the same issue, except that I would like to access to my firestore instance to pre-fetch data with SSR.

The current v3 documentation seems outdated as it suggests to use store.firebase which isn't available anymore.

useFirestore() or useFirebase() cannot be used either as hooks aren't allowed outside of the rendering functions. I'm looking to use it inside getInitialProps (Next.js)

What would be a decent solution to access to it from server to pre-fetch data ? For redux, a wrapper can set the store in this function's context, like this :

import React from 'react'
import firebase from 'firebase/app'
import 'firebase/auth'
import 'firebase/firestore' // make sure you add this for firestore
import { initializeStore } from '../redux/store'
import { useFirebase } from 'react-redux-firebase'

const isServer = typeof window === 'undefined'
const __NEXT_REDUX_STORE__ = '__NEXT_REDUX_STORE__'

const initFirebase = (appName = '[DEFAULT]') => {
  if (firebase.apps.find(app => app.name === appName)) {
    return
  }

  firebase.initializeApp({
****************
  }, appName)

  firebase.firestore() // <- needed if using firestore
}


function getOrCreateStore (initialState) {
  // Always make a new store if server, otherwise state is shared between requests
  if (isServer) {
    initFirebase();
    return initializeStore(initialState)
  }

  // Create store if unavailable on the client and set it on the window object
  if (!window[__NEXT_REDUX_STORE__]) {
    initFirebase();
    window[__NEXT_REDUX_STORE__] = initializeStore(initialState)
  }
  return window[__NEXT_REDUX_STORE__]
}

export default App => {
  return class AppWithRedux extends React.Component {
    static async getInitialProps (appContext) {
      // Get or Create the store with `undefined` as initialState
      // This allows you to set a custom default initialState
      const reduxStore = getOrCreateStore()

      // Provide the store to getInitialProps of pages
      appContext.ctx.reduxStore = reduxStore

      let pageProps = {}
      console.log("with-redux-store before");

      if (typeof App.getInitialProps === 'function') {
        pageProps = await App.getInitialProps(appContext)
        console.log("with-redux-store, pageProps: ",pageProps);
      }

      return {
        ...pageProps,
        initialReduxState: reduxStore.getState()
      }
    }

    constructor (props) {
      super(props)
      this.reduxStore = getOrCreateStore(props.initialReduxState)
    }

    render () {
      return <App {...this.props} reduxStore={this.reduxStore} />
    }
  }
}

@rijkerd
Copy link

rijkerd commented Jun 12, 2019

If getFirebase is removed, please recommend a way to use firebase on thunk.

@adjeteyboy
Copy link

hi, is there a way to wait for firebase for the auth to be ready in hooks and functional components

@prvnbist
Copy link

With Thunk

actionCreator.js

export const doSometing = (props, data) => {
    return (dispatch, getState) => {    // We get these from thunk
        props.firestore.collection('dataStore').add({
            ...data
        }).then(() => {
            dispatch({ type: 'DONE_SUCCESS', data });
        }).catch((err) => {
            dispatch({ type: 'DONE_FAILED', err });
        })
    }
}

props doesnt have a firestore method. Even the ownProps in the mapDispatchToProps doesn't have a firestore method.

@prescottprue
Copy link
Owner

To address this I have been considering adding back the reactReduxFirebase store enhancer to the v3 api. We will just need to make sure that the enhanced firebase instance that is created is shared with ReactReduxFirebaseProvider so that the listener tracking works across the two. That said, it is most likely that the internal API will still use the context created by ReactReduxFirebaseProvider instead of store.firebase.

prescottprue pushed a commit that referenced this issue Sep 5, 2019
@prescottprue prescottprue mentioned this issue Sep 5, 2019
3 tasks
prescottprue added a commit that referenced this issue Sep 6, 2019
* feat(auth): remove `signIn` option from createUser (new user is automatically signed in through Firebase SDK) - #513
* feat(core): new pattern for getting extended firebase instance in thunks (added back `getFirebase` to api) - #635
* fix(HOCs): switch to `UNSAFE_componentWillReceiveProps` in class based HOCs to prevent warnings with 16.9.0 - #755
* fix(HOCs): switch `withFirebase` and `withFirestore` back to pre-hooks compatible logic
* fix(core): replace lodash methods such as `isArray`, `isBoolean`, `isString`, `size`, `compact` and `isFunction` with native methods in a number of places
* chore(deps): update lodash to 4.17.15
* chore(docs): add docs for how to reference data from state for reselect selectors - #614
* chore(docs): update client side role assign example in roles recipes - #699
* chore(docs): add example for assigning role in cloud function - #699
@prescottprue
Copy link
Owner

The v3.0.0-beta release includes adding back getFirebase. It works directly with createFirebaseInstance instead of with the old reactReduxFirebase /store.firebase method. The extended firebase instance should be shared as expected as far as config and listeners (at least it was while testing).

I also was able to run my own thunks with the thunk.withExtraArgument setup and updated the docs to match.

Just wanted to say a huge thanks to everyone for all of the input - you all reaching out made it known that this needed to remain part of the API 😄 . As always, if it doesn't work as you expect, please reach out - it is hopefully just about ready to go to latest!

@daiky00
Copy link

daiky00 commented Oct 19, 2019

@prescottprue what do you mean with it works directly with createFirebaseInstance instead of with the old reactReduxFirebase /store.firebase method how can we use it any documentation? any example project?

@prescottprue
Copy link
Owner

@daiky00 Yes, the examples have been updated and the material example uses thunk.

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

No branches or pull requests