Observe.List sort plugin erroring on item removal

[daffl]

Ran into this when testing some new functionality when implementing the sort plugin for Observe.List (in conjunction on a Model.List).

When removing the last item in the list, an error is thrown stating that the "item is undefined".

In "can/observe/sort/sort.js", lines 142-170:

proto._changes = function(ev, attr, how, newVal, oldVal){
    if(this.comparator && /^\d+./.test(attr) ) {
        // get the index
        var index = +/^\d+/.exec(attr)[0],
            // and item
            item = this[index],
            // and the new item
            newIndex = this.sortedIndex(item);

        if(newIndex !== index){
            // move ...
            [].splice.call(this, index, 1);
            [].splice.call(this, newIndex, 0, item);

            can.trigger(this, "move", [item, newIndex, index]);
            ev.stopImmediatePropagation();
            can.trigger(this,"change", [
                attr.replace(/^\d+/,newIndex),
                how,
                newVal,
                oldVal
            ]);

            return;
        }
    }

    _changes.apply(this, arguments);
};

At line 149, newIndex = this.sortedIndex(item) throws an error because item doesn't exist in the List any longer. A little check to see if typeof item != "undefined" fixes the issue.

Revised method:

proto._changes = function(ev, attr, how, newVal, oldVal){
    if(this.comparator && /^\d+./.test(attr) ) {
        // get the index
        var index = +/^\d+/.exec(attr)[0],
            // and item
            item = this[index];

        if( typeof item != 'undefined') {
          // and the new item
          var newIndex = this.sortedIndex(item);

          if(newIndex !== index){
            // move ...
            [].splice.call(this, index, 1);
            [].splice.call(this, newIndex, 0, item);

            can.trigger(this, "move", [item, newIndex, index]);
            ev.stopImmediatePropagation();
            can.trigger(this,"change", [
                                        attr.replace(/^\d+/,newIndex),
                                        how,
                                        newVal,
                                        oldVal
                                        ]);

            return;
          }
        }
    }

    _changes.apply(this, arguments);
};

[ralphholzmann]

Hey @trickeyone,

Thanks for the bug report. I can't seem to reproduce this bug writing a simple test. Can you tell me or give me an example of how you're removing the last item in the list to trigger this error?

Thanks

[trickeyone]

Hi @ralphholzmann,

I tried to make a jsfiddle to present a test case, but some of the needed model functionality isn't included in the built CanJS libs.

http://jsfiddle.net/trickeyOne/ZBp45/

I tried to describe what was throwing the error and where via some comments in the controller.

[ralphholzmann]

I'm sorry @trickeyone but you'll have to provide a better reduced test case. I can't seem toe figure out where your error is cropping up.

[justinbmeyer]

Model lists automatically remove destroyed models. This is probably the problem. However, I can't tell because your example is complex to follow. If you could clean it up, that would help.

What needed model functionality? You can add every model/observe plugin in a fiddle.

[trickeyone]

Hi @ralphholzmann,

Could you suggest how I could provide a better test case? Since the functionality isn't included in the built version of CanJS, it's difficult to provide a full example.

Thanks.

[justinbmeyer]

your example sets up fixtures, controls, models, etc, when all it probably needs is model.List. Remove the unnecessary code.

[trickeyone]

The functionality that is missing is the Model.destroy method. In my method I'm just doing a this.options.model.Children.match('id', id) then looping through it to call the destroy on the child. Upon destroying, the sort plugin is being called again because there's been a change to the List. When it does this, it's trying to find an item with the specified index (attr) but it has been removed from the list and was the last item in the list. This is causing the item to be set to null. When this happens and sortedIndex is called with the item of null, sortedIndex is throwing an error because it's trying to do this: itemCompare = item.attr(this.comparator) at line 16.

If you can tell me how I can pull in the sort plugin and how to get the destroy method to show up on my fiddle, I can make a more apt example.

[justinbmeyer]

You can use steal to load from http://donejs.com

<script src="http://donejs.com/steal/steal.js">
</script>

  steal('can/observe/sort', function(){

  })

[trickeyone]

Ah, ok. So, I shouldn't use the latest releases (i.e. http://canjs.us/release/latest/can.jquery.js ) ?

[justinbmeyer]

That's normally fine. But you are using an unreleased plugin. There's not a packaged JS file you can use.

[trickeyone]

Ok. I'm working an update of the fiddle. It's still not simple, but that's because of the inter-dependencies of the whole setup. i.e. Parent->child stuff.

[trickeyone]

Ok, if you take a look at the fiddle again, you'll see the error at work. Click the "[x]" next to the last item in the list and you'll see the error happen.

http://jsfiddle.net/trickeyOne/ZBp45/

[justinbmeyer]

Can you remove fixtures and the need to click something? Can't you just create a model list like:

can.Model("Foo")
var mylist = Foo.models({id: 1},{id: 2})

// do whatever you need to break it

[justinbmeyer]

Another thing, why 20 objects, this probably happens with 2. Apologies about being so demanding about this, but @ralphholzmann and I are crazy busy with our client work. It helps us a lot if we can quickly get some small code that shows the problem. Something like:

can.Model("Foo",{
  destroy: function(){
    return new $.Deferred().resolve()
  }
})
var mylist = Foo.models({id: 1},{id: 2})

mylist[1].destroy()

But looking at the error, it seems that sort code is trying to run on a "remove" event. That's not only wrong because it's trying to find an item that's been removed, it's unnecessary because the array should already be sorted.

[trickeyone]

I can't make it simple as that because it's dependent on calling the Model.destroy method. So, the fixtures are needed to return a successful call. I've update the fiddle to just do a quick removal of a random item from the list, then the last item in the list. You'll see that it successfully removes the random item, but errors on the last item. I tried to put in some error catching, but it's not working since it's a deferred. You'll need to watch Firebug or some console in the browser to see the error that is thrown.

Why 20? I just picked the number at random. Only two items means there would only be 2 attempts. The updated one it a smaller run at it. I trimmed it down as much as I could but still produce the bug. There is some setup required, but only because of dependencies and data.

[justinbmeyer]

why are fixtures needed? Why can't you have destroy do what I showed in the example above?

[justinbmeyer]

I showed my model with the wrong static props. It should be:

can.Model("Foo",{
  destroy: function(){
    return new $.Deferred().resolve()
  }
},{})

That should work w/o having to create fixtures.

[justinbmeyer]

What is meant by:

The updated one it a smaller run at it.

[trickeyone]

Sorry, I meant "The updated fiddle is a smaller example"

[justinbmeyer]

were you ever able to figure this out / make it smaller?

[dispatchrabbi]

Gonna bump this and see if you've been able to make a smaller test yet, trickeyone.

[trickeyone]

I apologize, I haven't had a chance to make another test. Admittedly, I'm not great at writing test cases, and I haven't used the test framework for CanJS before. I can say that adding the small if-check as stated in the OP did fix the issue and I have not had any problems since.

[daffl]

I would like to close this issue. The suggested fix also works with the new tests and the updated sort plugin (which didn't really work as expected before anyway, see #170, #205 and #219). I recommend the fix as in the attached pull request without adding extra tests.