Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

Allow ordering for Array inheritors #8944

Closed
wants to merge 3 commits into from

Conversation

tsevan
Copy link
Contributor

@tsevan tsevan commented Sep 5, 2014

Hello,

yesterday I faced an issue with angular.
We use Coffeescript and its classes very widely in our rails project. We have a BaseCollection class, which was inherited from Array. It works cool, but there's one negative effect: I was unable to order this object with ng-repeat and orderBy.

I started digging and revealed, that angular rejects any objects, but Arrays itself. The problem was that Array.isArray does not define Array inheritors as arrays, while when I inherite from class I must follow the Liskov substitution principle and not to break Array functionality (and we do in our project).

Here's a short demonstration of the issue. Issue can be reproduced in 1.2.x, but in 1.1.x everything works fine.

I suggest to allow array-like objects to be sorted. I've also added a test to cover this and checked the green-red-green process with grunt test:jqlite. Please take a look at my fix or fix that issue by yourself in more diligent way.

@mary-poppins
Copy link

I'm sorry, but I wasn't able to verify your Contributor License Agreement (CLA) signature. CLA signature is required for any code contributions to AngularJS.

Please sign our CLA and ensure that the CLA signature email address and the email address in this PR's commits match.

If you signed the CLA as a corporation, please let us know the company's name.

Thanks a bunch!

PS: If you signed the CLA in the past then most likely the email addresses don't match. Please sign the CLA again or update the email address in the commit of this PR.
PS2: If you are a Googler, please sign the CLA as well to simplify the CLA verification process.

@mary-poppins
Copy link

CLA signature verified! Thank you!

Someone from the team will now triage your PR and it will be processed based on the determined priority (doc updates and fixes with tests are prioritized over other changes).

@@ -115,7 +115,7 @@
orderByFilter.$inject = ['$parse'];
function orderByFilter($parse){
return function(array, sortPredicate, reverseOrder) {
if (!isArray(array)) return array;
if (!(isArray(array) || array instanceof Array)) return array;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you consider using isArrayLike()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been trying, and it works well, so I've switched to isArrayLike(), since it's a common way for the Angular projects, as far as I understand. Thanks.

@petebacondarwin petebacondarwin added this to the 1.3.0 milestone Sep 5, 2014
@petebacondarwin petebacondarwin self-assigned this Sep 5, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants