feat(core): support lazy loading and modular loading of Firebase

nicolasgarnier commented 6 years ago

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 commented 6 years ago

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

prescottprue commented 6 years ago

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 commented 6 years ago

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

nicolasgarnier commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

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 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

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

nicolasgarnier commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

@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.

nicolasgarnier commented 6 years ago

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 commented 6 years ago

@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.

AVVS commented 6 years ago

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

prescottprue commented 6 years ago

@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 commented 6 years ago

@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 commented 6 years ago

awesome!

On Oct 19, 2017, at 1:17 PM, Scott notifications@github.com wrote:

@AVVS https://github.com/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.

The setup instructions will fully outline this.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/prescottprue/react-redux-firebase/issues/250#issuecomment-338025510, or mute the thread https://github.com/notifications/unsubscribe-auth/ABol0dCWTrFMry3Ig40adehJRxgsTD5Wks5st65agaJpZM4PAuIp.

prescottprue commented 5 years ago

Working on support for SSR and multiple apps as part of v3.0.0. There is also going to be a focus on bundle sizes.

@nicolasgarnier I know this was a while back, but did you end up coming up with a solution that worked for authing single users during SSR?

nicolasgarnier commented 5 years ago

Yes I had something that worked (partly) and that can now be improved with Service workers (so only in browsers that supports it)

The base principle is to send the ID Token of the user as part of the request to the server. And decode it on the server and sign the user into a Firebase app.

On the Server you:

Read, decode the ID token and verify it's valid So that you have the UID of the user.
Using The Admin SDK, generate a Custom token for that UID
Sign in the user in the JS SDK using the Custom Token. Make sure you use a separate 'named' Firebase App instance for that user so that you don't re-use that authenticated app instance when another user does a request. (See this code)
Run the rest of your React app using the authenticated Firebase app instance you just created.

The server part works well. Now let's talk about how to pass the ID Token of the user from the client to the server.

On the client, I was using cookies:

You get the ID token of the authenticated user.
you set the ID token in a cookie (e.g. the __session cookie)
make sure to clear the cookie if the user signs-out.
Have an observer on ID token refresh and set the cookie again.
It was all done in this code

This worked but the ID token is only valid for 1h so, in the case where the user has closed the app for more than 1h. Opens the app again then we'll be sending an expired ID token in the first request to the server and the site generated by SSR will be unauthenticated.

So the trick is to use a service workers that intercepts every requests to the backend and appends an ID token in the URL parameter (unfortunately Service wokers can't set cookies yet). This has been made recently possible, when I wrote my app Firebase Auth could not work inside a service worker (but now it can).

By using service workers we'll be able to generate a fresh valid ID token even when the user has closed the app for more than 1h and make sure it's sent as part of the request to the backend.

I would use both Service workers and cookies (as a fallback).

Voila, that's it 😅 😅

prescottprue commented 5 years ago

@nicolasgarnier Awesome, thanks for the full explantation. I may end up trying to write that into the docs or an example to clarify for others.

prescottprue / react-redux-firebase

feat(core): support lazy loading and modular loading of Firebase #250