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 we add a resultSelector to Observable.from? #1255

Closed
benlesh opened this issue Jan 27, 2016 · 11 comments
Closed

Should we add a resultSelector to Observable.from? #1255

benlesh opened this issue Jan 27, 2016 · 11 comments
Labels
help wanted Issues we wouldn't mind assistance with.
Milestone

Comments

@benlesh
Copy link
Member

benlesh commented Jan 27, 2016

The Good

  1. It adds symmetry to Array.from.
  2. It's compatible with Rx 4.
  3. It could reduce subscriptions created to map values from a source

The Bad

  1. It's not specifically compatible with the es-observable spec (cc @zenparsing)
  2. It brings up the debate on whether or not to provide a thisArg (it's there for Array.from)

The "meh"

  1. It doesn't necessary "break" compatibility with es-observable spec completely, unless it's being used.

Related #1009, #1166

@benlesh benlesh added help wanted Issues we wouldn't mind assistance with. type: discussion labels Jan 27, 2016
@benlesh benlesh added this to the 5.0.0-beta.2 milestone Jan 27, 2016
@benlesh benlesh changed the title Should we add a resultSelector to Observable.from Should we add a resultSelector to Observable.from? Jan 27, 2016
@staltz
Copy link
Member

staltz commented Jan 27, 2016

It adds symmetry to Array.from.

Meh. I don't see Array as the dual of Observable (Enumerable should be).

It's compatible with Rx 4.

Meh. (hope Matt doesn't hear this)

It could reduce subscriptions created to map values from a source

Reasonable.

It's not specifically compatible with the es-observable spec

Which is a moving target.


Disclaimer: I'm terse, not curt.

@justinwoo
Copy link
Contributor

Even though I made #1195 for Observable.from supporting either iterable, Scheduler OR iterable, mapFn, thisArg, Scheduler, I'd be totally for scrapping the resultSelector.

Also, I don't like the thisArg either way so I'd like to remove it (maybe someone's done analysis on JS in the wild using Array.from).

@mattpodwysocki
Copy link
Collaborator

@staltz I've got my 👀on you... I think about the efficiency standpoint of conversion for example to use the resultSelector instead of from(obj).map(fn) which could be costly.

@benlesh
Copy link
Member Author

benlesh commented Jan 29, 2016

I'm generally pro-resultSelector here. Mostly for the efficiency of it. I'm sure that's the primary reason for it existing in Array.from. I'm not totally sold on the thisArg thing though. Especially now that bind has improved in speed so much in newer engines.

@bholloway
Copy link

Forgive my ignorance but I am wondering what the use-case is for the resultSelector. An existing Array may be Array.map()'d when it is passed to the .from() operator, so I'm assuming that the resultSelector allows lazy processing for a cold observable .

Is this such a common pattern that the .map() operator has an impact on overall performance?

That said, the primary concern for a newcomer such as myself is consistency. The resultSelector is in the reactivex.io documentation and if I intended to use it I would be confused if it were not implemented.

The deciding factor may be whether it is easier to implement the feature than adjust the documentation.

@benlesh
Copy link
Member Author

benlesh commented Feb 8, 2016

@bholloway ... the use case is to reduce subscriptions for better performance, mostly. You "map" the value before it's passed into the subscriber (to be output by the observable).

@benjamingr
Copy link
Contributor

Just mentioning that the efficiency concern has been mostly addressed in #1323 . In my opinion Array.from compatibility is the only reason we should add the mapFn. As @staltz said Array is not the dual of Observable but that won't stop the TC from being problematic about it in es-observable since arrays have a .from method.

@benlesh
Copy link
Member Author

benlesh commented Feb 9, 2016

@benjamingr ... it hasn't completely been addressed. The flattening method I've used so far depends on chained operators at this point, and won't really apply to source observables like this one.

@benlesh benlesh modified the milestones: 5.0.0 release, 5.0.0-beta.2 Feb 10, 2016
@benlesh
Copy link
Member Author

benlesh commented Feb 10, 2016

Punting this to the next release since it's still under discussion.

@benlesh
Copy link
Member Author

benlesh commented Jun 17, 2016

Closing as stale.

@benlesh benlesh closed this as completed Jun 17, 2016
@lock
Copy link

lock bot commented Jun 7, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
help wanted Issues we wouldn't mind assistance with.
Projects
None yet
Development

No branches or pull requests

6 participants