Post-merge issues of decoupling the interface

[vitaly-t]

This is to address issues after PR #202, as a major rework of the architecture.

@RebeccaStevens Let's address any and all issues of that merge here, rather than the PR itself.

[vitaly-t]

I really wish I merged that big change into a dev branch, shouldn't code when falling asleep 😄

I needed a solid version, with the #200 change, so here what I did today:

• Cleaned up the list of branches, by deleting all the obsolete crap and merged ones. I even deleted develop branch.
• Created new branch dev from my last commit for Operators "wait" and "waitRace" should simply forward in sync mode #200, just before merging the decoupling PR.
• Released v2.7.0 with all the latest stuff (prior decoupling) from dev branch.

So we now have main and dev with what should be swapped in them, but it will have to do for the time being, alas.

We still have that 2.7.0-beta.0 published, with all the merged decoupling, which I've been using to test out the changes, not successfully for now, that's why we have this issue, so let's address it here...

You mentioned that the split of sync and async will require the same for custom operators... I wonder how this will look. I've run into a library shortcoming with the injectors, where I have to use wait() unconditionally, so if a promise is returned, it is resolved. That's why I did #200 change, so it no longer throws when inside a synchronous pipeline. I was thinking that we should have operator mapWait, which would resolve the promise returned, then things would be simpler? But then again, what should it do if a promise returned while inside a synchronous pipeline? Should it throw an error or just return Promise as a value? If it is the latter, then we end up where we are now, as spread in the same injector example will throw just like it does now. And if the former - then mapWait will throw first. Kind of catch 22.

[RebeccaStevens]

I've updated dev to have the new changes.
If you run this, it should get main back to where you want it.

git checkout main
git reset --hard 2.7.0
git push --force

[vitaly-t]

Just tried it, git reset --hard 2.7.0 produces:

fatal: ambiguous argument '2.7.0': unknown revision or path not in the working tree.

[RebeccaStevens]

Does 2.7.0 show up when you run git tag --list?

[vitaly-t]

It does not, 2.6.1 comes up as the latest, which is odd, because I can see 2.7.0 in the list of tags on the website...

[RebeccaStevens]

Try git fetch --all --tags

[vitaly-t]

That did it,...will release the direct commit hook now, and try again :)

[RebeccaStevens]

With regard to the documentation, there seems to be a bunch of "references" for types that are exported by multiple namespaces. It seems the async namespace is getting them as first-class entries while the index and sync namespaces just reference the async ones. I don't know if there's a way to make is so the index namespace is the one that gets them as first-class entries or not.

One simple solution could be just to only export those types in the index namespace rather than in all namespaces.

[vitaly-t]

Branch re-basement finished, looks good, cheers!

[RebeccaStevens]

This code should work in with 2.7.0-beta.0.

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

const input = [1, 2, 3];
const output = pipe(
  input,
  map((x) => x * 2)
);

console.log(output);

Note: If you are using TypeScript, you'll need to set moduleResolution to either Node16 or NodeNext to get the types to resolve properly.

[vitaly-t]

you'll need to set moduleResolution to either Node16 or NodeNext to get the types to resolve properly.

So how is it going to work inside web browsers?

2.7.0 works perfectly in my Angular projects, but 2.7.0-beta.0 cannot resolve the path...

And I cannot change the module target there, because changing it breaks everything else.

[RebeccaStevens]

In what regard? web browsers don't need type information.

[vitaly-t]

Angular, for example sets "moduleResolution": "node", If I change it, resolution becomes broken for all Angular modules.

[vitaly-t]

Also, if the library requires tweaking tsconfig manually after installation, it'll likely drive developers away.

[RebeccaStevens]

So "moduleResolution": "Node16" is the newer way of doing things.

AngularJS support has officially ended as of January 2022 so AngularJS might not ever support "moduleResolution": "Node16".

In this case, you'd need to do things to other (older) way.

import { async } from "iter-ops";
const { pipe, map } = async;

const input = [1, 2, 3];
const output = pipe(
  input,
  map((x) => Promise.resolve(x * 2))
);

(async () => {
  for await (const o of output) {
    console.log(await o); // This extra await shouldn't be needed. Bug?
  }
})();

Edit: Just realized that Angular and AngularJS are different. 👅

angular/angular#46181

[vitaly-t]

