rv-each removes the wrong component when I splice the model.

[saiphcbk]

Hi, I've been enjoying using rivets but have hit upon a problem I can't seem to get around.

I've started using components in an rv-each block. It all renders fine when it loads but when I try to remove the first element in the model's array using splice it appears that the wrong component is removed. When I check the model it correctly shows the first element removed.

I've created a fiddle to try and be more clear
http://jsfiddle.net/dko9b9k4/

var model2 = [{ "value":"1"},{"value":"2"},{"value":"3"}];

window.onload = function () 
{
    function controlviewmodel(attributes) {
        this.data = attributes;
    }

    rivets.components['control'] = {
        template: function() {
            return '<button>{data.option.value}</button>';
        },
        initialize: function(el, attributes) {
            return new controlviewmodel(attributes);
        }
    };

    rivets.bind(document.querySelector('#view'), {model:model2});

}

function doSplice()
{
    model2.splice(0,1);
}

<button onClick="doSplice()">Splice</button>

<table id="view">
<tr rv-each-option="model">
<td>
<control option="option"/>
</td>
<td>{option.value}</td>
</tr>
</table>

[Duder-onomy]

Hi,

I got your fiddle working: http://jsfiddle.net/98uhgtn8/

These are the important changes:

In the component definition:

    rivets.components['control'] = {
        template: function() {
            return '<button>{ option.value }</button>';
        },
        initialize: function(el, receivedModel) {
            return receivedModel;
        }
    };

Components are new to me, but I will be doing a deep dive tomorrow. I will respond to this thread if I figure out exactly "why" this is the case. I will also update the docs if, after I figure them out, find them to be misleading.

Please let us know if this helps you!

[saiphcbk]

Hi Duder-onomy,

Thank you, that did help. It is a shame not to be able to use the separate controlviewmodel as I need to add the rv-on-* functions but I can code around that for now by adding the functions to the receivedModel before returning it in the initialize.

I was going to do it like this example from LeedsEbooks:

function counterViewModel(attributes) 
{
    this.data = attributes;

    this.increment = function (event, scope) {
        scope.data.counterAttr.value++;
    };
    this.decrement = function (event, scope) {
        scope.data.counterAttr.value--;
    };
    this.toggleColor = function (event, scope) {
        var old = scope.data.colorAttr.value;

        if (old === 'red') scope.data.colorAttr.value = 'blue';
        else scope.data.colorAttr.value = 'red';
    };
}

Any extra info on components would be very welcome when you do get to the bottom of them. In my searches all I could find that was of use a fiddle by leedsebooks which my example was based on.

Many thanks.

[saiphcbk]

Hi, your suggested fix did fix the issue but only if each object uses the same template. I've been trying to build a page that will display form input items defined by an incoming JSON model string. I've found that when I Splice the first objectof the model array it removes the last dom element and pushes the data up to the other elements. So the first dom element is now bound to the second object in the array.

var model = [{ "type":"button","value":"1"},{"type":"label","value":"2"},{"type":"input","value":"3"}];

var buttonTemplate = "<button>{option.value}</button>";
var labelTemplate = "{option.value}";
var inputTemplate = "<input type='text' rv-value='option.value' size='1'/>"

window.onload = function () 
{
    rivets.components['control'] = {
        template: function() 
        {
            var type = this.view.models.option.type;
            if(type == "button")
            {
                return buttonTemplate;
            }
            else if (type == "label")
            {
                return labelTemplate;
            }
            else if (type == "input")
            {
                return inputTemplate;
            }
        },
        initialize: function(el, attributes) {
            return attributes;
        }
    };

    rivets.bind(document.querySelector('#view'), {model:model});

}

function doSplice()
{
    model.splice(0,1);
}

http://jsfiddle.net/nyjt2pox/2/

If I delete the first element in the model array I would expect it to remove the dom element that it was bound to, not the last dom element rendered.

Am I just trying to bend this to do something that it was never intended to ?

[benadamstyles]

Hi @saiphcbk , glad my fiddle was helpful to some extent and I hope it didn't confuse matters. I haven't tried doing a component in an each block and I'm having a hard time getting my head around why @Duder-onomy 's changes made it work, but I can say with certainty that your latest comment is expected behaviour. The each-* binder does remove the last element, then re-render all preceding elements in case the array was spliced in the middle.

If you need the each-* binder to work differently, why not write your own that works in the way you expect? Rivets is supposed to be used in this way – the built-in binders are really only there for convenience, and Rivets itself is really a system for combining your own binders, adapters and formatters. That's what makes it so nice to use, once you get accustomed to it.

