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(firebase): support lazy loading and modular loading of Firebase #250

Open
nicolasgarnier opened this Issue Aug 23, 2017 · 21 comments

Comments

3 participants
@nicolasgarnier

nicolasgarnier commented Aug 23, 2017

In general it would be nice if react-redux-firebase was compatible with lazy-loading or at least modular loading of the Firebase library which is rather large.

Use case is: What if I want to only use firebase auth (and not Firebase database) with react-redux-firebase?

In my code, if I only want to use Firebase auth (and not Firebase database, Firebase storage etc...) I would load the SDK like this:

import firebase from 'firebase/app';
import 'firebase/auth';

In general, the best would be that react-redux-firebase not depend on importing the firebase SDK but only rely on being passed a Firebase app instance and would simply test if firebase features are loaded - e.g. if(firebase.database){/*load firebase database middleware*/} - to activate parts of the middleware (or have multiple middlewares, one for each features).

@prescottprue prescottprue self-assigned this Aug 23, 2017

@prescottprue prescottprue added this to the v2.0.0 milestone Aug 23, 2017

@prescottprue

This comment has been minimized.

Show comment
Hide comment
@prescottprue

prescottprue Aug 24, 2017

Owner

@nicolasgarnier This is a really interesting thought. Going to look into adding it.

Owner

prescottprue commented Aug 24, 2017

@nicolasgarnier This is a really interesting thought. Going to look into adding it.

prescottprue added a commit that referenced this issue Aug 24, 2017

Support lazy/module loading - #249, #250
* Only existing parts of Firebase library are called to support
lazy/module loading
* enableRedirectHandling no longer needs to be set as false if not in
http environment such as SSR (firebase redirect only works if
location.protocol is http or https)

@prescottprue prescottprue referenced this issue Aug 24, 2017

Merged

v2.0.0 beta.7 #252

3 of 3 tasks complete

prescottprue added a commit that referenced this issue Aug 24, 2017

v2.0.0 beta.7 (#252)
* fix(reducer): allow setting paths ending in numbers - #248
* feat(auth): make signInWithCredential not dependent on provider - #247
* `signInWithCustomToken` no longer decodes token internally for profile data (`profileFactory` should be used) - #244
* Removed `jwt-decode` as a dependency (not needed since auth token no longer decoded internally)
* Support lazy/module loading - #249, #250 
* Updated SSR docs to include notes about `enableRedirectHanlding: false` - #251
* Updated Roadmap with detection of non-http environments
@prescottprue

This comment has been minimized.

Show comment
Hide comment
@prescottprue

prescottprue Aug 24, 2017

Owner

v2.0.0-beta.7 includes a first swing at supporting passing just the app instance.

Let me know if it does not handle your use case or work for you as expected.

It uses extendApp, which I didn't know about until today. Not sure if that is the best approach or not so totally open to other implementations.

Owner

prescottprue commented Aug 24, 2017

v2.0.0-beta.7 includes a first swing at supporting passing just the app instance.

Let me know if it does not handle your use case or work for you as expected.

It uses extendApp, which I didn't know about until today. Not sure if that is the best approach or not so totally open to other implementations.

@nicolasgarnier

This comment has been minimized.

Show comment
Hide comment
@nicolasgarnier

nicolasgarnier Aug 24, 2017

Thanks! I'll have a look asap and let you know :)

nicolasgarnier commented Aug 24, 2017

Thanks! I'll have a look asap and let you know :)

@nicolasgarnier

This comment has been minimized.

Show comment
Hide comment
@nicolasgarnier

nicolasgarnier Aug 24, 2017

Unfortunately this did not work for me.

I tried using react-redux-firebase with only Firebase auth loaded (and not firebase database).

Usecase: Let's say that hypothetically I am not interested in database but only Firebase auth state event or I want to load firebase/auth first, so that the sign-in pages load faster and then delay loading the firebase/database.

I am getting this error:

error: /Users/nivco/Documents/workspace/friendlypix-web-react/node_modules/react-redux-firebase/lib/createFirebaseInstance.js:30
      database: { ServerValue: firebase.firebase_.database.ServerValue }
                                                          ^

TypeError: Cannot read property 'ServerValue' of undefined
    at createFirebaseInstance (/Users/nivco/Documents/workspace/friendlypix-web-react/node_modules/react-redux-firebase/lib/createFirebaseInstance.js:30:59)
    at /Users/nivco/Documents/workspace/friendlypix-web-react/node_modules/react-redux-firebase/lib/compose.js:28:77
    at /Users/nivco/Documents/workspace/friendlypix-web-react/node_modules/redux/lib/applyMiddleware.js:38:19
    at /Users/nivco/Documents/workspace/friendlypix-web-react/node_modules/redux/lib/applyMiddleware.js:38:19
    at createStore (/Users/nivco/Documents/workspace/friendlypix-web-react/node_modules/redux/lib/createStore.js:65:33)
    at Object.<anonymous> (/Users/nivco/Documents/workspace/friendlypix-web-react/frontend/App.jsx:35:22)
    at Module._compile (module.js:570:32)
    at loader (/Users/nivco/Documents/workspace/friendlypix-web-react/node_modules/babel-register/lib/node.js:144:5)
    at Object.require.extensions.(anonymous function) [as .jsx] (/Users/nivco/Documents/workspace/friendlypix-web-react/node_modules/babel-register/lib/node.js:154:7)
    at Module.load (module.js:487:32)

Here is some code to replicate:

import firebase from 'firebase/app';
import 'firebase/auth'

// ...

const firebaseApp = firebase.initializeApp(firebaseConfig);

export const store = createStore(
  combineReducers({
    ...reducers,
    firebaseState: firebaseStateReducer
  }),
  initialState,
  compose(
    applyMiddleware(thunk.withExtraArgument(getFirebase)),
    reactReduxFirebase(firebaseApp, {enableRedirectHandling: false})
  )
);

Ideally, this should work. Either automatically or by passing an explicit option to disable database.

Bonus point if the Firebase database integration can start working again if I lazy load firebase/database later on.

I think this is important going forward to save on build size. And in case more features are added to Firebase in the future which would be a good fit for this library :)

nicolasgarnier commented Aug 24, 2017

Unfortunately this did not work for me.

I tried using react-redux-firebase with only Firebase auth loaded (and not firebase database).

Usecase: Let's say that hypothetically I am not interested in database but only Firebase auth state event or I want to load firebase/auth first, so that the sign-in pages load faster and then delay loading the firebase/database.

I am getting this error:

error: /Users/nivco/Documents/workspace/friendlypix-web-react/node_modules/react-redux-firebase/lib/createFirebaseInstance.js:30
      database: { ServerValue: firebase.firebase_.database.ServerValue }
                                                          ^

TypeError: Cannot read property 'ServerValue' of undefined
    at createFirebaseInstance (/Users/nivco/Documents/workspace/friendlypix-web-react/node_modules/react-redux-firebase/lib/createFirebaseInstance.js:30:59)
    at /Users/nivco/Documents/workspace/friendlypix-web-react/node_modules/react-redux-firebase/lib/compose.js:28:77
    at /Users/nivco/Documents/workspace/friendlypix-web-react/node_modules/redux/lib/applyMiddleware.js:38:19
    at /Users/nivco/Documents/workspace/friendlypix-web-react/node_modules/redux/lib/applyMiddleware.js:38:19
    at createStore (/Users/nivco/Documents/workspace/friendlypix-web-react/node_modules/redux/lib/createStore.js:65:33)
    at Object.<anonymous> (/Users/nivco/Documents/workspace/friendlypix-web-react/frontend/App.jsx:35:22)
    at Module._compile (module.js:570:32)
    at loader (/Users/nivco/Documents/workspace/friendlypix-web-react/node_modules/babel-register/lib/node.js:144:5)
    at Object.require.extensions.(anonymous function) [as .jsx] (/Users/nivco/Documents/workspace/friendlypix-web-react/node_modules/babel-register/lib/node.js:154:7)
    at Module.load (module.js:487:32)

Here is some code to replicate:

import firebase from 'firebase/app';
import 'firebase/auth'

// ...

const firebaseApp = firebase.initializeApp(firebaseConfig);

export const store = createStore(
  combineReducers({
    ...reducers,
    firebaseState: firebaseStateReducer
  }),
  initialState,
  compose(
    applyMiddleware(thunk.withExtraArgument(getFirebase)),
    reactReduxFirebase(firebaseApp, {enableRedirectHandling: false})
  )
);

Ideally, this should work. Either automatically or by passing an explicit option to disable database.

Bonus point if the Firebase database integration can start working again if I lazy load firebase/database later on.

I think this is important going forward to save on build size. And in case more features are added to Firebase in the future which would be a good fit for this library :)

@prescottprue

This comment has been minimized.

Show comment
Hide comment
@prescottprue

prescottprue Aug 27, 2017

Owner

@nicolasgarnier Is there documentation other than the API reference for how best to use the app instance?

I am having trouble figuring out the best pattern to do this. When passing around the app instance instead of the whole firebase library, firebase.database.ServerValue does not work as expected (firebase.database is only a function, the object/getter is not available on the app instance).

For now, I removed the usage of firebase.database.ServerValue in the extendApp call, and check for existence before using it.

Owner

prescottprue commented Aug 27, 2017

@nicolasgarnier Is there documentation other than the API reference for how best to use the app instance?

I am having trouble figuring out the best pattern to do this. When passing around the app instance instead of the whole firebase library, firebase.database.ServerValue does not work as expected (firebase.database is only a function, the object/getter is not available on the app instance).

