New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Observe.List sort plugin erroring on item removal #88

Merged
merged 1 commit into from Jan 18, 2013

Conversation

Projects
None yet
5 participants
@daffl
Contributor

daffl commented Jan 16, 2013

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

This comment has been minimized.

Show comment
Hide comment
@ralphholzmann

ralphholzmann Aug 14, 2012

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

ralphholzmann commented Aug 14, 2012

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

This comment has been minimized.

Show comment
Hide comment
@trickeyone

trickeyone Aug 14, 2012

Contributor

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.

Contributor

trickeyone commented Aug 14, 2012

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

This comment has been minimized.

Show comment
Hide comment
@ralphholzmann

ralphholzmann Aug 14, 2012

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.

ralphholzmann commented Aug 14, 2012

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

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Aug 14, 2012

Contributor

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.

Contributor

justinbmeyer commented Aug 14, 2012

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

This comment has been minimized.

Show comment
Hide comment
@trickeyone

trickeyone Aug 14, 2012

Contributor

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.

Contributor

trickeyone commented Aug 14, 2012

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

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Aug 14, 2012

Contributor

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

Contributor

justinbmeyer commented Aug 14, 2012

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

@trickeyone

This comment has been minimized.

Show comment
Hide comment
@trickeyone

trickeyone Aug 14, 2012

Contributor

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.

Contributor

trickeyone commented Aug 14, 2012

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

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Aug 14, 2012

Contributor

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

<script src="http://donejs.com/steal/steal.js">
</script>
  steal('can/observe/sort', function(){

  })
Contributor

justinbmeyer commented Aug 14, 2012

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

This comment has been minimized.

Show comment
Hide comment
@trickeyone

trickeyone Aug 14, 2012

Contributor

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

Contributor

trickeyone commented Aug 14, 2012

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

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Aug 14, 2012

Contributor

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

Contributor

justinbmeyer commented Aug 14, 2012

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

@trickeyone

This comment has been minimized.

Show comment
Hide comment
@trickeyone

trickeyone Aug 14, 2012

Contributor

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.

Contributor

trickeyone commented Aug 14, 2012

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

This comment has been minimized.

Show comment
Hide comment
@trickeyone

trickeyone Aug 14, 2012

Contributor

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/

Contributor

trickeyone commented Aug 14, 2012

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

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Aug 14, 2012

Contributor

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
Contributor

justinbmeyer commented Aug 14, 2012

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

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Aug 14, 2012

Contributor

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.

Contributor

justinbmeyer commented Aug 14, 2012

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

This comment has been minimized.

Show comment
Hide comment
@trickeyone

trickeyone Aug 14, 2012

Contributor

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.

Contributor

trickeyone commented Aug 14, 2012

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

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Aug 15, 2012

Contributor

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

Contributor

justinbmeyer commented Aug 15, 2012

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

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Aug 15, 2012

Contributor

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.

Contributor

justinbmeyer commented Aug 15, 2012

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

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Aug 15, 2012

Contributor

What is meant by:

The updated one it a smaller run at it.

Contributor

justinbmeyer commented Aug 15, 2012

What is meant by:

The updated one it a smaller run at it.

@trickeyone

This comment has been minimized.

Show comment
Hide comment
@trickeyone

trickeyone Aug 15, 2012

Contributor

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

Contributor

trickeyone commented Aug 15, 2012

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

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Oct 30, 2012

Contributor

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

Contributor

justinbmeyer commented Oct 30, 2012

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

@dispatchrabbi

This comment has been minimized.

Show comment
Hide comment
@dispatchrabbi

dispatchrabbi Nov 13, 2012

Contributor

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

Contributor

dispatchrabbi commented Nov 13, 2012

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

@trickeyone

This comment has been minimized.

Show comment
Hide comment
@trickeyone

trickeyone Nov 14, 2012

Contributor

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.

Contributor

trickeyone commented Nov 14, 2012

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.

@ghost ghost assigned daffl Dec 18, 2012

WearyMonkey pushed a commit to WearyMonkey/canjs that referenced this pull request Dec 31, 2012

@daffl

This comment has been minimized.

Show comment
Hide comment
@daffl

daffl Jan 16, 2013

Contributor

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.

Contributor

daffl commented Jan 16, 2013

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.

daffl added a commit that referenced this pull request Jan 18, 2013

Merge pull request #88 from bitovi/observe-sort-error-88
Observe.List sort plugin erroring on item removal

@daffl daffl merged commit f3518b7 into master Jan 18, 2013

1 check passed

default The Travis build passed
Details

@daffl daffl deleted the observe-sort-error-88 branch Jan 18, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment