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

fix(filter): Don't throw key.charAt is not a function when object's keys are not of type string. #15660

Closed
wants to merge 2 commits into from

Conversation

frederikprijck
Copy link
Contributor

@frederikprijck frederikprijck commented Feb 1, 2017

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Bug fix

What is the current behavior? (You can also link to an open issue here)
When an object has keys which are not of type string, the filter would throw an exception for trying to call charAt, which is not defined on a none-string type.

#15644

What is the new behavior (if this is a feature change)?
This commit checks whether charAt is defined before calling it.

Does this PR introduce a breaking change?
No

Please check if the PR fulfills these requirements

Other information:

@frederikprijck frederikprijck changed the title filter(filter): Don't throw key.charAt is not a function when object's keys are not of type string. fix(filter): Don't throw key.charAt is not a function when object's keys are not of type string. Feb 1, 2017
… keys are not of type `string`.

Previously, when an object has keys which are not of type string, the `filter` would throw an exception for trying to call `charAt`, which is not defined on a none-string type.

This commit checks whether `charAt` is defined before calling it.

Closes #15644
@Narretz
Copy link
Contributor

Narretz commented Feb 1, 2017

Can you please add a test for this?

@gkalpak
Copy link
Member

gkalpak commented Feb 1, 2017

I am afraid it is not possible (as discussed in #15644) 😁
I assume the Closure Compiler is manipulating the source code to iterate over the Map's keys using its own methods, instead of the standard for-in construct.

Unless we messed with the String prototype and hope that other parts of the code don't rely on it. But it is not worth it imo.

@@ -226,7 +226,7 @@ function deepCompare(actual, expected, comparator, anyPropertyKey, matchAgainstA
var key;
if (matchAgainstAnyProp) {
for (key in actual) {
if ((key.charAt(0) !== '$') && deepCompare(actual[key], expected, comparator, anyPropertyKey, true)) {
if (key.charAt && (key.charAt(0) !== '$') && deepCompare(actual[key], expected, comparator, anyPropertyKey, true)) {
Copy link
Member

Choose a reason for hiding this comment

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

Add a comment (and maybe a link to the issue) for context. Something like:

Under certain, extreme circumstances, key may not be a string. See ...`

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 added some comment.

@gkalpak
Copy link
Member

gkalpak commented Feb 1, 2017

LGTM if it LGT @Narretz 😃

@Narretz
Copy link
Contributor

Narretz commented Feb 8, 2017

LGTM

@gkalpak gkalpak closed this in 2af2607 Feb 8, 2017
gkalpak pushed a commit that referenced this pull request Feb 8, 2017
Previously, when an object has keys which are not of type string, `filterFilter`
would throw an exception for trying to call `key.charAt()`, which is a string
method.

This commit checks whether `charAt` is defined before calling it.

Fixes #15644

Closes #15660
ellimist pushed a commit to ellimist/angular.js that referenced this pull request Mar 15, 2017
Previously, when an object has keys which are not of type string, `filterFilter`
would throw an exception for trying to call `key.charAt()`, which is a string
method.

This commit checks whether `charAt` is defined before calling it.

Fixes angular#15644

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

Successfully merging this pull request may close these issues.

4 participants