Should it support non-iterable iterators?

[zloirock]

Iterators do not have to be iterable. However, with the current logic

Iterator.concat({ next() { while (true) return { value: Math.random(), done: !!(Math.random() > .8) } } });

will throw an error.

The namespace implies that the method should operate on iterators, not only iterables.

[bakkot]

Yes, this was intentional.

The problem with accepting iterators is that it becomes this function's responsibility to close them, and the logic for that gets quite complicated especially when combined with the validating-up-front behavior.

It's easy enough to wrap with Iterator.from if you have an iterator like this and are OK with it not being closed, and you'll get an error right away rather than only once the result iterator actually gets there, so hopefully it shouldn't cause much trouble in practice. Also, this is not the only static method on Iterator which will accept non-Iterator arguments; Iterator.range will take numbers.

[zloirock]

I'm unsure that Iterator.range is a good analogy. Iterator.range produces iterators, the types of arguments unrelated to iterators at all, and here makes no sense. But { Array, String }#concat accepts arrays and strings respectively (sure, not only and with some additions / extra logic), it's strange if Iterator.concat will not accept iterators with the proper iterator interface, only iterables.

[bakkot]

This also produces an iterator, like Iterator.range.

But yes, I agree this is somewhat strange. We decided to only accept real iterables because taking iterators invites a lot of complexity. For example, we decided we wanted to do eager validation, which means checking that all arguments are iterables up front (that would become "iterables or iterators"), but we don't want to open iterables until the result iterator actually reaches them so that we can avoid having to close them. So we'd have to store for each argument something like { isIterator: true, object, nextMethod } | { isIterator: false, object, symbolIteratorMethod }, and then when closing the result iterator we'd need to go through the list and close just the isIterator: true ones. That could be done, but it's a lot of complexity for relatively small benefit.

[zloirock]

I agree about complexity of this. But... is this really required? For example, Iterator#flatMap does not close all inner iterators, only active (and it's not possible since requires full walking over top level iterator).

[bakkot]

The general rule is that once someone hands you an iterator it is your responsibility to close it when you're done with it (unless it errors out or is exhausted). So iterators which haven't yet been returned by the flatMap mapper aren't its responsibility - flatMap just has the mapper function, the outer iterator, and the one active inner iterator, and it does close the two iterators it has.

[zloirock]

This is not always observed. Let's take the first method that comes to hand - Iterator.prototype.drop. If the limit is invalid, we just throw an error without iterator closing. And it's in many (I don't remember, maybe all?) cases where we pass an iterator but don't start work with it. Iterator.concat arguments list looks similar.

[bakkot]

The iterator helper methods probably should close their receiver case of invalid arguments, actually, though the prototype methods are kind of special. Probably not too late to fix that.

There's very few cases where we pass around an iterator - the keys method of the argument in Set.prototype.union and friends is the only one I can think of offhand, and in that case we do indeed close it when we stop early (e.g. in Set.prototype.isDisjointFrom step 5.c.ii.1.a). Everywhere else we work with iterables, and we're quite consistent about closing the iterator we get from an iterable.

[zloirock]

e.g. in Set.prototype.isDisjointFrom step 5.c.ii.1.a

It's not a similar case since here we already worked with this iterator (5.c.i).

[bakkot]

Whether we have advanced this iterator or not is not a relevant distinction.

[zloirock]

It does for current precedents. All I see is that if we start working with the iterator, we close it. If we didn't start, no.

[bakkot]

There's barely anything which works with iterators at all, and the few places which do make that distinction do so only by accident, not as a considered choice which we need to carry forward. I'll try to get those fixed next meeting, but it's not something we are bound by either way.