Make each-* routine compatible with collections

[ydaniv]

Current implementation overwrites objects in-place, so when another object (e.g. a UI component) references the object in the collection that's overwritten discrepancies start to appear between the state of that component and the object (model) in the collection it's referencing.

This implementation maintains the original objects while keeping the correct state and same index order of collection and DOM elements.

This means that the each-* routine can now be safely used with Backbone.js/Spine.js/etc collections and therefore, Rivets.js can become a genuine part of any SPA stack.

This implementation has been tested in a stack using Backbone.js and supports listening to the add, remove and sort events of Backbone's collections, with UI components referencing both the collection and each of its models.

~Y

[pajtai]

👍 👍 to merge this.

I had an issue with eaching over a collection (using a custom adapter), but eaching over collection.models with the default dot adapter causes the same problems. Upon removing a model from the middle of the collection, the on-* binders would not get correctly updated (even though the other things, such as rv-text would)

I tried out this pull request and it fixes the issue. Thanks @ydaniv !

[mikeric]

@ydaniv Thanks for taking the time to identify this and open a pull request. The implementation changes look reasonable so far, but It doesn't seem to maintain the proper ordering of an array. Notice how the order of the nodes is not consistent when changing list.items.

http://jsfiddle.net/ZaLQR/

Actually, the first node seems to be consistent with the first item in the array, but the rest of the items seem to go in reverse order. e.g. [1,2,3,4,5] is getting inserted / updated as [1,5,4,3,2]. Note that this is not the case on the first bind (the order seems to be correct initially).

I'm also curious about the issues you were seeing with the current implementation (of updating the existing Rivets.View nodes in place with new data instead of shuffling the nodes to maintain proper index). If there are no perf benefits from this new implementation, it's not clear to me what issue it's resolving. Do you think you or @pajtai could create an isolated demo that illustrates some of the issues with the current implementation (on jsfiddle or jsbin, etc.)?

[pajtai]

@mikeric Here are 3 minimal use cases. - This regards specifically the problems I rand into. I believe they are similar to @ydaniv , but they might not be identical. Both his pr, and rivets 0.6.5 solved my problem. Rivets 0.6.6 contains the problem.

Basically the example is removing an item from an array using splice and using on click handlers. This is roughly the equivalent of binding to a Backbone collection and calling model.destroy on one of the models in the collection.

This seems to be due to unbound click handlers.

• Steps to reproduce
  1. open console
  2. click: 3: log, 3: remove, 4: log, 4: remove

0.6.6 - unexpected
http://jsfiddle.net/pajtai/VFVQK/

(and looking into it further - it looks like the custom handler is causing this - the custom handler is needed to handle binding to prototype methods - e.g. model.prototype.destroy - here is a minimal use case without the use of prototype methods and a custom handler, and all works as expected - http://jsfiddle.net/pajtai/d2pdw/ )

0.6.5 - expected (this surprised me, I'll have to look into this more, and see whether our custom adapter works with this version) edit: yes, 0.6.5 but not 0.6.6 works with our custom bb adapter
http://jsfiddle.net/pajtai/R6k3V/

@ydaniv fork - expected
http://jsfiddle.net/pajtai/QfUTG/

[mikeric]

@pajtai Ah, yes this is definitely an issue, however it's not at all related to the each-* binder or the changes in this pull-request.

The issue is that when view.update is called with a new set of models, it doesn't set binding.model on the bindings that may have had their model changed, and since your rivets.config.handler function is using binding.model, it is still referencing the old model instead of the new one.

Here is a more isolated instance of the issue. http://jsfiddle.net/t2sxB/

This pull request does not actually fix that issue. It just worked in your case because this pull request is replacing the entire view for each iteration instead of calling view.update (where the issue is) on them in place.

A temporary solution is to use binding.observer.target in your handler, custom binders, components, etc. instead of binding.model. http://jsfiddle.net/t2sxB/2/

I will add a fix for this in 0.6.7 for sure though.

[pajtai]

@mikeric Thanks for the explanation!

[ydaniv]

@mikeric thanks for pointing out the order issues, I've fixed it, added an extra test for that case and will update the PR.

Note to self: don't use in operator in CS in condition expressions.

Regarding the issues I'm seeing, it's specifically about using view.update of existing Rivets.View with the binding.model of another Rivets.View. This is the root cause of the problems when using each- in conjunction with other binders (as in the case of @pajtai's problem with each- and on-), as well as the problems occurring when Rivets is used alongside another tool that is watching the same models in the collection.

The problem is that while Rivets may not care about the data each model contains, as long as it is in sync with the DOM, other components that hold references to specific models do care about the data in the model they are watching.

I'm working now on preparing a fiddle that will demonstrate it better.

[ydaniv]

@mikeric here you go, consider the following example of using Backbone views with Rivets. You might argue that views of Backbone and Rivets are mutually exclusive in an application (which I personally don't think so), but what's important here is the example of another component watching the collections' models:

This example is using your latest code:
http://jsfiddle.net/7S9yC/1/

Notice what happens after you add 3 items, then remove the middle one, and then click on the items to log their ID. The text created by the binding no longer matches the bound model.
This is because Rivets changes the object that the BB view is bound to.

This example is using the dist you created from my PR:
http://jsfiddle.net/tkxm2/7/

Here you can see that the bound model is left untouched by Rivets.

This gets repeated in even more nasty ways when trying to use any kind of UI components that bind to the models in the collection.

The main take away here is interoperability of Rivets with collections (be it BB, Spine, or any Arrayof Objects) and any other library.

[mikeric]

@pajtai ok the bug that you've outlined should be fixed now in master. Here is your example from above, but using 0.6.7.

http://jsfiddle.net/VFVQK/4/

[ydaniv]

@mikeric any comments about this PR's original intent? Anything not clear that I can try better explain?

[mikeric]

@ydaniv Yep, just want to be sure that I got this right. I might be way off on this, and please let me know if I'm just misunderstanding the issue completely. But I think you might be confusing interoperability with wanting Rivets to assume responsibility where it shouldn't be.

Problem

You're using Rivets to bind most things in that iterated view (and it successfully keeps those things in sync), however you've decided to use a Backbone view to bind that logId function, which is totally fine if you want to do that, but Rivets is not in charge of keeping that function in sync anymore, your Backbone view is.

This is why things are misaligned when your data changes — Rivets is updating things that it is in charge of, but your Backbone view is not.

Solution if you must use two different methods for binding

You need to make sure that your Backbone view (or any other means of binding) is observing and updating things in the same manner as Rivets. You can't tell Rivets to account for add, remove, reset and sort (as you've done in your adapter) and then only tell your Backbone view to account for add and expect it to magically update when you remove an item like Rivets does. This is why everything is updating except for that logId function.

Solution using Rivets

Using rv-on-click to bind the logId function will hand off the responsibility to Rivets to keep it in sync. Again, this is up to you if you want to use Rivets for event handlers, or if you'd prefer to use something else.

The solution provided by this pull request

This pull request kind of avoids the issue by not using view.update at all anymore in the rv-each routine. You'll still get a surprise if you try to update a view which uses mixed methods of binding and one of them is not fully implemented like your adapter.

I'm not opposed to merging this in, for example if it provides performance improvements. And then I guess as a bonus, it would provide a cop out for needing to define observers on your Backbone view when using rv-each. Just that there is no actual bug that it's addressing, as far as I can tell.

I'm going to run some benchmarks and compare the two.

[ydaniv]

@mikeric no, not confusing it. My point about interoperability is that if you change an object that doesn't belong to you, when you weren't asked to, then you're assuming authority over an action on an object that you weren't granted, or at least, were not expected to have taken.

Ignore the use of Backbone.View, that was just a mean to make a quick demo. I don't just them myself, I actually need Rivets to be a data binding library for use with uijet.
I actually did a TodoMVC example using uijet, Rivets and Backbone, but am dependent on this fix since components that are bound to the models get their models changed for them by Rivets, which breaks UI.

I'll explain my example in a more generic way:

1. User creates a collection.
2. User binds - only binds which suggests "just watch this please, but don't touch, unless asked to" - a Rivets.View to that collection.
3. User invokes something like collection.remove(model).
4. User expects the bound item to be removed from the DOM by Rivets.
5. User discovers other models, not removed, have been changed by Rivets.

So, back to my example, binding also to other actions, like remove, will not fix the problem, since the core logic of Rivets, to do view.update, will always cause objects to change in place, while the identity (reference) of that object remains the same.

Using rv-on-click will fix the UI behavior but will just hide the fact that models are being tempered with by Rivets. Any other object keeping reference to these models will lose its state as a result.

So, yes, I say "don't use view.update there at all", since it's breaking the contract between the binder and the collection.

Should I produce another example? (:

I can also get my TodoMVC example online on my fork if it helps.

[mikeric]

My point about interoperability is that if you change an object that doesn't belong to you, when you weren't asked to, then you're assuming authority over an action on an object that you weren't granted, or at least, were not expected to have taken.

Has this been demonstrated in your fiddle? I don't see Rivets changing any of the objects that you've bound to that view. The only bound object that seems to have been changed is your list collection when an item is removed, but that is carried out by your ListItem::removeMe function, not by Rivets.

User discovers other models, not removed, have been changed by Rivets.

Sounds like the same assertion as above. Can you confirm that Rivets is actually doing this?

Using rv-on-click will fix the UI behavior but will just hide the fact that models are being tempered with by Rivets. Any other object keeping reference to these models will lose its state as a result.

This is false. Rivets is not tampering with your models and/or references to them from other objects. Maybe it wasn't clear, but the reason why your view appears to be logging the incorrect model.cid is not because Rivets is somehow changing the model that your Backbone view is referencing; it is because the click handler is still bound to the original view that you initialized on that DOM node explicitly in your ListView::add function.

Here's a modified version of your fiddle that displays the view.cid in brackets so you can see which of your Backbone views are still bound to the incorrect DOM node.

http://jsfiddle.net/7S9yC/3/

Should I produce another example? (:

If the issue has been demonstrated in your first example, then no, there should be no need. But if it hasn't, then yes that would certainly be helpful :P

Seeing the TodoMVC app would be great for some context though. I'm also curious to check out UIJet.

Sorry for all the back and forth on this, I'd really like to come to a consensus and get it resolved. I still haven't had the chance to run some benchmarks, but if this is in fact faster, then I'll definitely merge it in regardless of the issue being the fault of Rivets or not.

[ydaniv]

OK... now I see what's happening. update is just changing references and not changing the objects in place.

What you said about the Backbone.Views initialized by me is right, they are bound to elements removed, and what's causing the discrepancies is that Rivets just pops/pushes <li> elements from/to the end of the list and reuses the rest of the elements, while the views' bindings are not updated.

So, yes that is not up to Rivets to maintain.

However, this method does cause a lot of headaches when trying to bring in another component that needs to bind itself to the DOM.
I'll try using current API to work around that and see what I can produce.

Thanks for putting up with me :P

But do tell if this brings any perfomance improvements, which I doubt.

[ydaniv]

@mikeric on second thought, I think there are some improvements in my implementation but it looks like it's still incomplete, and I think we can do much better.

The main issue with current implementation is that it's only optimized for the case of adding/removing items at the end of the iterable. If you're removing/adding to the beginning or middle of the list it will change the DOM for every element in the list, starting from the changed element and on.
So in case you have a list of 100 items and you added an item in he beginning, Rivets will update the DOM 100 times.

I suggest to take another shot at this implementation and try do the extra work in JS so to minimize the number of times Rivets touches the DOM, which is the real performance killer.

What say you?

[jhnns]

each-routines are always tricky... ^^

[pppdns]

+1 for minimizing DOM rerendering in each-*

[ydaniv]

I gave it a shot in PR #311.
Still not getting any decent improvement. Could use more help in optimizing iterations, and perhaps caching some of the calculations.