Skip to content

Conversation

@martynovs
Copy link
Contributor

Description

Extract initialization to separate module will allow to configure more than one firebase app and use externally loaded firebase (from firebase hosting, not from npm).
Also library will not dictate to use it's compose, in my case I use compose from redux dev tools.

store configuration may look like this:

import firebaseInit from 'react-redux-firebase/src/init';

function attachFirebaseToStore(store, config, name) {
  const app = firebase.initializeApp(config, name);
  const instance = firebaseInit(app, config, store.dispatch);
  store.firebase = instance;
  return instance;
}

Questions

  • Will a new version need to be released or is this a docs change? YES
  • Which version should this be published as a part of? ASAP
  • Does this impact the external API? YES
  • Will it need to be a breaking change? NO

Check List

  • All tests passing
  • Docs updated with any changes or examples
  • Added tests to ensure feature(s) work properly

Relevant Issues

@codecov
Copy link

codecov bot commented May 31, 2017

Codecov Report

❗ No coverage uploaded for pull request base (v1.5.0@63587ec). Click here to learn what that means.
The diff coverage is 92.92%.

@@            Coverage Diff            @@
##             v1.5.0     #150   +/-   ##
=========================================
  Coverage          ?   86.32%           
=========================================
  Files             ?       18           
  Lines             ?     1485           
  Branches          ?      244           
=========================================
  Hits              ?     1282           
  Misses            ?      203           
  Partials          ?        0

@prescottprue
Copy link
Owner

Nice work! I have been wanting to do this for a while now. Going to test it out.

Copy link
Owner

@prescottprue prescottprue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks awesome, just some small changes

src/init.js Outdated
import { isObject } from 'lodash'
import { authActions, queryActions, storageActions } from './actions'

export default function init (firebase, config, dispatch) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to see this as a const set to a function instead of a named function to limit scope unless there is a reason it is being named:

export default (firebase, config, dispatch) => {

@@ -0,0 +1,346 @@
import { isObject } from 'lodash'
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might help for clarity to call this file "createFirebaseInstance" or something along those lines.

@martynovs
Copy link
Contributor Author

I've applied your changes

@prescottprue prescottprue changed the base branch from master to v1.5.0 May 31, 2017 21:33
@prescottprue
Copy link
Owner

I believe the branch was made from master instead of v1.5.0.

Going to make similar changes and attempt to resolve conflicts.

@prescottprue
Copy link
Owner

Similar has been done and released in v2.0.0-alpha.

Hopefully will soon get time to try to get it into a v1.*.* version.

@prescottprue prescottprue reopened this Jun 22, 2017
@martynovs martynovs deleted the patch-2 branch June 22, 2017 02:45
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.

2 participants