Angular.js: AngularJS is throwing "Error: key.charAt is not a function" on Firefox when $filter is iterating over some kind of strange Object

[seriesoftubes]

Summary
When $filter is used against an array of custom objects, I received this console error in 1.5:
Error: key.charAt is not a function
deepCompare@https://cdnjs.cloudflare.com/ajax/libs/angular.js/1.5.11/angular.js:20587:16
deepCompare@https://cdnjs.cloudflare.com/ajax/libs/angular.js/1.5.11/angular.js:20587:42
deepCompare@https://cdnjs.cloudflare.com/ajax/libs/angular.js/1.5.11/angular.js:20587:42
createPredicateFn/predicateFn@https://cdnjs.cloudflare.com/ajax/libs/angular.js/1.5.11/angular.js:20562:12
filterFilter/<@https://cdnjs.cloudflare.com/ajax/libs/angular.js/1.5.11/angular.js:20526:12

And in 1.6:
Error: key.charAt is not a function
deepCompare@https://cdnjs.cloudflare.com/ajax/libs/angular.js/1.6.1/angular.js:20564:16
deepCompare@https://cdnjs.cloudflare.com/ajax/libs/angular.js/1.6.1/angular.js:20564:42
deepCompare@https://cdnjs.cloudflare.com/ajax/libs/angular.js/1.6.1/angular.js:20564:42
createPredicateFn/predicateFn@https://cdnjs.cloudflare.com/ajax/libs/angular.js/1.6.1/angular.js:20539:12
filterFilter/<@https://cdnjs.cloudflare.com/ajax/libs/angular.js/1.6.1/angular.js:20503:12
anonymous/fn@https://cdnjs.cloudflare.com/ajax/libs/angular.js/1.6.1/angular.js line 15156 > Function:2:489
regularInterceptedExpression@https://cdnjs.cloudflare.com/ajax/libs/angular.js/1.6.1/angular.js:16264:55

Steps to repro:
I saw this bug when iterating over a goog.structs.Map, and noticed that key was actually a number. Sorry, I can't reproduce this without figuring out how to pull in closure on plnkr. And it is still hard for me to believe that a for (var key in obj) loop can produce non-string keys, so I don't know how else to possibly reproduce it.

The best I can say is that this definitely was an issue on >= Angular 1.5 on Firefox (but not Chrome). I'm not sure about IE.

Possible fix:
checking for typeof key === 'string'
here: https://github.com/angular/angular.js/blob/master/src/ng/filter/filter.js#L229

[gkalpak]

If we can't rely on Object keys being strings, where is this world headed?

I assume that somehow iterating over the keys of a goog.struct.Map with for ... in returns the actual Map keys (either via __iterator__ or getKeys), which can be of any type.
(If my assumption is correct, Closure isn't palying very nice here imo 😞).

[seriesoftubes]

I think that must be it. I'm not familiar with iterators' behavior, but my goog.structs.Map did have numeric keys in the this.keys_ array, and I guess each one of those was being returned by the iterator.

[gkalpak]

How about we change key.charAt(0) with key[0]. Would that work for you (as in "not throw errors"), @seriesoftubes?

[lgalfaso]

Or someone needs to poke johnlenz
https://github.com/google/closure-library/blob/master/closure/goog/structs/map.js#L305

[seriesoftubes]

key[0] would work for me personally, but would not work for keys that are null or undefined.

[gkalpak]

but would not work for keys that are null or undefined

True!

I am a little on the fence with this one. I don't want to introduce extra work for every filtering operation, just to support some dubious, ultra-edgy usecase, where keys are not strings 😕

#

@seriesoftubes, if you want to put together a PR (using key.charAt && key.charAt(0) ...), we could definitely consider it. (BTW, I don't think there is any way to test this 😛)

[seriesoftubes]

I agree-- key[0] should be perfectly fine in the vast majority of cases. I have no idea why they chose .charAt(0) in the first place. Apparently key[0] is roughly identical performance-wise to key.charAt(0) and at least on modern Chrome, the output is consistent too, even with UTF8 characters.

I wish I had more time to file a proper PR-- I opened this issue hoping that there would be an easier way for someone else to fix it.

[gkalpak]

No worries! Soemone will take care of it.

#

So, if anyone is up for a little PR, without a way to test this (so no tests to be added), adding a key.charAt check to this line is as easy as it can get:

-if ((key.charAt(0) !== '$') && deepCompare(actual[key], expected, comparator, anyPropertyKey, true)) {
+if (key.charAt && (key.charAt(0) !== '$') && deepCompare(actual[key], expected, comparator, anyPropertyKey, true)) {

Writing the commit message (and explaining how an Objects key can be a non-String value) will be the tough part 😛