toIterable() fails with completed iterators

[mattbishop]

I have a case where I need to wrap a possibly-completed iterator as an iterator to pipe() through some other things. I know the result will be an empty iterator, but I don't have a way to test for this before building the pipe() chain.

In debugging, I found that toIterable() calls a typeguard function isIteratorResult:

iter-ops/src/typeguards.ts

Line 91 in 35bc52e

export function isIteratorResult<T, CastGeneric = unknown>(

and fails to see the next() result as an iterator result. It wraps it as a SingleIterator, which breaks my app because it isn't expecting an iterator result.

isIteratorResult tests for value, but a completed iterator does not return a value. It does return done in all cases:

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Iterators_and_Generators#iterators

The fix is to look for a done property instead of a value property.

[vitaly-t]

It may be argued that isIteratorResult should return return has(value, 'value') || has(value, 'done'), however the bug is triggered by has implementation, which returns false for value: undefined, which is definitely incorrect.

The proper syntax goes like this: value must be always present, and done is optional. However, all JavaScript implementations feature both at all times, i.e. the last element is always {value: undefined, done: true}.

But let's look at how IteratorResult is defined within ES2015 that we are using:

interface IteratorYieldResult<TYield> {
    done?: false;
    value: TYield;
}

interface IteratorReturnResult<TReturn> {
    done: true;
    value: TReturn;
}

type IteratorResult<T, TReturn = any> = IteratorYieldResult<T> | IteratorReturnResult<TReturn>;

So we can see that done is optional while inside generators, which is odd, but ok. That's why I think that fixing has implementation is more appropriate.

@RebeccaStevens What do you think? If you agree, please make the change 😉

[vitaly-t]

@RebeccaStevens b.t.w., you have this comment inside has - // Cannot use in operator as it doesn't work for strings.

Perhaps hasOwnProperty is usable for this? ;)

The difference from in is as follows...

Unlike the hasOwnProperty() method, the in operator will return true for both direct and inherited properties that exist in the object or its prototype chain.

But our {value, done} is always a simple object, it is never a class ;) So the question is - are we ever use it for classes? Anyway, I'm leaving it up to you.

[RebeccaStevens]

Object.prototype.hasOwnProperty (or Object.hasOwn) is usually my go to for this, but it doesn't work in this case.
In general, we need to know if the object has the key, regardless of if it's direct or inherited. Not for the case of isIteratorResult, but for the other functions such as isSyncIterable and isPromiseLike.
Additionally, that function doesn't work for symbols (Object.getOwnPropertySymbols would need to be used for symbols, but again, that's for direct symbols).

I deicide it is best to go back to using the in operator. If we convert the object to an Object first, then it should work as desired.

[vitaly-t]

v2.8.0-beta.2 now contains the fix.

I'm gonna play a little with the new concurrencyFork operator within iter-ops-extras, to make sure it is as every bit as good as I was expecting, and then will make a proper 2.8.0 release 😉

@RebeccaStevens Never got your view on the new concurrencyFork addition (in the discussions). I think it is a game changer. What do you reckon?

[RebeccaStevens]

It looks good. I'll have a more in-depth look at it a bit later.

[vitaly-t]

The fix for this was released in v2.8.0.

@RebeccaStevens I tested it out (concurrencyFork) thoroughly, and it was good, so I have released it ;)

[mattbishop]

Verified fix in 2.8.0. Thanks!