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

Questions regarding "auto-flatten" an non-iterables #275

Open
benlesh opened this issue Apr 10, 2023 · 13 comments
Open

Questions regarding "auto-flatten" an non-iterables #275

benlesh opened this issue Apr 10, 2023 · 13 comments

Comments

@benlesh
Copy link

benlesh commented Apr 10, 2023

Terribly sorry if this is clearly documented and I missed it

With Array#flatMap, it will automatically flatten/merge in values that are not arrays into the array output:

[1, 2, 3].flatMap(() => 42); // [42, 42, 42]

However, it will NOT auto convert any old iterable (I know this is a separate thing, just calling out the behavior):

[1, 2, 3].flatMap(() => new Set([42])) // [Set, Set, Set]

I'm using this information to help guide the direction of RxJS observables and our operators/methods long-term.

Questions

  1. What will happen in this scenario for any Iterable or AsyncIterable?
  2. What is the plan for this sort of support in any old Iterable or AsyncIterable? Will they flatten anything that implements Symbol.iterator or Symbol.asyncIterator respectively?
  3. Will this constitute a breaking change for Array#flatMap? Or is its implementation an Array-specific one-off from the iterator helpers?

Thank you in advance.

@ljharb
Copy link
Member

ljharb commented Apr 10, 2023

Iterables are ignored by design in array flatMap (because strings are iterable), only IsArray matters. However, in iterator helpers flatMap, primitive iterables throw, so any returned Object iterable or iterator is flattened.

In async helpers, I'd expect that also, an async iterable or iterator is flattened.

See #250, #229.

@bakkot
Copy link
Collaborator

bakkot commented Apr 10, 2023

Sorry, I misread your comment initially. Let me try again:

First, there's no changes to Array.prototype.flatMap in this proposal.

Second, there's a summary of the behavior at #233, which is correct except for the caveat that Iterator.from behaves differently from what's laid out there in that it gets the iterator from strings instead of throwing. So here's concretely what that means for your questions:

What will happen in this scenario for any Iterable or AsyncIterable?

For Iterator.prototype.flatMap, non-string Iterables will be flattened, AsyncIterables which are not iterable (or iterators) will throw, as will anything which is neither iterator nor iterable. Strings also throw.

This is inconsistent with Array, but has the advantage that it makes it less likely that adding Symbol.iterator to an existing class will be a breaking change.

What is the plan for this sort of support in any old Iterable or AsyncIterable? Will they flatten anything that implements Symbol.iterator or Symbol.asyncIterator respectively?

Not sure what this is asking that is different from the previous question so I don't know how to answer it.

Will this constitute a breaking change for Array#flatMap? Or is its implementation an Array-specific one-off from the iterator helpers?

Its implementation is an Array-specific one-off. The general principle we decided on at the time was "X.prototype.flatMap should only flatten Xs". We then immediately made an exception to allow iterator flatMap to also flatten iterables, but that's still the idea.

@benlesh
Copy link
Author

benlesh commented Apr 10, 2023

@bakkot What about promises in an AsyncIterable helper?

someAsyncIterable.flatMap(async* () => {
  yield 'hi'
})

// vs

someAsyncIterable.flatMap(async () => {
  return 'hi'
})

// vs

someAsyncIterable.flatMap(() => 'hi')

// vs

someAsyncIterable.flatMap(() => Promise.resolve('hi'))

Context: I'm asking all of these things because RxJS deals with a LOT of similar issues. I'm currently discussing what changes we might need to migrate towards to come to some sort of alignment here

In RxJS, for a very long time we've supported "auto converting" anything we can convert to an Observable. Including strings, but my hot take for you there is that usually when someone returns a string and it auto converts because it's Iterable, it's an error, and I think we should get away from it.

For example, in RxJS, all of these "just work":

