-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
[GLIMMER] Ensure objects with .forEach
can be iterated.
#13904
Conversation
@@ -175,6 +175,12 @@ class Iterable { | |||
return new EmberArrayIterator(iterable, keyFor); | |||
} else if (Array.isArray(iterable)) { | |||
return iterable.length > 0 ? new ArrayIterator(iterable, keyFor) : EMPTY_ITERATOR; | |||
} else if (iterable.forEach !== undefined) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it better to test for typeof iterable.forEach === "function"
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unsure, but that seems fine too. I will change it.
077a872
to
6659dc3
Compare
iterable.forEach(function(item) { | ||
array.push(item); | ||
}); | ||
return array.length > 0 ? new ArrayIterator(array, keyFor) : EMPTY_ITERATOR; | ||
} else if (isEachIn(ref)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isEachIn
probably needs to be moved up – if you do {{#each-in someArray ...}}
for some reason, it should do what you asked for instead of looping through the array. It's largely unrelated to what you are doing here because even if you move it above your branch it will still be a problem, but we should remember to fix it and add a test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, I'll follow up with that in a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was supported (though untested) in HTMLBars. Added some tests and implemented in Glimmer.