For now, I removed the usage of firebase.database.ServerValue in the extendApp call, and check for existence before using it.

@nicolasgarnier

This comment has been minimized.

Show comment
Hide comment
@nicolasgarnier

nicolasgarnier Aug 28, 2017

Hey @prescottprue

As you've noticed the Constants (like ServerValue...) are not part of a Firebase App instance. They need to be referred directly with the firebase namespace e.g. firebase.database.ServerValue.

I'm thinking for the purpose of allowing lazy loading, the only way we could do this is:

// Import just `firebase/app` to allow the developer to selectively load desired components.
import firebase from 'firebase/app';

// When `firebase.database` is not undefined this means that  'firebase/database' has been loaded and the database stuff is available.
if (firebase.database) {
  // this will always work
  console.log(firebase.database.ServerValue.TIMESTAMP);
}

But actually you can test for either firebase.database (on the SDK if you needed to import 'firebase/app') or firebaseApp.database (on the firebase App instance) both should work the same to detect that the database SDK has been loaded.

Also would be good to rename the firebase variable to firebaseApp when it is the App instance you are passing around so that it's more clear it's not the firebase SDK.

nicolasgarnier commented Aug 28, 2017

Hey @prescottprue

As you've noticed the Constants (like ServerValue...) are not part of a Firebase App instance. They need to be referred directly with the firebase namespace e.g. firebase.database.ServerValue.

I'm thinking for the purpose of allowing lazy loading, the only way we could do this is:

// Import just `firebase/app` to allow the developer to selectively load desired components.
import firebase from 'firebase/app';

// When `firebase.database` is not undefined this means that  'firebase/database' has been loaded and the database stuff is available.
if (firebase.database) {
  // this will always work
  console.log(firebase.database.ServerValue.TIMESTAMP);
}

But actually you can test for either firebase.database (on the SDK if you needed to import 'firebase/app') or firebaseApp.database (on the firebase App instance) both should work the same to detect that the database SDK has been loaded.

Also would be good to rename the firebase variable to firebaseApp when it is the App instance you are passing around so that it's more clear it's not the firebase SDK.

@nicolasgarnier

This comment has been minimized.

Show comment
Hide comment
@nicolasgarnier

nicolasgarnier Aug 28, 2017

By the way could you send me an email (email is on my profile), there are some non-public things I'd like to talk to you about :)

nicolasgarnier commented Aug 28, 2017

By the way could you send me an email (email is on my profile), there are some non-public things I'd like to talk to you about :)

@prescottprue

This comment has been minimized.

Show comment
Hide comment
@prescottprue

prescottprue Aug 29, 2017

Owner

@nicolasgarnier Reached out over email :)

Also, just wanted to mention:
The v2.0.0-beta.8 branch includes changes that should fix the TypeError: Cannot read property 'ServerValue' of undefined issue you were seeing. Still not sure everything works 100% perfect when passing the app instance, but I have started expanding the tests to cover it.

Owner

prescottprue commented Aug 29, 2017

@nicolasgarnier Reached out over email :)

Also, just wanted to mention:
The v2.0.0-beta.8 branch includes changes that should fix the TypeError: Cannot read property 'ServerValue' of undefined issue you were seeing. Still not sure everything works 100% perfect when passing the app instance, but I have started expanding the tests to cover it.

@nicolasgarnier

This comment has been minimized.

Show comment
Hide comment
@nicolasgarnier

nicolasgarnier Aug 31, 2017

Cool :) as soon as it's on npm I'll give it a try :)

nicolasgarnier commented Aug 31, 2017

Cool :) as soon as it's on npm I'll give it a try :)

@nicolasgarnier

This comment has been minimized.

Show comment
Hide comment
@nicolasgarnier

nicolasgarnier Aug 31, 2017

FYI in beta 7 I'm also facing another bug when I pass a FirebaseApp instance instead of the firebase lib:

info: There was an error TypeError: Cannot read property 'bind' of undefined
    at Object.FirebaseAppImpl.(anonymous function) [as auth] (/Users/nivco/Documents/workspace/friendlypix-web-react/node_modules/firebase/src/app/firebase_app.ts:564:4)
    at SplashPage.render (/Users/nivco/Documents/workspace/friendlypix-web-react/frontend/components/SplashPage.jsx:70:114)
    at /Users/nivco/Documents/workspace/friendlypix-web-react/node_modules/react-dom/lib/ReactCompositeComponent.js:795:21
    at measureLifeCyclePerf (/Users/nivco/Documents/workspace/friendlypix-web-react/node_modules/react-dom/lib/ReactCompositeComponent.js:75:12)
    at ReactCompositeComponentWrapper._renderValidatedComponentWithoutOwnerOrContext (/Users/nivco/Documents/workspace/friendlypix-web-react/node_modules/react-dom/lib/ReactCompositeComponent.js:794:25)
    at ReactCompositeComponentWrapper._renderValidatedComponent (/Users/nivco/Documents/workspace/friendlypix-web-react/node_modules/react-dom/lib/ReactCompositeComponent.js:821:32)
    at ReactCompositeComponentWrapper.performInitialMount (/Users/nivco/Documents/workspace/friendlypix-web-react/node_modules/react-dom/lib/ReactCompositeComponent.js:361:30)
    at ReactCompositeComponentWrapper.mountComponent (/Users/nivco/Documents/workspace/friendlypix-web-react/node_modules/react-dom/lib/ReactCompositeComponent.js:257:21)
    at Object.mountComponent (/Users/nivco/Documents/workspace/friendlypix-web-react/node_modules/react-dom/lib/ReactReconciler.js:45:35)
    at ReactCompositeComponentWrapper.performInitialMount (/Users/nivco/Documents/workspace/friendlypix-web-react/node_modules/react-dom/lib/ReactCompositeComponent.js:370:34)

