FirebaseExtended / angularfire

AngularJS bindings for Firebase
MIT License
2.73k stars 631 forks source link

v0.7.2 child_added fires whenever attached #282

Closed joshbowdoin closed 10 years ago

joshbowdoin commented 10 years ago

In v0.6.0, child_added events would only fire before the loaded event, and then would fire again for each new child.

In v0.7.1 & v0.7.2, child_added events fire for all children anytime a handler is attached. In other words, if I have 5 children, it will fire 5 times before the loaded event, and then if I attach a new handler to it one minute later, it will fire that handler 5 times.

This is a departure from the way the Firebase api handles it, and in my opinion makes it difficult to do things like init an array (using orderByPriority), and then simply add items when they are added. Instead, I'd have to add each item to an array, potentially firing multiple updates to my ui from the outset.

Was this change intentional?

katowulf commented 10 years ago

Hey @joshbowdoin!

This is a departure from the way the Firebase api handles it

Not true. This matches the Firebase behavior. Any time you attach a child_added listener, it gets notified for all existing children.

The Child Added event is typically used when retrieving a list of items (e.g. chat messages) in Firebase. Unlike 'value' which fires for the entire contents of the location, 'child_added' fires once for each immediate child and continues to trigger as new children are added.

It also matches the angularFire API, which statesinfers that the event handlers follow the Firebase spec.

Adds an event handler for the specified event. You can attach events for regular Firebase events (such as child_added and value, see the Firebase docs for a full list), and two additional ones...

Could you explain your use case for child_added? And also why you're using $firebase to extract values into an array instead of using the $firebase object as designed? It would be helpful in planning our upcoming API improvements.

Was this change intentional?

I fixed some bugs with our $bind, $getIndex, and $on('loaded') functionality by refactoring a small portion of the event handling code. I'm not sure if the original code behaved as you mentioned on purpose or because of a bug. My intent was simply to follow the spec as closely as possible and when in doubt, stick to the Firebase behaviors.

The good news is that we've got test specs underway, so we'll be able to solidify these sorts of details in the upcoming API improvements, and well in advance of our 1.0 launch.

Let me know your thoughts.

joshbowdoin commented 10 years ago

I see that you are right about the Firebase API. So, v0.6.0 was actually broken in this aspect?

I'm using the orderByPriority to create ordered lists. Much of my data is ordered, whereas objects aren't. Is there a better way to be doing this?

My use case for child_added:

var items = $firebase(new Firebase(firebaseURL));
var itemsArray;
items.$on('loaded', function () {
    itemsArray = $filter('orderByPriority')(items);

    //with v0.6.0, these only fired for NEW items, since it was attached after load
    items.$on('child_added', function () {
        var insertIndex = _.findIndex(itemsArray, { $id: data.prevChild }) + 1;
        itemsArray.splice(insertIndex, 0, items[data.snapshot.name]);
    });
});
joshbowdoin commented 10 years ago

One thought is that I can re-create the whole array for each child_added, child_removed, child_moved, but then Angular is required to update the whole ui, versus simply adding, removing or moving one item (and this gets sluggish with large lists)

katowulf commented 10 years ago

It's still a bit unclear what you actually need the array to be ordered for. If you're presenting the data in the UI, you can simply use ng-repeat="item in items | orderByPriority" of course.

You could simply use child_added and not create the array using $filter in the first place:

var itemsArray;
items.$on('loaded', function () {
    itemsArray = [];
    items.$on('child_added', function (data) {
        var insertIndex = _.findIndex(itemsArray, { $id: data.prevChild }) + 1;
        itemsArray.splice(insertIndex, 0, items[data.snapshot.name]);
    });
});

Of course, this isn't going to capture changes or remove events. You'd need to monitor those too, at which point, you've pretty much got a $firebase object. Which is what makes me curious why you need an array ordered by priority for anything other than presentation.

joshbowdoin commented 10 years ago

You are correct, the filtering is simply for the UI. However, when I have a list with many bindings, it is not performant to filter in the UI. (See: http://www.bennadel.com/blog/2489-How-Often-Do-Filters-Execute-In-AngularJS.htm)

When I am filtering in the controller, the UI only has to update when an item actually changes. When the filtering is done like ng-repeat="item in items | orderByPriority" the whole UI must be regenerated for every click (because a new array is created, and Angular does not recognize it). So, when I click a single toggle button, I get a long delay (0.5-1 second) before the UI updates.

Originally I thought Angular was being slow, then realized it was simply that Angular was regenerating the whole UI. Does this make sense?

joshbowdoin commented 10 years ago

I'm realizing that if the orderByPriority filter could simply UPDATE the array it created first, then this would solve much of the problem. I'm not really sure of an elegant way to do this with the way angular filters work, though. orderByPriority would have to maintain some sort of cache, which feels like a hack to me.

katowulf commented 10 years ago

Hi Josh, first of all, thanks for humoring me and talking me through this, I'm starting to grok the problem now.

Using an update in orderByPriority is probably reasonable. I'm working through some API changes that may get rid of the need for orderByPriority. That could also improve performance quite a bit.

How many records are we talking about that you're seeing these delays? Do you have a small repro I could try out? I'd like to see how many compiles are getting run, how many times the UI gets refreshed, how much data is too much, and through some benchmarks, see if we can remove any bottlenecks in the API, or address them in the next version's improvements.

joshbowdoin commented 10 years ago

Sure. Thanks for wanting to help me out!

Here is a sample. This sample seems to be snappy still, but when I have multiple values bound for each child (a form with 8-10 fields), you can imagine how angular would have trouble refreshing that quickly (twice! since orderByPriority ends up getting called twice per change).

http://embed.plnkr.co/OJR9MC1sX99vxGNifhAA/preview

(Modified - Many of my uses have nested lists, so when I sort the parents by priority as well, I'm actually getting four hits to orderByPrioirty per click)

katowulf commented 10 years ago

Hi @joshbowdoin

This one is fairly fast, even with 70 records in each path all opened at once: http://plnkr.co/edit/k4grOAXSLZCNTmo28ecR

It looks like orderByPriority is invoked multiple times for each child set, but it still works fairly efficiently. I think at this point I'm going to call this okay until we get some of our array-related API revisions in.

Thanks again for the walkthrough. This has been extremely useful in figuring out where to take our API revisions and pre 1.0 efforts!

joshbowdoin commented 10 years ago

@katowulf thanks for looking into this.

I just noticed that my previous statement is also wrong... When I click a button, the whole view isn't being regenerated by Angular (tested by changing some HTML on a different child in the inspector).

I had noticed that it WAS in the past, but perhaps Angular is now testing each item (which remains the same) versus testing the array....? I'm not sure. Will update this if I come across a case again where orderByPriority seems to be causing excessive regenerations.

I'm interested in what you mean by array-related API... Do you have anything that I could look at / test out?

katowulf commented 10 years ago

Hi Jonathan,

Make sure you check out the plunkr I created. It's quite a bit simpler than the original and does seem a bit faster.

re: array-related - I'll be putting in a PR with the proposed API changes soon. You can check it out then.