Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Iterator.concat instead of variadic Iterator.from #1

Open
bakkot opened this issue Jan 4, 2024 · 15 comments
Open

Iterator.concat instead of variadic Iterator.from #1

bakkot opened this issue Jan 4, 2024 · 15 comments

Comments

@bakkot
Copy link
Collaborator

bakkot commented Jan 4, 2024

Variadic Iterator.from is weird in a couple of ways:

  • it's not just a generalization of Iterator.from. Iterator.from(null) throws immediately. Iterator.from(null, it) will return an iterator which throws once used.
  • it's very different from Array.from(x, y)

So I would prefer not to do this. I'd prefer instead having a static (or prototype) concat method, which works the same way as the variadic case of from currently in this proposal.

@graphemecluster
Copy link

Taking consistency into consideration, Iterator.from(x, y) should instead behave like Iterator.from(x).map(y) (I can’t find anyone talking about it even with a thorough search – am I missing something?)

@michaelficarra
Copy link
Member

@graphemecluster The second parameter of Array.from is there to avoid having to iterate the whole array after iterating the first parameter. There's no such downside with Iterator.from(a).map(b), so matching the signature of Array.from would be pointless.

@zloirock
Copy link

zloirock commented Feb 4, 2024

It's an inconsistency that will make JS harder to learn and memorize.

There's no such downside with Iterator.from(a).map(b)

If a does not have %IteratorPrototype% in the prototype chain (userland iterator), it's 2 wrappers instead of 1.

@ljharb
Copy link
Member

ljharb commented Feb 4, 2024

Iterator.from always produced a wrapped iterator, so the return will always have .map on it.

@zloirock
Copy link

zloirock commented Feb 4, 2024

Iterator.from always produced a wrapped iterator

Nope.

image

@ljharb
Copy link
Member

ljharb commented Feb 4, 2024

Thanks, I'll rephrase: Iterator.from always produces an iterator that has .map on it (in a world where iterator helpers are shipped) - or at least, that was the entire point of it.

@zloirock
Copy link

zloirock commented Feb 4, 2024

I'll rephrase

I have no idea how it's related to this case.


In

Iterator.from({
  next: () => ({ done: Math.random() > .9, value: Math.random() })
}).map(it => it * 10 | 0).toArray();

Iterator.from produces an intermediate iterator (with WrapForValidIteratorPrototype), .map produces a second wrapper (with IteratorHelperPrototype). Like with an intermediate array in Array.from().map(), a little cheaper - but anyway it's overhead with extra proxied .next calling. A mapper as the second Iterator.from argument can reduce this overhead (a minor advantage) and make it a little more consistent with Array.from (a more significant advantage).

It's not something I can't live without - it's just an example that consistency is better.

@bakkot
Copy link
Collaborator Author

bakkot commented Feb 6, 2024

@ljharb said he would want any method named concat to share Array.prototype.concat's behavior of allowing both containers and items which are not containers, with the containers being spread and the non-containers used as-is.

@michaelficarra said he wasn't interested in that behavior. Combined, that rules out the name concat.

So, instead, how about a static variadic Iterator.append? I think that would be quite clear: Iterator.append(foos, bars).

@ljharb
Copy link
Member

ljharb commented Feb 6, 2024

Seems perfectly reasonable to me!

@bergus
Copy link

bergus commented Feb 7, 2024

@ljharb said he would want any method named concat to share Array.prototype.concat's behavior of allowing both containers and items which are not containers

Why is that? I believe more people are confused by this behaviour than pleased by it, and would gladly drop it (if it wasn't for backwards compatibility). Having to check for isConcatSpreadable every time can't be efficient either, can it?
Also String.prototype.concat already doesn't follow this rule, unless you treat strings as containers of characters and distinguish them from single-character strings. And I don't see why this convention would need to hold for static methods?

how about a static variadic Iterator.append?

Hm, I think that would confuse Pythonistas where list.append(…) is the single-element-push method. In my ears, append is at best a binary instance method, like foos.append(bars), not a static variadic function. Maybe that's my Haskell upbringing, where concat :: [[a]] -> [a] and (m)append :: [a] -> [a] -> [a].

I'd definitely prefer Iterator.concat(...iterables). If that's out of question already, maybe Iterator.concatenate(...iterables)?

@ljharb
Copy link
Member

ljharb commented Feb 7, 2024

Not because of isConcatSpreadable, but because it’s very very idiomatic to do [].concat(maybeArray || []).

Nobody ever uses string concat (other than Babel’s template literal transpile).

Anything that’s similar to concatenation implies the same behavior as array concat; “concatenation” is this the same as “concat” in this regard,

@ljharb
Copy link
Member

ljharb commented Jul 3, 2024

(i note the readme still says the chosen solution is called "concat"; my constraint remains that if it's called "concat" it must accept both iterables and non-iterables; alternatively a new name can be chosen)

@michaelficarra
Copy link
Member

Yeah at the June meeting I presented some names we may move forward with if concat can't work.

Iterator Sequencing for Stage 2

@ljharb
Copy link
Member

ljharb commented Jul 4, 2024

I'd vote to avoid concatenate or cat, due to possible confusion (cat is a shell command).

@bakkot
Copy link
Collaborator Author

bakkot commented Jul 4, 2024

My vote is still for concat, or failing that for append.

I am somewhat reluctant to give up on the obvious name based only on a single person's dislike of that name. I see the inconsistency you're pointing out but since it's already different by virtue of being static rather than prototype, and since you'll immediately get an error if you try to use it assuming it has the same behavior as [].concat, I don't think that inconsistency is enough reason to use a different name.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants