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

Should Observable.from have a mapFn argument? #75

Open
benlesh opened this issue Jan 16, 2016 · 23 comments
Open

Should Observable.from have a mapFn argument? #75

benlesh opened this issue Jan 16, 2016 · 23 comments

Comments

@benlesh
Copy link

benlesh commented Jan 16, 2016

Because...

  1. It would be symmetrical to Array.from
  2. It's more performant to map the values just before you emit them than it is to use a map operator, which incurs another subscription.

Arguably we would then want a thisArg argument as well, which is gross, because I hate those arguments personally, but symmetry is usually good.

@ljharb
Copy link
Member

ljharb commented Jan 16, 2016

+1 for symmetry with Array.from

@benjamingr
Copy link

How does .from act on other Observabes with the map argument?

@zenparsing
Copy link
Member

@benjamingr I guess the only thing that makes sense would be to essentially apply a map combinator to the input observable.

@benjamingr
Copy link

I personally really want to see this proposal progress and if this risks it -1. If it doesn't we might as well add .map

I think both alternatives should be presented to the TC and they can decide.

@benlesh
Copy link
Author

benlesh commented Jan 18, 2016

An addtional question would be: If from accepts iterables, should it also accept ArrayLike objects like Array.from does?

function makeArray() {
  return Array.from(arguments);
}
makeArray(1, 2, 3, 4, 5); // [1, 2, 3, 4, 5]
// or 
Array.from({ length: 5 }, (_, i) => i)  // [0, 1, 2, 3, 4];

Which seems to mean that Observable.from(arguments) or Observable.from({ length: 3 }, (_, i) => i) should work.

@benjamingr
Copy link

@Blesh the problem is that unlike Array.from you don't need (and we really shouldn't) enumerate the iterable all at once in memory.

I guess we could:

  • If it's an iterable, iterate it.
  • Otherwise - use the same conversion logic from Array.from only except saving the intermediates - fire onNext.

To illustrate:

Observable.from(arrayOfAMillionThings)

Doesn't really have to guard against the array being modified during iteration - and should not allocate an additional million item array.

@benlesh
Copy link
Author

benlesh commented Jan 18, 2016

@Blesh the problem is that unlike Array.from you don't need (and we really shouldn't) enumerate the iterable all at once in memory.

Sure, sure.... but that doesn't mean you couldn't accept an ArrayLike. I'm not advocating it nor am I disparaging it, but I'm wondering in the name of "symmetry". Really the only place it's useful is to create an Observable of arguments for the inner workings of some function. I'd argue that's probably (at least a little big of) an anti-pattern, but that doesn't mean we shouldn't allow it and provide symmetry with Array.from.

This issue is mostly just to get some thought around this. It's come up in RxJS.

@domenic
Copy link
Member

domenic commented Jan 18, 2016

I feel like with these kind of questions we are at a crossroads in terms of how much we want to ape Array. Should they just be "rhyming" methods that do roughly the same thing, or should they try to copy all of the array version's implementation quirks for maximum compatibility?

I think I'd argue toward rhyming. That is, they should accept the same arguments and values, but their behavior can be different---for example, being lazy. That aligns with my thinking on how Observable.prototype.map (or, for that matter, Iterable.prototype.map) will be lazy, unlike Array.prototype.map. We kind of discussed this at the last TC39 I believe.

@benjamingr
Copy link

Really the only place it's useful is to create an Observable of arguments for the inner workings of some function

arguments is iterable in ES2015, so that's not really an issue.

This issue is mostly just to get some thought around this. It's come up in RxJS.

Yeah, it's good you brought it up don't get me wrong :)

@benjamingr
Copy link

I think I'd argue toward rhyming. That is, they should accept the same arguments and values, but their behavior can be different---for example, being lazy.

Hi Dom! Few things:

  • Do you think that a mapFn argument would pose a risk to the proposal?
  • Is there any way to omit it and remain "future compatible"?

(For what it's worth, I don't really see how Observable.from can be lazy, it's more about the extra allocation that I think we should avoid)

@domenic
Copy link
Member

domenic commented Jan 18, 2016

I think every deviation from existing APIs actually proposes a risk. That is, if Observable has a method that Array does, their signatures need to match. Same argument as needing thisArg in a few places.

So IMO the lack of mapFn is just an oversight that really needs to be corrected.

I don't really see how Observable.from can be lazy

Maybe I am missing something but couldn't it just behave similarly to map, only grabbing a value from the source inside next()?

@benjamingr
Copy link

Maybe I am missing something but couldn't it just behave similarly to map, only grabbing a value from the source inside next()?

Well like Erik taught us - observables are eager, async iterators are lazy. Converting an iterator to an async iterator could be done lazily but as soon as you .subscribe to an observable it'd have to enumerate the entire iterable (no backpressure mechanisms or anything to prevent that).

Honestly I'm not sure I understand why we need Array.from to have a mapFn in the first place but that sailed a looong time ago. I guess since for the vast majority of use cases it wouldn't matter - Observable.from could just internally call Array.from's logic and since it would all be internal engines could optimize it away in practice.

@benlesh
Copy link
Author

benlesh commented Jan 18, 2016

So IMO the lack of mapFn is just an oversight that really needs to be corrected.

I agree with @domenic, I realized RxJS 5 was missing it and thought "oops, that means it's missing in es-observable as well, I'd bet".

@benlesh
Copy link
Author

benlesh commented Jan 18, 2016

Honestly I'm not sure I understand why we need Array.from to have a mapFn in the first place

@benjamingr if I had to speculate, I'd say it's because it's more efficient to transform them as you're creating the array, rather than creating an array of values, and then creating a second array of the transformed values. That pattern is all over RxJS too via "resultSelectors", but the idea there is to reduce subscriptions in the chain.

@domenic
Copy link
Member

domenic commented Jan 19, 2016

Well like Erik taught us - observables are eager, async iterators are lazy. Converting an iterator to an async iterator could be done lazily but as soon as you .subscribe to an observable it'd have to enumerate the entire iterable (no backpressure mechanisms or anything to prevent that).

Right, "lazy" might not be the right word. What I really meant is that it seems OK to iterate the input each time subscribe is called, instead of exhausting the iterable once into an array and then using that for all future subscriptions. Even though the latter is more like Array.from, the former seems more in line with the semantics for observables.

@zenparsing
Copy link
Member

I don't have a strong feeling here, other than to say:

  • mapFn isn't really in the spirit of Observables, whereas a "map" combinator is.
  • With Observables, mapFn doesn't have the same advantage that it has for Array.from. If I remember correctly, the main advantage of supplying a mapping function was to avoid the creation of an intermediate array. With Observables, there's no intermediate data structure, even when you use a "map" combinator.

@benlesh
Copy link
Author

benlesh commented Jan 19, 2016

With Observables, mapFn doesn't have the same advantage that it has for Array.from. If I remember correctly, the main advantage of supplying a mapping function was to avoid the creation of an intermediate array. With Observables, there's no intermediate data structure, even when you use a "map" combinator.

It has an advantage, but it's different. A map combinator will incur an additional subscription, which is the "expensive" bit, where a mapFn will be applied to the values prior to emission in the first subscription.

Observable.from(blah).map(toThings).subscribe() // 2 subscriptions
Observable.from(blah, toThings).subscribe() // 1 subscription

@benlesh
Copy link
Author

benlesh commented Jan 19, 2016

As for it being in the "spirit" of Observables, I can't say I guess, but avoiding subscriptions when necessary is totally of the spirit of Rx.

Case in point: flatMap.

It could be .map(x: any => Observable<T>).mergeAll(), but .flatMap(x: any => Observable<T> reduces the subscriptions to 1 instead of two.

Going even deeper in RxJS, there are resultSelectors on many operators like this, so:

3 subscriptions

source.map(x: any => Observable<T>).mergeAll().map(y: T => R).subscribe()

2 subscriptions

source.flatMap(x: any => Observable<T>).map(y: T => R).subscribe()

1 subscription

source.flatMap(x: any => Observable<T>, (x: any, y: T) => R).subscribe()

all of them are the same.

@benlesh
Copy link
Author

benlesh commented Jan 29, 2016

FWIW: There's a related discussion going on around this in RxJS 5: ReactiveX/rxjs#1255

@zenparsing
Copy link
Member

@Blesh I've been watching. I think adding the mapFn is fine.

@benlesh
Copy link
Author

benlesh commented Jan 29, 2016

We'll if it's added here, it strengthens the case for it there. I'm still iffy on the thisArg, though.

@benjamingr
Copy link

I just want to mention w.r.t the performance gain that @Blesh has solved it on the Rx side,

@zenparsing zenparsing changed the title Observable.from should have a mapFn argument Should Observable.from should have a mapFn argument? Mar 1, 2016
@zenparsing zenparsing changed the title Should Observable.from should have a mapFn argument? Should Observable.from have a mapFn argument? Mar 2, 2016
@benjamingr
Copy link

Can this be brought up at the next TC meeting and decided one way or the other? I wanted to use it the same way and was not sure if it's possible.

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

5 participants