Skip to content
This repository has been archived by the owner on Oct 8, 2024. It is now read-only.

should flatMap flatten iterables or iterators? #229

Closed
michaelficarra opened this issue Sep 7, 2022 · 21 comments · Fixed by #233
Closed

should flatMap flatten iterables or iterators? #229

michaelficarra opened this issue Sep 7, 2022 · 21 comments · Fixed by #233

Comments

@michaelficarra
Copy link
Member

It currently flattens iterables. If X.prototype.flatMap flattens Xs, we should instead be flattening iterators.

As mentioned by @rbuckton in #117 (comment).

@zloirock
Copy link
Contributor

zloirock commented Sep 7, 2022

Flattening iterables is much more convenient than iterators.

@devsnek
Copy link
Member

devsnek commented Sep 7, 2022

getting some serious deja vu from this question 😄. I guess I would ask: what are some uses we can come up with for this flatMap method and what values are they dealing with?

@ljharb
Copy link
Member

ljharb commented Sep 8, 2022

How does it currently flatten iterables? The spec steps seem to assume an iterator.

@bakkot
Copy link
Collaborator

bakkot commented Sep 8, 2022

flatMap step 3.a.vi is

vi. Let innerIterator be Completion(GetIterator(mapped, sync)).

and GetIterator consumes iterables.

@michaelficarra
Copy link
Member Author

same for async flatMap step 3.a.viii

@ljharb
Copy link
Member

ljharb commented Sep 8, 2022

gotcha - but if we removed the GetIterator call, then you wouldn't be able to return an array, right? that seems problematic - although you WOULD be able to return a string, which seems beneficial.

@bakkot
Copy link
Collaborator

bakkot commented Sep 8, 2022

gotcha - but if we removed the GetIterator call, then you wouldn't be able to return an array, right?

Right. You'd have to return [0, 1, 2].values().

@ljharb
Copy link
Member

ljharb commented Sep 8, 2022

That seems like a pretty large ergonomics hit.

Now that I'm thinking about strings, though - currently, would i have to wrap it in [] to be able to flatMap over it and yield a full string? having to do [string].values() seems even worse than just array-wrapping.

@bakkot
Copy link
Collaborator

bakkot commented Sep 8, 2022

currently, would i have to wrap it in [] to be able to flatMap over it and yield a full string?

Yes.

This is arguably a reason to prefer only flattening iterators - if we flatten iterables, return "foo" will get spread; if we require iterators, then you have to actually think if you want "foo"[Symbol.iterator]() or [foo].values().

@ljharb
Copy link
Member

ljharb commented Sep 8, 2022

for strings, i agree with you - but for every other value, it's a huge tax :-/

@bergus
Copy link

bergus commented Sep 8, 2022

It currently flattens iterables. If X.prototype.flatMap flattens Xs, we should instead be flattening iterators.

As mentioned by @rbuckton in #117 (comment).

This has already been discussed about 3 years ago: #47 (comment), and the string problem/feature in #55.
So I concur with @devsnek: Déjà vu :-)

@rbuckton
Copy link

rbuckton commented Sep 9, 2022

Isn't string the only non-object iterable (if you exclude future tuples)? Would it make sense to have flatMap only flatten object iterables? You could still return a [..."foo"] if you intentionally wanted to spread a string, but I'd imagine spreading a string isn't generally what a user would intend.

Especially since each element yielded from an iterable string is, itself, also an iterable. If we were to add a .flat(), we'd likely have to special case strings anyways to avoid infinite recursion.

@rbuckton
Copy link

rbuckton commented Sep 9, 2022

Also, I certainly wouldn't want to exclude iterables here. I just wonder if the behavior of flatMap shouldn't be like Iterator.from(), where you could return an iterable or a user-defined iterator that simply has a next.

@rbuckton
Copy link

rbuckton commented Sep 9, 2022

Isn't string the only non-object iterable (if you exclude future tuples)? Would it make sense to have flatMap only flatten object iterables?

I just saw #55 (comment), but I'm not sure I agree. Strings are special, and flattening them is more than likely unintentional.

@michaelficarra
Copy link
Member Author

michaelficarra commented Sep 9, 2022

This has already been discussed about 3 years ago

Thanks for the pointer @bergus. I had forgotten about that thread. A significant difference between then and now is that (after #55) we are now considering a flatMap that does not lift a non-flattenable value into a singleton flattenable structure like what Array.prototype.flatMap does. I am happy with that decision.

I just wonder if the behavior of flatMap shouldn't be like Iterator.from(), where you could return an iterable or a user-defined iterator that simply has a next.

I'm leaning toward this solution. What would be the downside of this?

  1. If it's iterable, get an iterator and iterate that
  2. If it's an object and "next" is callable, iterate the object
  3. throw

@ljharb
Copy link
Member

ljharb commented Sep 9, 2022

The downside is that you shouldn't have to wrap a value in an iterator just to be able to return it from the mapper.

@michaelficarra
Copy link
Member Author

@ljharb you currently have to wrap it in an iterable, so I don't see how that is different. If you want to fight that fight, have at it in #55, but I strongly support that decision.

@ljharb
Copy link
Member

ljharb commented Sep 9, 2022

Fair enough, although I don't think that decision has been brought to plenary yet, and it's a pretty big one.

@bergus
Copy link

bergus commented Sep 9, 2022

I'm leaning toward this solution:

  1. If it's iterable, get an iterator and iterate that
  2. If it's an object and "next" is callable, iterate the object
  3. throw

I see 4 variants there:

  • a) test for Symbol.iterator, then test for next, else throw
  • b) test for next, then test for Symbol.iterator, else throw
  • c) test only for Symbol.iterator, else throw
  • d) test only for next, else throw

I think the majority agrees that d) has bad developer experience (having to wrap all iterables in Iterator.from(…) or calling [Symbol.iterator]()) and is in general not what the language does in other places where it iterates anything.
Between the other three, I don't see that much of a difference, given that all iterators which inherit from Iterator.prototype are iterable already. This holds true even for polyfilled iterators and most handcrafted iterators as well. For those that don't inherit from Iterator.prototype, if we did choose variant c), the rule would be simple: if you cannot invoke .flatMap() on it, you also cannot return it from the flatMap callback.
I would still favor c) - which is also the current proposal - but if anyone feels strongly about non-iterable iterators, we could indeed adopt a). I would avoid variant b) though since it's slower for the common case of non-iterator iterables.

@michaelficarra
Copy link
Member Author

@bergus I've opened #233 which takes variant a. Please review.

@devsnek
Copy link
Member

devsnek commented Sep 10, 2022

I also think we should keep the currently specified semantics.

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

Successfully merging a pull request may close this issue.

7 participants