Ensure array items are returned in the correct order

[morgante]

This pull request fixes #36 by using the DataSnapshot.forEach method to build up an array before passing it to React. This ensures that arrays are sent to React in the order which the query specified.

[morgante]

Also, quick note: it's unclear how to package up a forked npm distribution for use in our package.json files.

I ended up trying to commit the dist directory (f8b47c6) but that also didn't work.

[jwngr]

Hey @morgante - I don't think this actually fixes the problem. In your code, data will be equivalent to dataSnapshot.val() since they are both un-ordered JavaScript objects with the same exact keys. So the ordering for push IDs will still not be correct due to the way object iteration works in JavaScript. In the array case, we actually need to pass through the snapshot and assign indices for each item in the DataSnapshot.forEach() loop.

As for the the npm issue, this is intentional. We don't really want people working off of a non-released branch as that leads to a huge support headache for us. We only support official releases. I wouldn't even suggest using the master branch as it has not been vetted for production.

[morgante]

@jwngr I'm not sure I follow. We build data iteratively using the keys within the DataSnapshot.forEach loop, which ensures that the object is built in the correct order (and then converted to an array with the correct order). We have tested this with our data and it functions as expected (ie. the array is built with the correct order on this branch, but not on master).

It's unfortunate that you don't support people working on a non-released branch. I definitely understand why you would not want everyone doing so, but as of now this is blocking a release for us.

[jwngr]

Sorry that you are blocked on this. In your fork, you can add the dist/ files in a commit and then just do npm install <githubname>/<reponame> on your repo. You should be able to use that until we get a new official release out. See how our automated bot does this in the commits from November 6th.

What does your data look like that is working? If we have an object like this:

{
  "-Ja923adg90": "foo",
  "-Ja92820df0": "bar",
  ...
}

Then converting that to an object via dataSnapshot.val() will result in the same exact thing as data in your code. The problem is that adding items to a JavaScript object in order does not actually order them. Take this for example;

var obj = {};
obj.b = 1;
obj.a = 2

Object.keys(obj).forEach(function(item){
  console.log(item);
});

While this might print out b and then a on your browser, that ordering is not guaranteed. That is the whole point of DataSnapshot.forEach(). It handles looping through things in the proper order for you. Converting that back into an unordered object loses that ordering. See this Stack Overflow post for more details.

[morgante]

Interesting. I can see how we could use DataSnapshot.forEach for arrays (that would just be a slight tweak to my code), but what is your plan for using it with objects?

[jwngr]

Everything in Firebase is technically an object. Arrays are just objects with keys like 0, 1, etc. But people want lists of items which were added via push() (and thus have these long push IDs as noted in my last comment) to show up as actual JavaScript arrays on the client. That is the point of the bindAsArray() method. We need to handle coercing those push IDs into an actual array with numeric indices. The push ID will be stored somewhere else, as noted in #37.

[jwngr]

So, to answer your question, the forEach() is not needed for bindAsObject(), but it is needed for object-like values which users call bindAsArray() on.

[morgante]

Okay, that clarifies things further—I thought we had to support ordering for both bindings, but we only have to do it for bindAsArray, right?

If that's true, I'd be happy to rewrite this PR quickly to actually build up an array from within the forEach.

[jwngr]

Yup, just for bindAsArray(). Would love it if you could take another crack at it.

[jwngr]

This was superseded by #43. I went with a more complex and complete fix for this issue. Thanks again for taking a crack at this.

[bogdantmm92]

This is not working correctly when using orderByChild(). I can see that you only have tests for orderByValue.
PS: it seems that the parameter previousChildKey in _arrayChildAdded is undefined.

[jwngr]

I just added tests for orderByChild() and orderByKey() with PR #59. Everything seems to be passing and I can't think of anything else to test. Would you be able to provide a failing test case that I can debug?

It's odd that previousChildKey is undefined. How are you seeing that? it should only every be a string or null, as noted in the Firebase API docs for on() (look at the "Child Added" section). If you are seeing undefined, then there is probably a bug in the Firebase client, which would be good to track down.

What versions of ReactFire and Firebase are you using? Does upgrading them to the latest fix the problem at all?

Also, please open a new issue if you think you've found a new bug. This is a PR and was not even merged in. Thanks!