I do not get the issue when I pass the firebase SDK.

In that componant I'm using the HOC:

export default compose(firebaseConnect(), connect(mapStateToProps, mapDispatchToProps))(SplashPage);

and then in the render method I'm trying to access the firebaseApp isntance:

render() {
    return (
      // ...
          <FirebaseAuth className={styles.firebaseui} uiConfig={this.uiConfig} firebaseAuth={this.props.firebase.auth()}/>
      // ...

Strange since this.props.firebase.auth() should work on a FirebaseApp instance...

nicolasgarnier commented Aug 31, 2017

FYI in beta 7 I'm also facing another bug when I pass a FirebaseApp instance instead of the firebase lib:

info: There was an error TypeError: Cannot read property 'bind' of undefined
    at Object.FirebaseAppImpl.(anonymous function) [as auth] (/Users/nivco/Documents/workspace/friendlypix-web-react/node_modules/firebase/src/app/firebase_app.ts:564:4)
    at SplashPage.render (/Users/nivco/Documents/workspace/friendlypix-web-react/frontend/components/SplashPage.jsx:70:114)
    at /Users/nivco/Documents/workspace/friendlypix-web-react/node_modules/react-dom/lib/ReactCompositeComponent.js:795:21
    at measureLifeCyclePerf (/Users/nivco/Documents/workspace/friendlypix-web-react/node_modules/react-dom/lib/ReactCompositeComponent.js:75:12)
    at ReactCompositeComponentWrapper._renderValidatedComponentWithoutOwnerOrContext (/Users/nivco/Documents/workspace/friendlypix-web-react/node_modules/react-dom/lib/ReactCompositeComponent.js:794:25)
    at ReactCompositeComponentWrapper._renderValidatedComponent (/Users/nivco/Documents/workspace/friendlypix-web-react/node_modules/react-dom/lib/ReactCompositeComponent.js:821:32)
    at ReactCompositeComponentWrapper.performInitialMount (/Users/nivco/Documents/workspace/friendlypix-web-react/node_modules/react-dom/lib/ReactCompositeComponent.js:361:30)
    at ReactCompositeComponentWrapper.mountComponent (/Users/nivco/Documents/workspace/friendlypix-web-react/node_modules/react-dom/lib/ReactCompositeComponent.js:257:21)
    at Object.mountComponent (/Users/nivco/Documents/workspace/friendlypix-web-react/node_modules/react-dom/lib/ReactReconciler.js:45:35)
    at ReactCompositeComponentWrapper.performInitialMount (/Users/nivco/Documents/workspace/friendlypix-web-react/node_modules/react-dom/lib/ReactCompositeComponent.js:370:34)

I do not get the issue when I pass the firebase SDK.

In that componant I'm using the HOC:

export default compose(firebaseConnect(), connect(mapStateToProps, mapDispatchToProps))(SplashPage);

and then in the render method I'm trying to access the firebaseApp isntance:

render() {
    return (
      // ...
          <FirebaseAuth className={styles.firebaseui} uiConfig={this.uiConfig} firebaseAuth={this.props.firebase.auth()}/>
      // ...

Strange since this.props.firebase.auth() should work on a FirebaseApp instance...

@prescottprue

This comment has been minimized.

Show comment
Hide comment
@prescottprue

prescottprue Aug 31, 2017

Owner

@nicolasgarnier Good to know. Did you get a chance to try out the v2.0.0-beta.8 branch? Hoping to get this ironed out before getting into RC.

Owner

prescottprue commented Aug 31, 2017

@nicolasgarnier Good to know. Did you get a chance to try out the v2.0.0-beta.8 branch? Hoping to get this ironed out before getting into RC.

@nicolasgarnier

This comment has been minimized.

Show comment
Hide comment
@nicolasgarnier

nicolasgarnier Aug 31, 2017

What's the easiest way to test the v2.0.0-beta.8 branch?

npm run prepublish and then use whatever is in /lib ?

nicolasgarnier commented Aug 31, 2017

What's the easiest way to test the v2.0.0-beta.8 branch?

npm run prepublish and then use whatever is in /lib ?

@prescottprue

This comment has been minimized.

Show comment
Hide comment
@prescottprue

prescottprue Aug 31, 2017

Owner

@nicolasgarnier the steps in npm-linking section of the contributing guide is usually what I suggest.

It will make it so with any project you want to test with you can run the v2.0.0-beta.8 branch logic.

Owner

prescottprue commented Aug 31, 2017

@nicolasgarnier the steps in npm-linking section of the contributing guide is usually what I suggest.

It will make it so with any project you want to test with you can run the v2.0.0-beta.8 branch logic.

@nicolasgarnier

This comment has been minimized.

Show comment
Hide comment
@nicolasgarnier

nicolasgarnier Aug 31, 2017

On beta.8 I'm still getting the error above:

info: There was an error TypeError: Cannot read property 'bind' of undefined
    at Object.FirebaseAppImpl.(anonymous function) [as auth] (/Users/nivco/Documents/workspace/friendlypix-web-react/node_modules/firebase/src/app/firebase_app.ts:564:4)
    at SplashPage.render (/Users/nivco/Documents/workspace/friendlypix-web-react/frontend/components/SplashPage.jsx:70:114)
    at /Users/nivco/Documents/workspace/friendlypix-web-react/node_modules/react-dom/lib/ReactCompositeComponent.js:795:21
    at measureLifeCyclePerf (/Users/nivco/Documents/workspace/friendlypix-web-react/node_modules/react-dom/lib/ReactCompositeComponent.js:75:12)
    at ReactCompositeComponentWrapper._renderValidatedComponentWithoutOwnerOrContext (/Users/nivco/Documents/workspace/friendlypix-web-react/node_modules/react-dom/lib/ReactCompositeComponent.js:794:25)
    at ReactCompositeComponentWrapper._renderValidatedComponent (/Users/nivco/Documents/workspace/friendlypix-web-react/node_modules/react-dom/lib/ReactCompositeComponent.js:821:32)
    at ReactCompositeComponentWrapper.performInitialMount (/Users/nivco/Documents/workspace/friendlypix-web-react/node_modules/react-dom/lib/ReactCompositeComponent.js:361:30)
    at ReactCompositeComponentWrapper.mountComponent (/Users/nivco/Documents/workspace/friendlypix-web-react/node_modules/react-dom/lib/ReactCompositeComponent.js:257:21)
    at Object.mountComponent (/Users/nivco/Documents/workspace/friendlypix-web-react/node_modules/react-dom/lib/ReactReconciler.js:45:35)
    at ReactCompositeComponentWrapper.performInitialMount (/Users/nivco/Documents/workspace/friendlypix-web-react/node_modules/react-dom/lib/ReactCompositeComponent.js:370:34)

nicolasgarnier commented Aug 31, 2017

On beta.8 I'm still getting the error above:

info: There was an error TypeError: Cannot read property 'bind' of undefined
    at Object.FirebaseAppImpl.(anonymous function) [as auth] (/Users/nivco/Documents/workspace/friendlypix-web-react/node_modules/firebase/src/app/firebase_app.ts:564:4)
    at SplashPage.render (/Users/nivco/Documents/workspace/friendlypix-web-react/frontend/components/SplashPage.jsx:70:114)
    at /Users/nivco/Documents/workspace/friendlypix-web-react/node_modules/react-dom/lib/ReactCompositeComponent.js:795:21
    at measureLifeCyclePerf (/Users/nivco/Documents/workspace/friendlypix-web-react/node_modules/react-dom/lib/ReactCompositeComponent.js:75:12)
    at ReactCompositeComponentWrapper._renderValidatedComponentWithoutOwnerOrContext (/Users/nivco/Documents/workspace/friendlypix-web-react/node_modules/react-dom/lib/ReactCompositeComponent.js:794:25)
    at ReactCompositeComponentWrapper._renderValidatedComponent (/Users/nivco/Documents/workspace/friendlypix-web-react/node_modules/react-dom/lib/ReactCompositeComponent.js:821:32)
    at ReactCompositeComponentWrapper.performInitialMount (/Users/nivco/Documents/workspace/friendlypix-web-react/node_modules/react-dom/lib/ReactCompositeComponent.js:361:30)
    at ReactCompositeComponentWrapper.mountComponent (/Users/nivco/Documents/workspace/friendlypix-web-react/node_modules/react-dom/lib/ReactCompositeComponent.js:257:21)
    at Object.mountComponent (/Users/nivco/Documents/workspace/friendlypix-web-react/node_modules/react-dom/lib/ReactReconciler.js:45:35)
    at ReactCompositeComponentWrapper.performInitialMount (/Users/nivco/Documents/workspace/friendlypix-web-react/node_modules/react-dom/lib/ReactCompositeComponent.js:370:34)

prescottprue added a commit that referenced this issue Sep 1, 2017

Use internal firebase_ if app instance is passed
* Use internal firebase_ if app instance is passed - #250
* Remove extendApp functionality
@prescottprue

This comment has been minimized.

Show comment
Hide comment
@prescottprue

prescottprue Sep 1, 2017

Owner

@nicolasgarnier I found out that the app instance has firebase_ which has everything that is needed. Using that, I was able to remove the extendApp logic from before.

I was able to get the material example working completely with passing in just an App instance even when importing one service at a time. Also added some error catching to make things more clear.

These updates have been pushed to the v2.0.0-beta.8 branch. There are some other things that need to get released, so I am most likely going to publish in the day or so regardless of status on this issue.

Owner

prescottprue commented Sep 1, 2017

@nicolasgarnier I found out that the app instance has firebase_ which has everything that is needed. Using that, I was able to remove the extendApp logic from before.

I was able to get the material example working completely with passing in just an App instance even when importing one service at a time. Also added some error catching to make things more clear.

These updates have been pushed to the v2.0.0-beta.8 branch. There are some other things that need to get released, so I am most likely going to publish in the day or so regardless of status on this issue.

@prescottprue prescottprue referenced this issue Sep 1, 2017

Merged

v2.0.0-beta.8 #263

3 of 3 tasks complete
@nicolasgarnier

This comment has been minimized.

Show comment
Hide comment
@nicolasgarnier

nicolasgarnier Sep 1, 2017

Cool. We're getting a bit further now and I'm now getting this issue:

info: There was an error { [DEFAULT]: Firebase: No Firebase App '[DEFAULT]' has been created - call Firebase App.initializeApp() (app/no-app).
    at error (/Users/nivco/Documents/workspace/friendlypix-web-react/node_modules/firebase/src/app/firebase_app.ts:620:42)
    at app (/Users/nivco/Documents/workspace/friendlypix-web-react/node_modules/firebase/src/app/firebase_app.ts:468:3)
    at Object.serviceNamespace [as auth] (/Users/nivco/Documents/workspace/friendlypix-web-react/node_modules/firebase/src/app/firebase_app.ts:541:9)
    at Object.init (/Users/nivco/Documents/workspace/react-redux-firebase/lib/actions/auth.js:161:12)
    at /Users/nivco/Documents/workspace/react-redux-firebase/lib/compose.js:30:28
    at /Users/nivco/Documents/workspace/friendlypix-web-react/node_modules/redux/lib/applyMiddleware.js:38:19
    at /Users/nivco/Documents/workspace/friendlypix-web-react/node_modules/redux/lib/applyMiddleware.js:38:19
    at createStore (/Users/nivco/Documents/workspace/friendlypix-web-react/node_modules/redux/lib/createStore.js:65:33)
    at Object.makeStore (/Users/nivco/Documents/workspace/friendlypix-web-react/frontend/App.jsx:29:10)
    at createFirebaseAppWithSignedInUser.then.firebaseApp (/Users/nivco/Documents/workspace/friendlypix-web-react/microservices/renderTemplate.js:50:25)
  code: 'app/no-app',
  message: 'Firebase: No Firebase App \'[DEFAULT]\' has been created - call Firebase App.initializeApp() (app/no-app).',
  name: '[DEFAULT]' }

How to repro: I did not create a "default app" instead I created a "named app like this:

import firebase from 'firebase/app';
import 'firebase/auth'

// ...

const firebaseApp = firebase.initializeApp(firebaseConfig, 'myName'); // Named app here!!

export const store = createStore(
  combineReducers({
    ...reducers,
    firebaseState: firebaseStateReducer
  }),
  initialState,
  compose(
    applyMiddleware(thunk.withExtraArgument(getFirebase)),
    reactReduxFirebase(firebaseApp, {enableRedirectHandling: false})
  )
);

It looks like somewhere in your code you tried to use the firebase SDK to get the "default" app like firebase.app() or firebase.auth() would do that. If you do this calls on the Firebase App instance that was passed it would work (e.g. firebaseApp.auth()).

for instance if I look at your code here: https://github.com/prescottprue/react-redux-firebase/blob/v2.0.0-beta.8/src/actions/auth.js#L144-L149

const setupPresence = (dispatch, firebase) => {
  // exit if database does not exist on firebase instance
  if (!firebase.database || !firebase.database.ServerValue) {
    return
  }
  const ref = firebase.database().ref()
  // ...

Looks like indeed you are passing the firebase SDK pointer around rather than the app instance. Maybe this piece of code should look more like this:

const setupPresence = (dispatch, firebaseApp) => {
  // exit if database does not exist on firebase instance
  if (!firebaseApp.firebase_.database || !firebaseApp.firebase_.database.ServerValue) {
    return
  }
  const ref = firebaseApp.database().ref()
  // ...

or do this:

import firebase from 'firebase/app';

// ...

const setupPresence = (dispatch, firebaseApp) => {
  // exit if database does not exist on firebase instance
  if (!firebase.database || !firebase.database.ServerValue) { // Here we test on the firebase SDK
    return
  }
  const ref = firebaseApp.database().ref() // Here we use the firebase App that is being passed arround
  // ...

I think it's important to re-use the passed firebase App whenever possible. For example here is a usecase where this matters:

I want to do authorized users on an isomorphic (SSR) app.
When doing server side rendering I can't safely use the same firebase app because there might be concurrent requests that are coming from different users. What I do is that I pass the user's ID token in a cookie and I try to sign these users in on the server. For this I have to use different named firebase app instances for each users like this:

const firebaseApp = firebase.initializeApp(firebaseConfig, uid); // The name of the app is the UID of the user.
// Now I sign in the user on firebaseApp.
firebaseApp.auth().signInWithCustomToken( // etc...

This way I'm not re-using the same Firebase instance for different users and I won't have issues with the auth states being mixed up on the server side. If I re-use the default app instance for instance, there might be some cases where 2 users get signed in the same default app at the same time and I'll be serving a user's data to 2 different users.

nicolasgarnier commented Sep 1, 2017

Cool. We're getting a bit further now and I'm now getting this issue:

info: There was an error { [DEFAULT]: Firebase: No Firebase App '[DEFAULT]' has been created - call Firebase App.initializeApp() (app/no-app).
    at error (/Users/nivco/Documents/workspace/friendlypix-web-react/node_modules/firebase/src/app/firebase_app.ts:620:42)
    at app (/Users/nivco/Documents/workspace/friendlypix-web-react/node_modules/firebase/src/app/firebase_app.ts:468:3)
    at Object.serviceNamespace [as auth] (/Users/nivco/Documents/workspace/friendlypix-web-react/node_modules/firebase/src/app/firebase_app.ts:541:9)
    at Object.init (/Users/nivco/Documents/workspace/react-redux-firebase/lib/actions/auth.js:161:12)
    at /Users/nivco/Documents/workspace/react-redux-firebase/lib/compose.js:30:28
    at /Users/nivco/Documents/workspace/friendlypix-web-react/node_modules/redux/lib/applyMiddleware.js:38:19
    at /Users/nivco/Documents/workspace/friendlypix-web-react/node_modules/redux/lib/applyMiddleware.js:38:19
    at createStore (/Users/nivco/Documents/workspace/friendlypix-web-react/node_modules/redux/lib/createStore.js:65:33)
    at Object.makeStore (/Users/nivco/Documents/workspace/friendlypix-web-react/frontend/App.jsx:29:10)
    at createFirebaseAppWithSignedInUser.then.firebaseApp (/Users/nivco/Documents/workspace/friendlypix-web-react/microservices/renderTemplate.js:50:25)
  code: 'app/no-app',
  message: 'Firebase: No Firebase App \'[DEFAULT]\' has been created - call Firebase App.initializeApp() (app/no-app).',
  name: '[DEFAULT]' }

How to repro: I did not create a "default app" instead I created a "named app like this:

import firebase from 'firebase/app';
import 'firebase/auth'

// ...

const firebaseApp = firebase.initializeApp(firebaseConfig, 'myName'); // Named app here!!

export const store = createStore(
  combineReducers({
    ...reducers,
    firebaseState: firebaseStateReducer
  }),
  initialState,
  compose(
    applyMiddleware(thunk.withExtraArgument(getFirebase)),
    reactReduxFirebase(firebaseApp, {enableRedirectHandling: false})
  )
);

It looks like somewhere in your code you tried to use the firebase SDK to get the "default" app like firebase.app() or firebase.auth() would do that. If you do this calls on the Firebase App instance that was passed it would work (e.g. firebaseApp.auth()).

for instance if I look at your code here: https://github.com/prescottprue/react-redux-firebase/blob/v2.0.0-beta.8/src/actions/auth.js#L144-L149

const setupPresence = (dispatch, firebase) => {
  // exit if database does not exist on firebase instance
  if (!firebase.database || !firebase.database.ServerValue) {
    return
  }
  const ref = firebase.database().ref()
  // ...

Looks like indeed you are passing the firebase SDK pointer around rather than the app instance. Maybe this piece of code should look more like this:

const setupPresence = (dispatch, firebaseApp) => {
  // exit if database does not exist on firebase instance
  if (!firebaseApp.firebase_.database || !firebaseApp.firebase_.database.ServerValue) {
    return
  }
  const ref = firebaseApp.database().ref()
  // ...

or do this:

import firebase from 'firebase/app';

// ...

const setupPresence = (dispatch, firebaseApp) => {
  // exit if database does not exist on firebase instance
  if (!firebase.database || !firebase.database.ServerValue) { // Here we test on the firebase SDK
    return
  }
  const ref = firebaseApp.database().ref() // Here we use the firebase App that is being passed arround
  // ...

I think it's important to re-use the passed firebase App whenever possible. For example here is a usecase where this matters:

I want to do authorized users on an isomorphic (SSR) app.
When doing server side rendering I can't safely use the same firebase app because there might be concurrent requests that are coming from different users. What I do is that I pass the user's ID token in a cookie and I try to sign these users in on the server. For this I have to use different named firebase app instances for each users like this:

const firebaseApp = firebase.initializeApp(firebaseConfig, uid); // The name of the app is the UID of the user.
// Now I sign in the user on firebaseApp.
firebaseApp.auth().signInWithCustomToken( // etc...

This way I'm not re-using the same Firebase instance for different users and I won't have issues with the auth states being mixed up on the server side. If I re-use the default app instance for instance, there might be some cases where 2 users get signed in the same default app at the same time and I'll be serving a user's data to 2 different users.

@prescottprue

This comment has been minimized.

Show comment
Hide comment
@prescottprue

prescottprue Sep 1, 2017

Owner

@nicolasgarnier Didn't realize named apps function differently, thanks for the explanation. Looking into that now.

It is great to know your intended use case.

Owner

prescottprue commented Sep 1, 2017

@nicolasgarnier Didn't realize named apps function differently, thanks for the explanation. Looking into that now.

It is great to know your intended use case.

prescottprue added a commit that referenced this issue Sep 2, 2017

v2.0.0-beta.8 (#263)
* fix(reducer): `MERGE` action added and `SET` action to reverted to v1 behavior - #255
* feat(populate): population of ordered data - #239
* feat(populate) populate once queries - #256
* feat(auth): emptyOnLogin config option added (defaults to `true`) - #254
* feat(query): emit `NO_VALUE` for `once` queries that are empty - #265
* fix(populate) Fixed populate function to return null for null paths
* app instance handling now uses `firebase_` instead of `extendApp` (more functionality) - #250
* Removed no longer in use code from v1 (validateConfig)

@prescottprue prescottprue changed the title from [v2] feature request: Make react-redux-firebase work with lazy loading and modular loading to [v2] feat(firebase): support lazy loading and modular loading of Firebase Sep 2, 2017

@prescottprue prescottprue changed the title from [v2] feat(firebase): support lazy loading and modular loading of Firebase to feat(firebase): support lazy loading and modular loading of Firebase Sep 2, 2017

@prescottprue prescottprue added this to To Do in v2.0.0 Oct 18, 2017

@prescottprue prescottprue removed their assignment Oct 18, 2017

@AVVS

This comment has been minimized.

Show comment
Hide comment
@AVVS

AVVS Oct 19, 2017

firestore in recent beta increases bundle size by 60KB gzip, so it would be great to see separation there as well

AVVS commented Oct 19, 2017

firestore in recent beta increases bundle size by 60KB gzip, so it would be great to see separation there as well

@prescottprue

This comment has been minimized.

Show comment
Hide comment
@prescottprue

prescottprue Oct 19, 2017

Owner

@AVVS Yup, I noticed that as well. Using babel-plugin-lodash I was able to shrink it a bit, but we may need to make it a peer dependency and have including it be part of the setup.

Thanks for the input!

Are you using the UMD bundle or the es/commonjs builds? Only the UMD should have been increased.

Owner

prescottprue commented Oct 19, 2017

@AVVS Yup, I noticed that as well. Using babel-plugin-lodash I was able to shrink it a bit, but we may need to make it a peer dependency and have including it be part of the setup.

Thanks for the input!

Are you using the UMD bundle or the es/commonjs builds? Only the UMD should have been increased.

@prescottprue

This comment has been minimized.

Show comment
Hide comment
@prescottprue

prescottprue Oct 19, 2017

Owner

@AVVS I have come up with a way to have redux-firestore be an optional dependency instead of a full dependency. This way it is up to the end user to choose to include redux-firestore.

It will be part of the next release and the setup instructions will fully outline this.

Owner

prescottprue commented Oct 19, 2017

@AVVS I have come up with a way to have redux-firestore be an optional dependency instead of a full dependency. This way it is up to the end user to choose to include redux-firestore.

It will be part of the next release and the setup instructions will fully outline this.

@AVVS

This comment has been minimized.

Show comment
Hide comment
@AVVS

AVVS Oct 19, 2017

AVVS commented Oct 19, 2017

@prescottprue prescottprue removed this from To Do in v2.0.0 Dec 22, 2017

@prescottprue prescottprue added this to To Do in Future Versions Dec 22, 2017

@prescottprue prescottprue modified the milestones: v2.0.0, v2.1.0 Dec 30, 2017

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