IjzerenHein / firestorter

Use Google Firestore in React with zero effort, using MobX 🤘
http://firestorter.com
MIT License
378 stars 50 forks source link

Query Clauses with Nested Collections #37

Closed imikemiller closed 6 years ago

imikemiller commented 6 years ago

Hi,

Could you provide some guidance on how to implement the following correctly? If I remove the orderBy clause the nested collection is returned as expected. I assume this is related to my implementation:

const Items = observer(class Items extends React.Component{
  ...
  constructor(props){
    super(props);
    this.items = new Collection(`${this.props.list.path}/items`,{
      query: (ref) => ref.orderBy('created_stamp','desc')
    });
  }
...
IjzerenHein commented 6 years ago

Actually that looks good. What is not working?

imikemiller commented 6 years ago

Thanks a lot for getting back. Empty result from the nested collection is the issue. Without orderBy I get the expected docs from the nested collection. Can I provide any more information?

On Tue, 25 Sep 2018, 5:48 pm Hein Rutjes, notifications@github.com wrote:

Actually that looks good. What is not working?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/IjzerenHein/firestorter/issues/37#issuecomment-424394269, or mute the thread https://github.com/notifications/unsubscribe-auth/AXtHNtWXXnSJZayIN7w6wi8jhnK2Exfeks5uelAzgaJpZM4W4Itc .

IjzerenHein commented 6 years ago

Hmm that's weird, would have expected that to work. Is it possible you don't have an index defined on created_stamp in your Firestore configuration?

imikemiller commented 6 years ago

Very possible, in fact I definitely don't. However on a different query on the parent collection I use the same orderBy and it's cool. also usually firebase gives a yellow box when it reckons it needs an index. But having said all that, you make a good shout, I will definitely add an index and see if it helps.

On Tue, 25 Sep 2018, 6:34 pm Hein Rutjes, notifications@github.com wrote:

Hmm that's weird, would have expected that to work. Is it possible you don't have an index defined on created_stamp in your Firestore configuration?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/IjzerenHein/firestorter/issues/37#issuecomment-424411196, or mute the thread https://github.com/notifications/unsubscribe-auth/AXtHNl_zP80JDbSeH-7aKAEHAPxfgV9Nks5uelshgaJpZM4W4Itc .

imikemiller commented 6 years ago

OK I have tested the indexing and no joy. As I thought its not required as I have only a single field clause.

Error: HTTP Error: 400, this index is not necessary, single field indexes are built in

If I log the state of the result in my render function it looks correct its just empty:

Proxy {length: 0, Symbol(mobx administration): ObservableArrayAdministration}
  [[Handler]]: Object
  [[Target]]: Array(0)
  [[IsRevoked]]: false
IjzerenHein commented 6 years ago

Hmm this is a weird one. I know adding the index was a long shot, but was worth a try. So you say that the orderBy works on a different global collection, which seems to indicate the code for it seems to work.

Could you try enabling the debug flag on the Collection and send me the logs?

new Collection(mypath, {query: ..., debug: true});

The next thing I would try to verify whether the problem is with Firestore itself or in Firestorter. E.g. to quickly define a query using the Firebase API, like this:

firebase.firestore().collection('mycol').doc('mydoc').collection('subcol').orderBy('created_stamp', 'desc').onSnapshot((snapshot) => {
  console.log('onSnapshot, docs.length: ', snapshot.docs.length);
});
imikemiller commented 6 years ago

I have no idea what I changed. I tested with firebase API and it worked fine, after running with debug on and tidying up my code a bit it started working. Wish I could point to anything significant that I changed! Thanks a lot for your help, I will drop a something in the tips jar when i get a chance

IjzerenHein commented 6 years ago

Alright appreciate it man. Glad the problem is resolved. If it re-appears just drop a note here and we'll have a closer at it. Cheers, Hein

imikemiller commented 6 years ago

You're a legend cheers

On Wed, 26 Sep 2018, 6:33 pm Hein Rutjes, notifications@github.com wrote:

Alright appreciate it man. Glad the problem is resolved. If it re-appears just drop a note here and we'll have a closer at it. Cheers, Hein

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/IjzerenHein/firestorter/issues/37#issuecomment-424784214, or mute the thread https://github.com/notifications/unsubscribe-auth/AXtHNjCb5sGchcGLwdBJryJYf-_ccY3aks5ue6x0gaJpZM4W4Itc .

imikemiller commented 6 years ago

Im ashamed to be back - I figured out what I had changed - I was not returning the orderBy clause from the query callback. If I actually return the query (as shown below) I get empty results and the logs shown below. I have also tried with a where clause to see if its related to orderBy but get same result.

constructor (props) {
    super(props);
    this.items = new Collection(`${this.props.list.path}/items`,{
      debug:true,
      query:(ref)=> ref.orderBy('created_stamp','desc')
    });
  }

screenshot from 2018-09-27 11-17-28

I get the render error when the orderBy clause is removed and the results come back properly. The dodgy dummy/id hack also works fine without the orderBy.

This is the log if I remove the orderBy:

screenshot from 2018-09-27 11-27-26

Seems like the _updateRealtimeUpdates function is not re-starting while there is some clause being passed to a nested collection query.

imikemiller commented 6 years ago

OK so by moving the query inside the componentWillRecieveProps function I got it to work. Thanks a lot.. again!

componentWillReceiveProps (newProps) {
    if (newProps.list !== this.props.list) {
      this.items = new Collection(`${newProps.list.path}/items`, {
        query:(ref)=>{ return ref.orderBy('created_stamp','desc')}
      });
    }
  }