[Duder-onomy]

@saiphcbk I spent around 4 hrs last night trying to figure out why your fiddle did not work.

I thought maybe it had to do with the new'ing up of the model, which might have broken the binding. Though, @Leeds-eBooks fiddle works fine.

Anyhow, I think you found a bug. I am trying to isolate it and write a failing test for it.

Will keep in touch.

[saiphcbk]

Thanks for looking into it. Glad I'm not going crazy :)
@Leeds-eBooks fiddle definitely works, the bug seems to be with the rv-each and then removing an element. Its somehow always removing the last instance of the newly created object no matter which you try to splice.
I thought my second fiddle might be connected as it removes the last element when I splice the first. I guess in this case its just not re rendering the html but is redoing the binding.

[Duder-onomy]

Yeah,
Currently the each binder works nearly identical to this : http://bl.ocks.org/mbostock/3808218

If the length of the iterated item is shortened, it unbinds from the end, then rebinds all that is left.
https://github.com/mikeric/rivets/blob/master/src/binders.coffee#L189

Anyhow. Just a side note. But the fact that it pops one off the end of the iterated when you splice is expected. But it should have called update on all of the bindings that were left.
https://github.com/mikeric/rivets/blob/master/src/binders.coffee#L218

My feeling is that, because you have new'd up a object, the binding is broken. If you deconstruct the dataAttributes into separate properties on the bound component things seem to work fine. So, I am trying to figure out where the binding breaks and if it can be avoided and accounted for by rivets core.

Just a dream here, but after we all get the new ES6 branch totally pimped out, my number one goal is going to be implement the each binder more like this: http://bl.ocks.org/mbostock/3808234

[benadamstyles]

@Duder-onomy @saiphcbk do guys think this issue can be closed, or are there still questions to be resolved?

[Duder-onomy]

@Leeds-eBooks Unfortunately I think this is a real bug. I was going to look into it more. I do not fully understand why it is an issue.

[sgnix]

I have the same problem and I can attest that this is a real bug. The rv-each-* binding always removes the last element from the collection, instead of the element actually removed. I spent a couple of hours reading the Coffee scrips. This is the offensive line of code https://github.com/mikeric/rivets/blob/master/src/binders.coffee#L191.
IMNSHO, this makes the rv-each-* binding useless and it might as well be removed from Rivets. A better option would be to fix it, perhaps by adding an additional parameter holding the index of the deleted element. Alternatively, you could redraw all elements each time.

I don't mind writing my own each-* as suggested, but then what's the purpose of the stock one?

@mikeric and @Leeds-eBooks fwiw, I know a lot of people (myself included) who use Rivets. Some of them work for big corporations.

Don't give up!

PS. Rewriting for ES6 ... probably not a good idea for now.

[benadamstyles]

@Naturalist Just to confirm, is your issue regarding a list of components, or just a list of objects? If the latter, then I don't believe this can be seen as a bug – this is simply how the rv-each binder works. It has served me well for several years now.

Can you explain why rewriting in ES6 is a bad idea?

[sgnix]

@Leeds-eBooks I am using a list of objects. Digging more into the code, it looks like after the excess elements are removed (popped from the end of the list), the remaining elements are supposed to be re-rendered with new values, which works in some cases and doesn't in others. In theory it's solid and should be fine, so scratch what I said above about rv-each-* being useless. I'll keep digging and I'll post once I've found the issue.

Re: ES6 ... well, why not keep supporting what you already have, rather than rewrite it in another language and possibly introduce a number of new issues? Rewriting doesn't solve any real problems. If anything it may introduce new ones.

[benadamstyles]

Ok well let us know if you find anything that seems strange to you, if there's a bug here or unexpected behaviour then it should definitely be addressed. I don't suppose you're using an rv-if or rv-unless binder in the list?

Your reasoning is completely sound regarding the rewriting, but there is one principal reason why we are (or were, the branch seems to have stalled recently...!) rewriting it in ES6 – none of the new contributors (as far as I know) are comfortable in CoffeeScript. I for one would not even have considered becoming a contributor if the ES6 branch was not started. For Rivets to have a solid future – which I know many people really want and need – it needs contributors who can devote time and goodwill to it, and it became clear a while ago that this wasn't going to happen in CoffeeScript.

[sgnix]

I figured out what the problem is. I don't know if it's an issue at all or if it's expected behavior.

When handling collections of objects via rv-each-*, it does matter if the individual object was created using prototype or not.

So if you have a collection of Items, and Item was defined as shown below, then rv-each will not handle the rendering properly:

function Item(name) {
    this.name = name;
}

Item.prototype.upperName = function () {
    return this.name.toUpperCase();
};

However, if your Item is defined as follows, then rv-each's rendering works as expected:

function Item(k) {
  this.name = k;
  this.upperName = function() {
    return this.name.toUpperCase()
  }
}

I suspect this is also what's causing the issue reported by @saiphcbk. CoffeeScript will use prototype to create classes, which in turn makes Rivets components buggy.

It's worth noting that this is only a visual issue and it only appears when you splice the collection. This is what's happening when you splice:

1. Your collection is correctly spliced
2. Rivets triggers an update method
3. Rivets determines that N number of items have been deleted from the collection
4. Rivets removes N number of DOM elements from the end of the view representing the collection
5. Rivets updates the remaining DOM elements with the values of the (now shortened) collection

The last step is expected to update the DOM elements. It works if the model was defined as shown in code example 2, but it does not work if it's defined using prototype as shown in code example 1.

Again, I don't know if this is expected behavior, but it sucks for me because I use TypeScript and it generates prototype-based inheritance.

[Duder-onomy]

Rivets binding and property observation works by wrapping all the bound objects 'has own' properties in getters and setters. A prototype method does not fall into this category. Checkout the adapter definition:

rivets/src/adapter.coffee

Line 10 in 65cfcfd

unless obj.hasOwnProperty @id

That is funny that TypeScript does that. Composition and Inheritance are one of the coolest features of JS and seems weird that TypeScript would behoove itself to make a prototype vs instance decision for you. Seems like not a bug in Rivets, but a issue with TypeScript? Maybe there is a way to force TypeScript to do what you want?

Anyhow. You are correct about the each binder, it works identical to this: http://bl.ocks.org/mbostock/3808218

It would be non-trivial, but incredibly useful for the each binder to behave more like this: http://bl.ocks.org/mbostock/3808234 Though, rivets would need know how to determine if it is the same object. Angular has the track by expression to handle this. Looks something like, ng-repeat='item in collection track by item.id. This expression will let the user specify explicitly what properties equality makes the object the same and update, or new and add it in. The angular source code for that is pretty simple. Implementing in rivets would be a little harder. In my initial pass at implementing I tried to use an additional element attribute to signal the property on the iterated that should be used for equality. If the user was to add this rv-track-by='' binder to the same element that had the rv-each then it would properly track the collection and keep it perfectly in sync with DOM. If you can get around your TypeScript issue I would love to help you implement the each binder that stays in sync.

How this helps you.

[blikblum]

For the record i updated the fiddle to log the mutations: http://jsfiddle.net/nyjt2pox/3/. Although not sure how to fix or even exactly where the problem is

[sgnix]

@Duder-onomy Thanks for the reply! I suspected that what I'm observing is expected behavior rather than a bug. It would save many hours of debugging for someone if it was explained in the docs.
TypeScript is a superset of JavaScript, so I solved my own problem by simply writing the instance, instead of relying on TS's class keyword. Though, I've lost TS's strict type checks and such .. No big deal.

I'd be interested to see how you solved this problem using rv-track-by. There is no rush ... It would suffice if you sent me a gist.

[stalniy]

@Duder-onomy @blikblum @Leeds-eBooks @Naturalist

Has anybody fixed this?

[Duder-onomy]

@stalniy No, we have been busy with the other PR's and pushing our own 0.9 commitments forward.
This has to do with components, which I believe is the least understood part of rivets ATM.

It seems like you know how components are supposed to work better than we do (I have never used components in our apps, prefer another approach to making views) Would you like to take a stab at it?

[blikblum]

I fixed the issue in my fork. See commits:

a9e0c14
00e3495
5bccf01

[ShqipognjaLL]

A little late to the party, but just want to point out that in addition to the aforementioned issues, it appears that when the model is re-arranged , sorted or added to anywhere not at the end of the list, that the model attached to the element is also juxtaposed improperly. For example if I have

<li rv-each-item="model.items" rv-hasvaluechanged="item.bool">item.value</li>

with the model looking something like

model.items = [{value: 1, bool: true}, {value:2, bool: false}]

and the binder just logging if a change has occured in the boolean

rivets.binders.hasvaluechanged = function(el, value){
     if(value){
          console.log("it has changed!");
     }
}

If we insert an item to this list at the beginning or in between the two objects, you will notice the binder being triggered, even though the boolean shave not been changed, because the model attached to the iteration binding has been juxtaposed.

Am I using rv-each incorrectly here? Is this intended behaviour? This rv-each binder is the only sad part to this amazing library for me, it has created so many headaches :(