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

fix(Angular.js): Improve logic powering isArrayLike() #2878

Closed

Conversation

its-your-favorite
Copy link

Enable isArrayLike() to correctly identify one-off-arrays (aka
'subclasses').
Thus array-like objects with enhanced prototypes can be used with
ng-repeat.
Previously, they would be misidentified as objects and not iterate
correctly.

Enable isArrayLike() to correctly identify one-off-arrays (aka
'subclasses').
Thus array-like objects with enhanced prototypes can be used with
ng-repeat.
Previously, they would be misidentified as objects and not iterate
correctly.
@dcherman
Copy link
Contributor

Rather than make my own PR I figured I'd add this on here - I'm wondering it'd be worth using the battle tested definition of arrayLike from jQuery ( https://github.com/jquery/jquery/blob/master/src/core.js#L828-L843 ).

@its-your-favorite
Copy link
Author

So I tested jQuery's function and it works for my test case too, so that would be a satisfactory for my use case.

I'm not sure why this hasn't gotten any response yet. Should I make a test-case to justify this PR?

@pkozlowski-opensource
Copy link
Member

PR Checklist (Minor Feature)

  • Contributor signed CLA now or in the past (if you just signed, leave a comment here with your real name for cross reference)
  • Feature improves existing core functionality
  • API is compatible with existing Angular apis and relevant standards (if applicable)
  • PR doesn't contain a breaking change
  • PR contains unit tests
  • PR contains e2e tests (if suitable)
  • PR contains documentation update
  • PR passes all tests on Travis (sanity)
  • PR passes all tests on ci.angularjs.org (cross-browser compatibility)
  • PR is rebased against recent master
  • PR is squashed into one commit per logical change
  • PR's commit messages are descriptive and allows us to autogenerate release notes (required [commit message

@pkozlowski-opensource
Copy link
Member

@anfurny Thnx for this PR. To move it further we will need unit tests. Also please have a look at the checklist above.

@pkozlowski-opensource
Copy link
Member

@anfurny ping. Are you planning to add unit tests for this PR?

@chirayuk
Copy link
Contributor

Ping …

@dcherman
Copy link
Contributor

I'll do this sometime next week if tests aren't added between now and then, just didn't want to steal @anfurny 's thunder here.

@petebacondarwin
Copy link
Contributor

Closing in favour of #3356, since that one has unit tests.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants