Skip to content

Conversation

@marekolszewski
Copy link

Description

This PR adds a new connect helper to the props.firebase object that lets users connect their redux store to the firebase DB without requiring the firebaseConnect HOC. This is useful for populating your redux state ahead of rendering your React app, a typical pattern when performing server side rendering.

Check List

If not relevant to pull request, check off as complete

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

@prescottprue
Copy link
Owner

prescottprue commented Oct 20, 2017

Nice idea for a feature!

I actually started on some examples that include doing this by hand with watchEvent in the last few days, but it is much nicer to have a helper like this.

One concern is the unmounting of the listeners added by watchEvent. We could handle this with explanation notes in the docs, but it might be nice to think about an easy way to handle it.

We may have to come up with a nicer name for export though since connect is used by react-redux, which is commonly integrated with react-redux-firebase. Maybe firebaseWatchers or firebaseListeners?

@prescottprue prescottprue self-requested a review October 20, 2017 01:58
@prescottprue prescottprue self-assigned this Oct 20, 2017
const connect = (watchArray, props) => {
const inputAsFunc = createCallable(watchArray)
const prevData = inputAsFunc(props, firebase)
const firebaseEvents = getEventsFromInput(prevData)
Copy link
Owner

@prescottprue prescottprue Oct 20, 2017

Choose a reason for hiding this comment

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

Kinda a nitpick: We should be able to use watchEvents and just pass the listener event settings array right into it instead of needing to call the map here.

Only reason I care is that I followed the same pattern when building the new redux-firestore stuff.

Copy link
Author

Choose a reason for hiding this comment

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

That's what I had initially but watchEvents doesn't return a promise, which is important for SSR as you have to wait for the data to be fetched before rendering. I could update watchEvents if you want.

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.

Great work. I think it will be ready to merge if we come up with a new name for the export.

Maybe firebaseWatchers or firebaseListeners?

reloadAuth,
linkWithCredential
linkWithCredential,
connect
Copy link
Owner

Choose a reason for hiding this comment

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

What about a different name for this export since react-redux uses connect and people commonly integrate with that.

Copy link
Author

Choose a reason for hiding this comment

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

Sure, happy to change it. How about firebaseConnectHelper or connectToEvents (to match watchEvents)?

const deleteFile = (path, dbPath) =>
storageActions.deleteFile(dispatch, firebase, { path, dbPath })

/**
Copy link
Owner

Choose a reason for hiding this comment

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

Great work with the comments here.

@prescottprue prescottprue changed the title Added a non-HOC version of firebaseConnect, for use with server side rendering feat(core): non-HOC version of firebaseConnect, for use with server side rendering Oct 20, 2017
@prescottprue prescottprue merged commit 9051570 into prescottprue:v2.0.0-beta.12 Oct 20, 2017
@prescottprue prescottprue mentioned this pull request Oct 24, 2017
3 tasks
@prescottprue
Copy link
Owner

This has been renamed to promiseEvents to more closely match watchEvent since it is exposed on the firebase instance itself. It looked a little weird to have firebase.firebaseWatch

prescottprue added a commit that referenced this pull request Oct 24, 2017
* feat(core): `recompose` used for `withFirebase` and `withFirestore` HOCs
* feat(core): `promiseEvents` method added. Similar to `firebaseConnect`, but not a React Higher Order Component - #299 - thanks @marekolszewski 
* feat(core): `redux-firestore` is no longer included and is now an optionalDependency (increased bundle size too much)
* feat(helpers): `getVal` added to simplify migration from v1 to v2 (can easily replace `dataToJS`) - #305 - thanks @fej-snikduj 
* feat(reducer): added error to clearly indicate wrong initial state being passed to `errors` state
* feat(reducer): `preserve` now supports passing an object to preserve `auth`, `profile`, `errors`, and `ordered`
* feat(docs): Firestore setup instructions switched to using `redux-firestore` externally
* feat(examples): firestore example uses `redux-firestore` directly (since it is no longer included)
* fix(typings): remove an artifact from a previous merge conflict - #304 - thanks @cbellino 
* fix(reducer): `authError` state is now `null` by default (tests changed to match)
* fix(examples): examples all point to `next` tag for consistency
* fix(examples): snippets README.md files updated
* fix(examples): `tests` folder removed from material example

### Potentially Breaking

* `authError` state is now `null` by default instead of `{}` - this is more clear, and more closely resembles `v1.*.*`
prescottprue pushed a commit that referenced this pull request Oct 25, 2017
* Migration guide updated with info about auth state and `getVal` - #305
@prescottprue prescottprue mentioned this pull request Oct 26, 2017
3 tasks
prescottprue added a commit that referenced this pull request Oct 26, 2017
* fix(reducers): add `EMPTY_AUTH_CHANGE` case to Profile Reducer - #305 
* feat(constants): `enableEmptyAuthChanges` config option replaced by `preserveOnEmptyAuthChange`  - #305 
* feat(profile): `autoPopulateProfile` support for v2 (still `disabled` by default)
* fix(presence): support presence option on `react-native-firebase` versions without `setPriority` on `RNFirebase.database.ThenableReference` - #267
* fix(core): withFirebase now works for all main methods
* feat(examples): material example updated to be much more simple (uses mostly functional components over classes)
* feat(docs): `promiseEvents` added to SSR docs - #299
@greaveselliott
Copy link

greaveselliott commented Apr 2, 2018

Hi @prescottprue,

I'm attempting to preload my redux state from firebase using store.firebase.promiseEvents().

Even though the promise resolves, the firebase data is never pulled, instead the request just hangs. I've tested to see if this is due to a race-condition, but I get the same problem. I've included a snapshot of my store's state when the promise resolves below.

{ 
  requesting: { 
    once: true,
    flamelink: true,
    'flamelink/environments/production/content/blog/en-US': true 
  },
  requested: { 
    once: false,
    flamelink: false,
    'flamelink/environments/production/content/blog/en-US': false 
  },
  timestamps: { 
    once: 1522711881049,
    flamelink: 1522711881049,
    'flamelink/environments/production/content/blog/en-US': 1522711881052 
  },
  data: {}
...
}

I've been tinkering with this for a few hours and seem to be getting nowhere, I appreciate any help you can offer.

Elliott

@prescottprue
Copy link
Owner

@greaveselliott A little hard to tell from what you are posting. Lets open a new issue with your exact use case with the listeners that are being set as well as any rules you have to make sure it is correctly replicated.

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.

3 participants