I wasn't talking about the ancient AngularJS, but the modern Angular v15, which is a totally different thing these days :)

b.t.w. That's my primary tool for Web. I'm an Angular specialist ;)

[RebeccaStevens]

So that code example in my previous comment should work for you for now until that linked issue is resolved.

[vitaly-t]

I just redid my tests against the very latest Angular, and it behaves differently again...

Now setting "moduleResolution": "NodeNext", does nothing.

And for some reasons it wants to find things inside iter-ops/dist/async, as shown below, from which pipe types are casted incorrectly... And this is regardless of how moduleResolution is set, it seems not to care anymore. Weird.

[RebeccaStevens]

Here's the way it should be able to be imported:

Option 0 - No changes:

"moduleResolution": "Node"

import { pipe, delay, tap } from "iter-ops";

Option 1 - Classic resolution to access the new namespaces:

"moduleResolution": "Node"

import { async } from "iter-ops";
const { pipe, delay, tap } =  async;

Option 2 - Modern resolution to access the new namespaces:

"moduleResolution": "Node16"

import { pipe, delay, tap } from "iter-ops/async";

Option 3 - Access the internals to access the new namespaces:

"moduleResolution": "Node"

import { pipe, delay, tap } from "iter-ops/dist/async";

[vitaly-t]

I'm not sure how we are going to proceed here, I'm just gonna leave it in your hands, for now. Version 2.7.0 installs and works without any issues everywhere, and we should have the same expectation for this branch, which is currently not the case, as far as I can see it...

For the time being, I might just focus on something simpler... like picking some operator from custom operators, and adding it to the library. Operators like appendIf, prependIf + replaceIf are probably the most valuable there on the list.

P.S. We clicked submit almost at the same time 😄 it's not an answer to your last post)

[vitaly-t]

Per your comments earlier + mine after, is there an option that would make it work reliably out of the box in both NodeJS (v12+) and Web without any post-install changes? That really would be the priority.

[RebeccaStevens]

You should be able to use the library exactly the same way as you could before.

import { pipe, delay, tap } from "iter-ops";

The new changes are an optional alternative way to do things.

[RebeccaStevens]

I just updated the tests and fixed up a few bugs that were revealed when doing so.
Do you want to release a new beta? 3.0.0-beta.0

[vitaly-t]

What are the breaking changes? Trying to figure out if 2.8.0-beta.0 would be more appropriate...

[RebeccaStevens]

They are very minor ones. I might even be able to remove them and make it so there aren't any.

The type UnknownIterableOrIterator no longer exists.

Other than that, there may not actually be any. The other major changes I made don't actually seem to be publicly visible. 2.8.0-beta.0 might be the better choice.

[vitaly-t]

2.8.0-beta.0 published from the latest dev branch ;)

[vitaly-t]

I took some time going over the dev branch, and the custom operators PR, plus tried to play with it locally...

I honestly do see this as an improvement. It might offer better type casting (I'm guessing here), but it replaces the plain operator syntax with sync.operator and async.operator syntax, and splits a single-file implementation into 3-4 files, with essentially repeated implementation. All that, combined with such a radical change in terms of the structure... I'll be honest, I'm not warming up to this idea.

I like the current architecture, it is way simpler, the way I see it. And I most definitely wouldn't want to complicate the idea of simple custom operators, with one where you have to create 4 files for it... that's just gonna be a turn-off for most developers, IMO.

I really do not want to seem ungrateful for how much code you have written for it, it is an incredible job, but I'm not sure I want this change. I wish we discussed it first, or you just did a POC instead... Really sorry!

[RebeccaStevens]

I've made some changes to the dev branch to restructure things and simplify it a bit.

I think I might have also been able to solve the resolution issue so that "moduleResolution": "Node16" is no longer needed (untested - but I think it should work).

There are some type issues that have arisen from the rebase, it should be an easy fix.

Let me know what you think of this change.

[vitaly-t]

I have postponed this for now, and released v3.0.0 without it, for there were too many changes since v2.0.0 as it is.

Also, we need a solid milestone here before considering such a big change as decoupling the interface.

[RebeccaStevens]

I believe I've now fixed up all the issues.
Please re-review when you have time.

[vitaly-t]

Published 4.0.0-beta.0 from the dev branch, to make evaluation easier.

In the meantime, check out TC39 update.

@RebeccaStevens