Type issues

[RebeccaStevens]

I've looked into the type issues we are having, and it seems like we may be hitting a TypeScript bug. I've made an issue here.

I'll see if I can come up with a work-around.

[RebeccaStevens]

Looks like we'll have to avoid creating any intersections of functions. There are also problems with using overloads as they work in the same way.

[RebeccaStevens]

This definition seems to resolve most type issues:

type Operation<T, R> = (
    i: AsyncIterable<T> | Iterable<T>
) => AsyncIterable<R> & Iterable<R>

However, it isn't "correct" and thus if we go with it, we'd want to only use it internally (not export it). I'll see if I can come up with a better solution.

[RebeccaStevens]

I think the only way to properly get around this issue is to split all operations into explicit sync and async version. However, this will not be as elegant, so I understand not wanting to take this approach.

Originally, I was hoping that splitting the pipe function into a sync and async version would be enough, but unfortunately not. I would still however recommend keeping the split pipelines as it means users don't have to explicitly convert a non-async iterate before using the async pipeline (the pipeSync function can potentially be dropped).

[RebeccaStevens]

@vitaly-t I've done a bunch more looking into this and things aren't looking great 😕

I've spent the last 8 hours or so converting all the function overloads to a single function definition that uses conditional types in hope that this would get around the TypeScript limitation/bug. This involved having to change the definition of AsyncOperation and SyncOperation as well as completely removing the main Operation

// New
type SyncOperation<
    T extends Iterable<unknown>,
    R extends Iterable<unknown>
> = (i: T) => R;

// Old
type SyncOperation<T, R> = (i: Iterable<T>) => Iterable<R>;

Although this did result in improvements with some operators, others had no improvements. I think the only way we can actually get the types working right is to split the operations.

Maybe we could do something like this:

import {pipe, filter, map} from 'iter-ops/sync';

const i = pipe(
    [1, 2, 3, 4, 5],
    filter((a) => a % 2 === 0),
    map((value) => ({value}))
);

import {pipe, distinct, delay} from 'iter-ops/async';

const i = pipe(
    [1, 2, 2, 3, 3, 4],
    distinct(),
    delay(1000)
);

This approach will allow for the sync and async operations to keep the same names and should allow for all the type issues to be solved.

I imagine that usually users will only want to use one of the pipelines at a time so this should be a non-issue. If users want to use both pipelines, they could possibly do something like this.

import {sync, async} from 'iter-ops';

const i = sync.pipe(
    [1, 2, 3, 4, 5],
    sync.filter((a) => a % 2 === 0),
    sync.map((value) => ({value}))
);

const j = async.pipe(
    [1, 2, 2, 3, 3, 4],
    async.distinct(),
    async.delay(1000)
);

Out of all the option we have, I think splitting the operations is the best solution. With every other solution I've come up with, it doesn't seem to be possible to get the types right.

I wish TypeScript would solve that issue, but as it seems to be a design limitation, the only way it will ever be solved is if TypeScript itself undergoes a major rewrite, which is unlikely to happen any time soon.

[RebeccaStevens]

Also, splitting the operations will allow for the sync and async versions to be tree-shaken from each other, thus this would be a performance optimization.

[vitaly-t]

Please remind me, what was the initial issue of changing so much code that's in the current v1.6.4 release? Did the types not work there correctly?

I've made an issue here

But that's a duplicate, for which a fix was released awhile ago?

P.S. I kind of really wish now that all this was a development branch instead... I might in fact force-split it now, because the state in which the master now isn't great. I cannot see at this point when the next update will be possible to publish.

[RebeccaStevens]

Please remind me, what was the initial issue of changing so much code that's in the current v1.6.4 release? Did the types not work there correctly?

The types "work" there but not in all cases. These cases are what I'm trying to fix. Fixing them should also allow for all of the anys and casts to be removed from the code base. Currently they can't be as TypeScript will then detect the cases where they don't work and complain.

I plan on adding a bunch more type test to test the types. The current type tests are just "happy path" tests.

But that's a duplicate, for which a fix was released awhile ago?

It's a duplicate of https://github.com/microsoft/TypeScript/issues/26591 but there is no fix. That issue was labeled with "Design Limitation" which means "No apparent way of fixing this exists (short of wholescale redesign)". So TypeScript wants to fix the issue but they can't.

P.S. I kind of really wish now that all this was a development branch instead... I might in fact force-split it now, because the state in which the master now isn't great. I cannot see at this point when the next update will be possible to publish.

I've reverted a bunch of the changes that were making the issues more apparent. This should make things look cleaner now like they did pre the 2.0 betas (though the underlying issues still remain).

[vitaly-t]

I tried everything in the updated master branch, including deleting all temporary folders and reinstalling, but tests still won't run anymore:

There's some issue with the rollup library. I'm investigating...

b.t.w. I'm on Windows 10

[vitaly-t]

npm run doc in master also doesn't work anymore:

[vitaly-t]

After looking a little closer into issues with running tests....

Rather than running the combined npm test, I tried separate npm run test:js and npm run test:types, and it is only the latter that fails for me, while the former is running fine.

@RebeccaStevens

[RebeccaStevens]

I think I've been able to recreate the issue you were having. Updating the dependencies seems to have fixed the issue for me. Hopefully it will fix the issue for you too. On the main branch, try running yarn to install the dependence updates and the then yarn test. Hopefully it will all work this time.

Edit: Looks like there was another issue related to how windows parses command line arguments. I've just pushed a fix for that.

[vitaly-t]

Yep, all issues look resolved 😄

[vitaly-t]

And then a little later, the issue with documents came back again, no idea why, as I didn't change anything...

Any idea why this keeps happening?

[vitaly-t]

I researched it, and I was able to fix it by adding the following dependency:

"@types/estree": "1.0.0"

[vitaly-t]

@RebeccaStevens

What's your current plan for the project? If we cannot make progress for the types, then maybe we should just focus on what's at hand - revise types and code for flat + flatMap (both are in the master), and make another Beta at least? :wink:

[RebeccaStevens]

I'll prioritize working on those with the current type system.

What were the breaking changes btw?

[vitaly-t]

What were the breaking changes btw?

I didn't introduce any, there were only two new operators added.

You, however, mentioned breaking change through some type addition/change in #124. That was the whole reason I started 2.0.0 Beta versions. Please review those. And if you think we do not really have any breaking change, then there is no reason to up the version, and I can release 1.7.0 instead.

[RebeccaStevens]

Ahh, right. It's quite a simple change but yeah, it is breaking so we should do a major release.

[vitaly-t]

Great thanks! So let me add some docs here for the new operators + in general, and maybe you can look into #103 in the meantime, since you reported it? Then we can release 2.0.0, or at least I will do one more Beta first :wink:

Actually, I'd prefer that you also look into the types for the two new operators, they don't look quite finished to me :wink:

@RebeccaStevens

[RebeccaStevens]

Will do.

#151 should fix the types for flat

[vitaly-t]

As I first mentioned it in discussion, here's what I think it should be:

1. pipe should remain as is, taking a mixed type of iterable, and handling mixed type casting.
2. pipeSync should throw an error when passed an async input, but only when called directly from the client (not from pipe). It also should be able to show compile-time issue when async is passed into it.
3. pipeAsync should automatically convert to async when passed synchronous data

We would need to separate pipeSync and pipeAsync code so that implicit (from pipe) and explicit calls are separate.

[vitaly-t]

To address what's outstanding, I have opened this issue.

[vitaly-t]

@RebeccaStevens Are you still planning to do some work here?

I think we may still have some type of discrepancies here and there...

From the top of my head, operator flatMap has a declaration discrepancy whereby it fails to identify in its signature that the callback can return a promise, even though it does support that, see the test.

[RebeccaStevens]

I think for now we can mark this as done. The majority of the happy-paths work. To fix everything would probably require some fundamental restructuring of the library (if it's even possible).

[vitaly-t]

Do we really have that many problems? 🤦‍♂️ I thought you did full restructuring :smile:

What about that flatMap declaration? It doesn't look right to me, it's callback is supposed to allow promises, but it does not, while the implementation has it.

[RebeccaStevens]

The type issues should only show up when you go looking for them (such as sync operations working with async types (e.g. accepting promises)).

I'll look into that flatMap one.

[vitaly-t]

Great, I'll wait for this one before making another release :wink:

[vitaly-t]

Ok, I have published v2.3.0 with everything as is. This issue with flatMap can be addressed in the next update.