someObservable.pipe(concatMap(() => [1, 2, 3])); // Observable<1 | 2 | 3>
someObservable.pipe(concatMap(async () => 42)); // Observable<42>
someObservable.pipe(concatMap(() => Promise.resolve(42))); // Observable<42>
someObservable.pipe(concatMap(function* () {
  yield 1;
  yield 2;
  yield 3;
}); // Observable<1 | 2 | 3>
someObservable.pipe(concatMap(async function* () {
  yield 1;
  yield 2;
  yield 3;
}); // Observable<1 | 2 | 3>

By and large, it's been a benefit to the community. In particular the handling of promises. However, the string thing still gets folks from time to time, so I think you made the right choice there.

@benlesh
Copy link
Author

benlesh commented Apr 10, 2023

primitive iterables throw

@ljharb I presume that string is a "primitive iterable". Are there others in other runtimes? Or is this just a generic direction in case others are added at some point?

@ljharb
Copy link
Member

ljharb commented Apr 10, 2023

No others, and if primitive Tuples were to be added, they'd likely be allowed instead of throwing, but it's more the consistency point underscoring that making strings iterable in the first place was a huge mistake.

@benlesh
Copy link
Author

benlesh commented Apr 10, 2023

primitive Tuples

That's an interesting case. I'm not sure what people would mean if they were like source$.pipe(concatMap(() => #[1, 2])) (or whatever the syntax is now). I feel like they'd want an observable of tuples... but that's unclear to me.

RxJS could benefit from this sort of spec-ing, an honestly it might serve the observable proposal a bit if we did this sort of thing. So I'm trying to learn here.

@benlesh
Copy link
Author

benlesh commented Apr 10, 2023

Still curious about this:

What about promises in an AsyncIterable helper?

And further, I'm not totally sure what this would do:

someAsyncIterable.flatMap(async function* () {
  yield* someOtherAsyncIterable;
})

// vs

someAsyncIterable.flatMap(async function* () {
  yield someOtherAsyncIterable;
})

I presume they'd return AsyncIterable<T> and AsyncIterable<AsyncIterable<T>> respectively?

@bakkot
Copy link
Collaborator

bakkot commented Apr 10, 2023

What about promises in an AsyncIterable helper?

AsyncIterator's flatMap behaves similarly to a for-await over the return value of the passed function, except that it also allows things which are sync or async iterators rather than only things which are sync or async iterables (and also it rejects strings).

(I should note that this only applies to flatMap - e.g., you can do asyncIterable.map(async x => 0) and get 0, 0, 0.)

In particular, just like for await (let x of Promise.resolve(0)) will throw, you can't return a bare Promise from the callback and have that work. But you can return an Array. You can even return an Array of promises, and those will be awaited.

Concretely:

someAsyncIterable.flatMap(async* () => {
  yield 'hi'
})
// ["hi", "hi", ...]

someAsyncIterable.flatMap(async () => {
  return 'hi'
})
// throws, since the result is a Promise, which is not iterable or an iterator in either the sync or async sense

someAsyncIterable.flatMap(() => 'hi')
// throws, since the result is a string

someAsyncIterable.flatMap(() => Promise.resolve('hi'))
// throws, since the result is a Promise, which is not iterable or an iterator in either the sync or async sense

But, this does work, since the result is iterable:

someAsyncIterable.flatMap(() => ['hi'])
// ["hi", "hi", ...]

So does this, and it will unwrap the promise just like for await (let item of [Promise.resolve(0)]) console.log(item) would:

someAsyncIterable.flatMap(() => [Promise.resolve('hi')])
// ["hi", "hi", ...] - note that the promise is unwrapped

Including strings, but my hot take for you there is that usually when someone returns a string and it auto converts because it's Iterable, it's an error, and I think we should get away from it.

Yeah. Incidentally Tab has a good essay about it being a mistake to have made String iterable in the first place.

Though note that Iterator.from will still let you pass a string, on the assumption that this was an explicit conversion rather than being implicit. It's only flatMap specifically which rejects it.

And further, I'm not totally sure what this would do:

someAsyncIterable.flatMap(async function* () {
  yield* someOtherAsyncIterable;
})

someAsyncIterable.flatMap(async function* () {
  yield someOtherAsyncIterable;
})

I presume they'd return AsyncIterable<T> and AsyncIterable<AsyncIterable<T>> respectively?

Correct.

@benlesh
Copy link
Author

benlesh commented Apr 10, 2023

Great! Thank you for answering these questions. Very helpful.

@benlesh benlesh closed this as completed Apr 10, 2023
@bakkot
Copy link
Collaborator

bakkot commented Apr 10, 2023

@benlesh Oh, one thing I just noticed - your examples were writing someAsyncIterable.flatMap, but this proposal isn't adding iterable helpers, it's adding iterator helpers. There is, unfortunately, no generic "Iterable prototype" to put stuff on, so this stuff only applies to iterators specifically. (And to be more precise, only those iterators which inherit from the built-in Iterator prototype - which is all built-in iterators, including web-platform ones, and anything produced by a generator, but of course not not manually written ({ next() { ... } }) iterators, which will need to be converted with Iterator.from(...).)

That is, you're not going to be able to do (new Set).flatMap(). You have to do (new Set).values().flatMap (or Iterator.from(new Set) or other similar ways of getting an iterator).

That's also why this isn't a breaking change for Array: [].flatMap is Array.prototype.flatMap, and is the only flatMap anywhere in the prototype chain of an array. To get the version from this proposal, you'd have to do Iterator.from([]).flatMap etc.

@benlesh benlesh reopened this Apr 10, 2023
@benlesh
Copy link
Author

benlesh commented Apr 10, 2023

iterable helpers, it's adding iterator helpers. There is, unfortunately, no generic "Iterable prototype" to put stuff on, so this stuff only applies to iterators specifically.

That's interesting. I guess it makes sense.

Just to jump to my cases around this (which are admittedly off-topic for this repository)... Observable is the "dual" of Iterable, such that it's really an iterable turned inside out. So thinking about iterator having the methods is a bit mend bending.

The transformation from Iterable to Observable would be something like as follows (assuming a more "classic" non-JS iterable):

interface Iterator<T> {
  next(): T;
  done(): boolean
}

interface Iterable<T> {
  iterate(): Iterator<T>
}


//////// "DUAL" ///////////////
// Just pass the value instead of returning it, really.

interface Observer<T> {
  next(value: T): void;
  done(isDone: boolean): void;
}

interface Observable<T> {
  observe(observer: Observer<T>): void;
}

Plus you need some sort of cancellation semantic to remove the observer from that observe call, which could be anything... an unobserve function, a returned teardown value, or a passed cancellation token of some sort. (In RxJS observe is just subscribe, and the cancellation is a returned Subscription object).

It makes me wonder about what it would look like to put the methods/operators on Observer instead. Probably not good, I guess.


Regardless, it brings me to another question about this:

Considering an Iterator is stateful... what does one get in this scenario:

function* generateNumbers() {
  for (let n = 0; n < 10; n++) {
    yield n;
  }
}

const gen = generateNumbers()

// Iterate two values off of the iterator
gen.next();
gen.next();

// NOW map it.
const whatWillHappen = gen.map(n => n + n)

for (const value of whatWillHappen) {
  console.log(value); // What does this log??
}

I'd presume that it would log 4,6,8,10,12,14,16,18,20?

It's an interesting foot-gun. Although I suspect that most people will be a victim of iterating once, then mapping and trying to iterate it again, which is a known foot-gun with iterators regardless.

@bakkot
Copy link
Collaborator

bakkot commented Apr 10, 2023

Observable is the "dual" of Iterable, such that it's really an iterable turned inside out. So thinking about iterator having the methods is a bit mend bending.

Yeah. Though I don't know that the "dual" idea entirely holds in the way which is relevant here - Iterable and Iterator are both conceptually sources, whereas Observable is conceptually a source and Observer is conceptually a sink. And these methods make more sense on source than sinks (though you could put them on sinks, in principle, and have them apply before the value reaches the sink).

On the one hand, I do think it is kind of unfortunate that it's not practical for us to put methods on an abstract Iterable class - but on the other hand, Iterable is in practice more interface-like than Observable is, and having a concrete class (rather than an interface) to have the methods is nicer in some ways also. That is, specifically, calling filter on an Iterator unambiguous just produces an Iterator, whereas for example it would be weird for (new Set).filter to produce a generic Iterable rather than an actual Set. That's not as relevant to Observable, since I think in general if you call .filter on an Observable you're expecting to get a generic Observable out.

I'd presume that it would log 4,6,8,10,12,14,16,18,20?

Yup. It'll be really important to internalize that iterators are single-use, and that calling one of the helper methods constitutes a "use" just as much as iterating it would.

@benlesh
Copy link
Author

benlesh commented Apr 11, 2023

Oh... To be clear, there are ways you can create all of the same APIs over Observer. And they're single use too, it's just that they're often stateless. Duality still applies.

// In this theoretical API:

const observer = new Observer()
  .map(x => x + x)
  .filter(x => x >= 10)
  .reduce((acc, x) => acc + x, 0)
  .handle(console.log)
  
// someObservable emits 1,2,3,4,5,6,7,8
// console logs 52

someObservable.observe(observer)

Weirdly, at this point, in RxJS they would be Subjects.

import { Subject } from 'rxjs';

const subject = new Subject<number>().pipe(
  map(x => x + x),
  filter(x => x >= 10),
  reduce((acc, x) => acc + x, 0)
)

subject.subscribe(console.log)

// someObservable emits 1,2,3,4,5,6,7,8
// console logs 52

someObservable.observe(subject)

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

3 participants