add iterator functionality and tests #235
Conversation
| push(null, _.nil); | ||
| } | ||
| else if (iterElem.done) { | ||
| if (iterElem.value) { |
There was a problem hiding this comment.
This should check specifically for undefined right? All other falsy values should be passed on.
There was a problem hiding this comment.
Also, add a test for this case. done === true and value === false or something.
There was a problem hiding this comment.
Sorry, yeah, that's a mistake. Whendone gets set to true value will always be undefined unless we're dealing with an iterator from a generator that uses the return keyword. I should be checking for the specific case of when done is true and value is not undefined. Not sure why there needs to be a test for value being false though?
There was a problem hiding this comment.
Just a regression test for the corner case. To make sure that we correctly pass through the value when it is not undefined even though it == false.
I think the spec allows any iterator to specify a return value and not just one that vomes from a generator.
There was a problem hiding this comment.
Ah sorry, I thought you meant a test in the generator function. No probs.
|
Every iterable has a I'm curious, does a try-catch deoptimize all enclosing functions, not just the innermost one? That's interesting if true. |
| }; | ||
|
|
||
| //ES6 iterators Begin | ||
| if(global.Map && global.Symbol) { |
There was a problem hiding this comment.
Probably no need to check for Symbol if you don't use Symbol.iterator (my original comment assumed that you would).
Also style note: space in between if and first parentheses (here and elsewhere).
There was a problem hiding this comment.
Actually, that's a good point we should just default to the Symbol.iterator property if we're given an iterable. I think it makes sense to support both as they're two sides of the same coin. One thing though, when I do something like:
else if (typeof Symbol !== 'undefined' && xs[Symbol.iterator]) {
return iteratorGenerator(xs[Symbol.iterator]());
}ESLint gives me an error saying Symbol is not defined, should I be checking for global variables using the _global object where the global nil object gets deined?
Also, I think you're right about the deoptimisation only applying to the inner function, I was misremembering that Bluebird performance doc.
There was a problem hiding this comment.
Yeah, it think just using _global would work.
svozza
force-pushed
the
130-accept-iterators-as-source
branch
from
318d410 to
d526171
Compare
| //probably a promise | ||
| return promiseGenerator(xs); | ||
| } | ||
| else if (_.isFunction(xs.next)) { |
There was a problem hiding this comment.
Oh, and maybe a comment here saying not to swap the order of the iterator and iterable checks.
Generators are both, and their Symbol.iterator returns this, so swapping the checks will result in an infinite loop.
svozza
force-pushed
the
130-accept-iterators-as-source
branch
from
e2f5cd5 to
e1a5a3a
Compare
add node v0.12 test env add iterable support use falsy value in custom iterator test update comments/docs and add check undefined function fix typo use of Set in docs was incorrect
svozza
force-pushed
the
130-accept-iterators-as-source
branch
from
e1a5a3a to
f4b070f
Compare
| push(null, _.nil); | ||
| } | ||
| else if (iterElem.done) { | ||
| if (iterElem.value !== void 0) { |
|
Alright. Thanks for bearing with me! :) This is awesome, by the way. |
Add support for constructing from iterator/iterable. Resolves #131.
|
Not all all, a proper and thorough code review saves so much pain down the road. :) |
|
Just noticed a problem with the docs, the link to the iterator protocol wasn't being created properly. I've fixed it but will I reopen and commit the change to this branch or just create a new PR? |
|
Just make a new PR. |
Resolves #131. I separated the iterator functionality out into a private function so that the try/catch block doesn't cause the compiler to deoptimise the whole constructor. I was also thinking it might be nice to just be able to pass an ES6 collection like a map or a set in directly and automatically have the stream return the entries but I don't think we guarantee that every iterable will have a
.entries()function so I just left it.