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

Ember.computed.sort fails #11252

Closed
jamiechong opened this issue May 22, 2015 · 6 comments
Closed

Ember.computed.sort fails #11252

jamiechong opened this issue May 22, 2015 · 6 comments

Comments

@jamiechong
Copy link

Firebase claims that their unique ids are naturally sorted. However, with a hasMany recordset sorted ascending I get the wrong order.

This is a snippet of my Model

    tables: DS.hasMany('table', {async: true}),
    tablesAscById: ['id:asc'],
    tablesAsc: Ember.computed.sort('tables', 'tablesAscById'), 

In Firebase I have these table IDs (sorted asc):

-JptZAW2fIGBytToSEUd
-Jpta_VvP4KPqpYBXXuf
-JptcVIBawyPhhfC1BWp
-Jptd8Bf8zPInsOnSZBw
-JptdArBnF_cokaYfBde

The tablesAsc computed should give me the same order, but instead it gives me this:

-Jpta_VvP4KPqpYBXXuf
-JptcVIBawyPhhfC1BWp
-Jptd8Bf8zPInsOnSZBw
-JptdArBnF_cokaYfBde
-JptZAW2fIGBytToSEUd

Interestingly when using Javascript Sort, nothing changes (as expected):

var arr = ["-JptZAW2fIGBytToSEUd", "-Jpta_VvP4KPqpYBXXuf", "-JptcVIBawyPhhfC1BWp", "-Jptd8Bf8zPInsOnSZBw", "-JptdArBnF_cokaYfBde"];
arr.sort(); // maintains the same order (as it should)
Ember      : 1.12.0-beta.1
Ember Data : 1.0.0-beta.17
Firebase   : 2.2.4
EmberFire  : 0.0.0
jQuery     : 1.11.3
@jamiechong
Copy link
Author

Can anyone else confirm this bug? I've created a JSBin of the problem here:
http://emberjs.jsbin.com/mudimuzago/1/edit?html,js,output

@jamiechong jamiechong changed the title Ember.computed.sort fails for Firebase Lexicographical ids Ember.computed.sort fails Jun 20, 2015
@mmun
Copy link
Member

mmun commented Feb 21, 2016

Hi @jamiechong. Sorry for the ridiculously long response time! I can confirm your report.

The problem lies with Ember.compare. In JavaScript, the follow statements are all true:

'A' < 'Z' // true
'Z' < 'a' // true
'a' < 'z' // true

however using Ember.compare gives

Ember.compare('A', 'Z') // -1
Ember.compare('Z', 'a') // 1, should be -1
Ember.compare('a', 'z') // -1

This is because Ember.compare delegates to String#localeCompare() rather than <. It's hard to say if this behaviour is desirable or not. I think most people would expect it to sort by <. Unfortunately, there are probably a few apps out there that rely on the current behavior.

What do you think @stefanpenner / @rwjblue?

@mmun mmun added the Bug label Feb 21, 2016
@rwjblue
Copy link
Member

rwjblue commented Feb 21, 2016

I'm unsure. It definitely seems like a bug to me, and we should probably fix it. We can land a fix on canary and let it live through a full cycle to see if it impacts folks...

duggiefresh added a commit to duggiefresh/ember.js that referenced this issue Feb 24, 2016
instead of String#localeCompare

There are edge cases where String#localeCompare behaves unexpectedly,
so, we'll use relational operators instead to compare Strings.

Closes emberjs#11252
@krisselden
Copy link
Contributor

This is not a bugfix, it is a breaking change to make string compare not use the default locale, many locales use case insensitive compares. A locale compare is not going to work for comparing strings that should use a locale invariant.

@krisselden krisselden reopened this Mar 2, 2016
@rwjblue
Copy link
Member

rwjblue commented Mar 2, 2016

If the fix was invalid, then I believe this issue cannot be fixed in a locale aware way (i.e. we can make it work for ascii/English but not across the board). If that is the case, this issue is basically a CANTFIX, right?

@krisselden
Copy link
Contributor

@rwjblue it is a can't fix, but one could either do an addon or request a locale invariant version of this API. You would need a compareInvariant and a sortInvariant or something like that, but I don't see how this can be changed other than adding a new API or some kind of flag. Same thing you'd have to do if you wanted to use Intl.Collator.

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

4 participants