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

OrderBy not returning any transactions at all #132

Closed
dusty-phillips opened this Issue Aug 7, 2018 · 11 comments

Comments

Projects
None yet
3 participants
@dusty-phillips

dusty-phillips commented Aug 7, 2018

Do you want to request a feature or report a bug?

This is a bug. I notice quite a few open OrderBy related issues, but this one seems to be even more opaque. However, I can see it's not happening to other people, so I suspect it's at least partially related to my setup. But my setup is very normal.

What is the current behavior?

Adding an orderBy clause to any firestoreConnect component causes no data to be loaded:

export default compose(
  connect(mapStateToProps, mapDispatchToProps),
  firestoreConnect((props) => {
    return [
      {
        collection: 'categories',
        where: [['uid', '==', props.uid],],
        orderBy: [['name', 'asc']] // doesn't work
        // orderBy: ['name'] // doesn't work
        // orderBy: 'name' // Nope

      }
    ]
  }),
)(CategoryChooser)

What is the expected behavior?

Elements should be returned, preferably in the requested order.

Which versions of dependencies, and which browser are affected by this issue? Did this work in previous versions or setups?

It's behaving the same in 0.5.7 and 0.6.0-alpha2, under firefox on Fedora Linux 28.

The JS project was created with create-react-app. Here are the deps from my package.json:

  "dependencies": {
    "firebase": "^5.3.1",
    "moment": "^2.22.2",
    "object-hash": "^1.3.0",
    "papaparse": "^4.6.0",
    "react": "^16.4.2",
    "react-dom": "^16.4.2",
    "react-redux": "^5.0.7",
    "react-redux-firebase": "^2.1.7",
    "react-router-dom": "^4.3.1",
    "react-scripts": "^2.0.0-next.3e165448",
    "react-social-login-buttons": "^2.1.2",
    "redux": "^4.0.0",
    "redux-firestore": "^0.6.0-alpha.2",
    "redux-thunk": "^2.3.0"
  },

Steps to reproduce and if possible a minimal demo of the problem via codesandbox or similar.

I suspect there will be some back and forth on this, but my setup is pretty generic. I'm composing my component as shown above, and I have my store configured as follows

import firebaseConfig from '../firebaseConfig.js'
import { createStore, compose, applyMiddleware } from 'redux'
import thunk from "redux-thunk"
import { reactReduxFirebase, getFirebase } from 'react-redux-firebase'
import { reduxFirestore } from 'redux-firestore'
import { initialState, rootReducer } from './reducers'

import firebase from 'firebase/app'
import 'firebase/auth'
import 'firebase/firestore'

firebase.initializeApp(firebaseConfig)
const fs = firebase.firestore()


const ReactReduxFirebaseConfig = {
  userProfile: 'users',
  useFirestoreForProfile: true,
  enableLogging: true,
}

const enhancers = [
  reactReduxFirebase(firebase, ReactReduxFirebaseConfig),
  reduxFirestore(firebase)
]

const composedEnhancers = compose(
  ...enhancers
)

const store = createStore(rootReducer, initialState, composedEnhancers)


export default store

For completeness, my reducers are defined correctly (everything works unless I orderBy):

import { combineReducers } from 'redux'
import { firebaseReducer } from 'react-redux-firebase'
import { firestoreReducer } from 'redux-firestore'
import { account_reducer } from './accounts'
import { transaction_reducer } from './transactions'

export const initialState = {}


export const rootReducer = combineReducers({
  firebase: firebaseReducer,
  firestore: firestoreReducer,
  accounts: account_reducer,
  transactions: transaction_reducer
})

There are no error messages in the console. I disabled redux-dev-tools to make sure it wasn't conflicting somehow, but when it was on, it was showing an uninformative error returned from firebase:

errors: {
      byQuery: {
        'categories?where=uid:==:PijZAm394pMu24jSjAM2yVLfPLZ2': {
          code: 'failed-precondition',
          name: 'FirebaseError'
        }
      },
      allIds: [
        'categories?where=uid:==:PijZAm394pMu24jSjAM2yVLfPLZ2'
      ]
    }
@dusty-phillips

This comment has been minimized.

dusty-phillips commented Aug 7, 2018

Of course putting everything together and posting it made the answer come clearer.

The problem was that there is a missing index for a composite query. In raw firebase, there would be an informative error message in the console with a link to create the index. Is it possible to expose that to the developer in this case, too?

@prescottprue

This comment has been minimized.

Owner

prescottprue commented Aug 7, 2018

Those messages should be passed through if you use logErrors: true within your config.

It may be smart to have certain errors, like the one for indexes not being set, come through by default in the future. Thanks for posting.

@karltaylor

This comment has been minimized.

karltaylor commented Aug 7, 2018

I feel like #131 is related?

@dusty-phillips

This comment has been minimized.

dusty-phillips commented Aug 7, 2018

I had enabled logErrors and not seen anything in the console, but that was probably due to not looking correctly. It was getting pretty late at night when I gave up. ;-) I think all this really needs is a documentation update -- more clarity on what logErrors does and maybe a faq entry on indexes.

@karltaylor

This comment has been minimized.

karltaylor commented Aug 7, 2018

I have added 'enableLogging' and 'logErrors' but I'm not getting any logs through ☹️

const store = createStore(
  rootReducer,
  compose(
    reactReduxFirebase(firebase, rrfConfig), // includes same as below.
    reduxFirestore(firebase, { logErrors: true, enableLogging: true }),
    ...enhancers
  )
)

Copied from your example here

@prescottprue

This comment has been minimized.

Owner

prescottprue commented Aug 7, 2018

@karltaylor and @buchuki The docs and my previous suggestion were wrong, it looks like the code is actually looking for logListenerError. Try switching to that and giving it another go.

Its weird since it is defaulted to true. Wondering if it is being run over by reactReduxFirebase? Maybe try putting reduxFirestore before reactReduxFirebase? Going to investigate more here since this for sure should not happen.

This should totally be documented more clearly as well.

@karltaylor

This comment has been minimized.

karltaylor commented Aug 7, 2018

@prescottprue Bingo!

Putting reduxFirestore first logged the error. It does seem to be getting overwritten by reactReduxFirebase

@karltaylor

This comment has been minimized.

karltaylor commented Aug 8, 2018

@prescottprue I'd love to help you out with the docs for this but not sure if you plan on fix the logListenerError config being overwritten by reactReduxFirebase?

I can always add some documentation for it for the time being and then update as required

@prescottprue

This comment has been minimized.

Owner

prescottprue commented Aug 8, 2018

@karltaylor Seems like a bug in react-redux-firebase where all redux-firestore config is being overwritten. For now though, like you said, we will just document putting reduxFirestore before react-redux-firebase.

prescottprue added a commit to prescottprue/react-redux-firebase that referenced this issue Aug 13, 2018

Support config already existing on store
* fix(enhancer): support config already existing on store - [132 of redux-firestore](prescottprue/redux-firestore#132)
@prescottprue

This comment has been minimized.

Owner

prescottprue commented Aug 13, 2018

@karltaylor I added a change to the PR for v2.2.0-alpha that prevents config that is already existing on the store from being run over. This way, it shouldn't matter if reduxFirestore comes before or after reactReduxFirebase when creating the store.

prescottprue added a commit to prescottprue/react-redux-firebase that referenced this issue Aug 13, 2018

v2.2.0-alpha
* fix(profile): profile update on login works with email login (used to require `createUser`) - #475
* feat(HOCs): optimize firestoreConnect unset / set listeners - @demoran23
* fix(auth): move detaching of profile listeners before `signOut` within `logout` method to fix `permission_denied` errors - #494
* fix(enhancer): support config already existing on store - [132 of redux-firestore](prescottprue/redux-firestore#132)
@prescottprue

This comment has been minimized.

Owner

prescottprue commented Sep 6, 2018

Anyone looking to solve this can get the latest version of next by calling npm i --save react-redux-firebase@next.

prescottprue added a commit to prescottprue/react-redux-firebase that referenced this issue Nov 10, 2018

v2.2.0
* fix(storage): uploadFile gets downloadURL before writing metadata - #487, #488
* fix(storage): add unit tests for `fileMetadataFactory` (config level) and `metadataFactory` (option for uploadFile) to confirm downloadURL is available before writing metadata - #487, #488
* fix(storage): fix downloadURL error in file upload metadata - @jpierce42
* fix(auth): `LOGOUT` action dispatch only include preserve parameter if provided in settings (not when undefined)
* fix(profile): profile update on login works with email login (used to require `createUser`) - #475
* feat(HOCs): optimize firestoreConnect unset / set listeners - @demoran23
* fix(HOCs): firestoreConnect no longer requires store.firebase (only attaches to props if available)
* fix(auth): move detaching of profile listeners before `signOut` within `logout` method to fix `permission_denied` errors - #494
* fix(enhancer): support config already existing on store - [132 of redux-firestore](prescottprue/redux-firestore#132)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment