Skip to content
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

Using Ember.computed.sort with itemController computed properties leads to and "Index out of range" error on resort #4831

Closed
nmk opened this issue May 7, 2014 · 18 comments

Comments

@nmk
Copy link

nmk commented May 7, 2014

I have made a JSBin to illustrate the issue: http://emberjs.jsbin.com/zidonulu/1/edit

As you can see the default sort is working when initially displaying the output.

After clicking on the "City" button, the gasConsumption values are correctly updated but the order of the list is not changed. I would expect it to change to reflect the new property values.

If you then click on "Highway" and then on "City" again an "Index out of range" error is thrown.

Is this a bug I am hitting, or am I using the sorting mechanism in a wrong way?

@GetContented
Copy link

I don't see a city button?

@nmk
Copy link
Author

nmk commented May 7, 2014

I am sorry, it seems JSBin hadn't saved yet. It is there now.

@GetContented
Copy link

Weird... when i click between them a few times, i end up with this:

Beta: 4.8
Gamma: 6.1
Gamma: 6.1

@nmk
Copy link
Author

nmk commented May 7, 2014

Yep, that is also then the "Index out of range" error is thrown. Do you see an error in the way I am using Ember.computed.sort?

@GetContented
Copy link

Sorry this is a bit beyond me. It seems you're sorting using :asc and a computed property and that's not mentioned in anything I could find on the net or in the API. Seems like you've found a bug to me, but I didn't even know that your usage was correct until just now!

Having said that, I'm not 100% sure your code is matching up with the documentation provided at reduceComputed... http://emberjs.com/api/#method_reduceComputed and computed.sort - it might be best to provide a function rather than use a static sort order if you're going to change the sort direction... I'm not sure it's supposed to be used the way you're using it, or even if your use would work correctly... where did you get the ":asc" syntax from??

@nmk
Copy link
Author

nmk commented May 7, 2014

I saw the example here: #4308 (comment)

@GetContented
Copy link

Cool :) Maybe we should ask @LevelbossMike - either way, I don't think it's a great way to do something where the sort order is changing, but like I said I'm by no means an expert on this stuff. Sorry for bringing noise!

@nmk
Copy link
Author

nmk commented May 9, 2014

I have found a solution to this: http://emberjs.jsbin.com/zidonulu/7/edit

It seems that Em.computed.sort expects the property of each itemController on which one is sorting to change one by one, and not all at the same time. Is this the desired behaviour?

@endash
Copy link
Contributor

endash commented May 23, 2014

Running into this now that I've replaced using sortProperties on the array controller (because it destroys controllers on resort) with computed.sort. Can't win :/. Solution (for me) was to eliminate the changeProperties and just let the 1x1 behaviour happen.

@williamli
Copy link

confirmed hitting this error as well.
also when the content of the array changes (which will trigger a resort), the index will go out of range eventually as well.

@lulezi
Copy link

lulezi commented Aug 26, 2014

I ran into this issue a couple of times lately, so I'd be quite interested in a fix. I'll also gladly help fixing it if anyone could give me a hint where to start.

@jwahdatehagh
Copy link

+1

@wagenet wagenet added the Bug label Sep 12, 2014
@alvincrespo
Copy link
Contributor

👍

@courthead
Copy link

+1. This is a pretty nasty bug, especially considering the fact that almost every web app needs to render sorted lists. It's bitten me in literally every project I've ever worked on with Ember.

EDIT: So I got around this bug by taking @nmk's comment to heart and ensuring that any updates to an array that's subject to Ember.computed.sort happen one item at a time. So instead of replacing an entire array, I go through add/remove items one at a time, ensuring not to wrap this process in anything like changeProperties (which would cause the updates to happen en masse rather than individually). Alternatively, you can use the fix from @nmk's JS Bin, which avoids using Ember.computed.sort altogether.

@gone
Copy link

gone commented Sep 24, 2014

+1 :(

@wagenet
Copy link
Member

wagenet commented Nov 1, 2014

There are definitely some known issues with ArrayComputed which we're thinking about solutions for.

@wagenet wagenet closed this as completed Nov 1, 2014
@wagenet wagenet reopened this Nov 1, 2014
@e00dan
Copy link
Contributor

e00dan commented Dec 19, 2014

What's the status of this issue?

@stefanpenner
Copy link
Member

ember 2.0 removes the concept of controllers and itemControllers.

stefanpenner added a commit that referenced this issue Jun 23, 2015
#9492, #5319, #5268, #4831] Move away from AC/RC instead use the simpler naive enumerable methods, and rely on glimmers stable rendering for efficiency.

For more complex scenarios, custom solutions should be used.

@wagenet & @stefanpenner
stefanpenner added a commit to stefanpenner/ember.js that referenced this issue Jun 26, 2015
…rjs#9462, emberjs#4919, emberjs#4231, emberjs#3706, emberjs#5596, emberjs#9485, emberjs#9492, emberjs#5319, emberjs#5268, emberjs#4831, emberjs#5558] Move away from AC/RC instead use the simpler naive enumerable methods, and rely on glimmers stable rendering for efficiency.

For more complex scenarios, custom solutions should be used.

@wagenet & @stefanpenner
stefanpenner added a commit that referenced this issue Jun 28, 2015
#5596, #9485, #9492, #5319, #5268, #4831, #5558] Move away from AC/RC instead use the simpler naive enumerable methods, and rely on glimmers stable rendering for efficiency.

For more complex scenarios, custom solutions should be used.

@wagenet & @stefanpenner

(cherry picked from commit 0dc1a6c)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests