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

Remove ScalarObservable optimizations for map, filter, reduce, scan, etc? #1142

Closed
benlesh opened this issue Jan 6, 2016 · 9 comments · Fixed by #1171
Closed

Remove ScalarObservable optimizations for map, filter, reduce, scan, etc? #1142

benlesh opened this issue Jan 6, 2016 · 9 comments · Fixed by #1171
Assignees
Labels
help wanted Issues we wouldn't mind assistance with.

Comments

@benlesh
Copy link
Member

benlesh commented Jan 6, 2016

There is an issue #1140 that just popped up (I was waiting for it).

I'm neither hot nor cold on these optimizations. So let's discuss them:

The Good:

  1. The optimizations only effect observables created like Observable.of(a), but not Observable.of(1,2,3). They only effect ScalarObservables.
  2. The optimizations doubled or tripled the speed of the operators for these Observables.
  3. No one should be doing Observable.of(a).map(toSomethingElse) very often. It's code that only really shows up in demos and playground code normally, and/or it's an anti-pattern usually.
  4. Generally it's a bad idea to mutate outer scoped variables inside of a map or filter handler.

The Bad:

  1. Users trying to leverage map to create side effects might see funky behavior for ScalarObservables
  2. The mapping or filtering functions are effectively no longer "lazy". If they're expensive or they create side effects, unexpected behavior can result.
@benlesh
Copy link
Member Author

benlesh commented Jan 6, 2016

AGAIN: I don't care if we keep them or remove them

The idea was that we'd implement them and try them out during beta.

@kwonoj
Copy link
Member

kwonoj commented Jan 6, 2016

I'm in favor of this in general, but this specific effectively no longer "lazy". If they're expensive or they create side effects, unexpected behavior can result is bit concern. Every user of of(x) need to be aware of behavior if we're going with this.

@trxcllnt
Copy link
Member

trxcllnt commented Jan 6, 2016

We should make them lazy. If someone wants to optimize so their ScalarObservable chains are only invoked once, they can multicast with a ReplaySubject.

@benlesh
Copy link
Member Author

benlesh commented Jan 7, 2016

This all sounds good... I'll let this simmer overnight.

I think we should also try to add some "protips" or something to the docs that say things like "If you're creating chains like Observable.of(a).map(doSomething), you might be doing something weird."

@staltz
Copy link
Member

staltz commented Jan 7, 2016

I'm also rather neutral on this one, but I tend to agree with Paul.

@benlesh
Copy link
Member Author

benlesh commented Jan 7, 2016

Meh. Let's remove them for now. Individuals that want this behavior could always monkey-patch ScalarObservable I suppose.

@benlesh benlesh added the help wanted Issues we wouldn't mind assistance with. label Jan 7, 2016
@chrisprice
Copy link
Contributor

Just before this is done in response to my issue can I double check that the problem I had is actually what's been discussed? I have a feeling it might just be a bug (see my latest comment on the issue)....

FWIW I do agree though, just not in my name! 😄

@benlesh
Copy link
Member Author

benlesh commented Jan 7, 2016

There are two issues: 1. the optimization which were discussing removing here, and 2. an apparent bug in the optimization that needs to be fixed or removed with the optimization.

kwonoj added a commit to kwonoj/rxjs that referenced this issue Jan 11, 2016
- remove usage of ScalarObservable for optimization
- implementation of ScalarObservable still remain for possible further usage

closes ReactiveX#1142, ReactiveX#1150, ReactiveX#1140
@benlesh benlesh self-assigned this Jan 12, 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
5 participants