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

combineLatest on synchronous sources #2414

Closed
staltz opened this issue Feb 24, 2017 · 7 comments
Closed

combineLatest on synchronous sources #2414

staltz opened this issue Feb 24, 2017 · 7 comments
Labels
bug Confirmed bug

Comments

@staltz
Copy link
Member

staltz commented Feb 24, 2017

RxJS version: 5.1.0

Code to reproduce:

const a$ = Rx.Observable.of(1, 2);
const b$ = Rx.Observable.of(10);

const c$ = Rx.Observable.combineLatest(a$, b$, (a,b)=>a+b);

c$.subscribe(c => console.log(c));

Expected behavior:

11
12

Actual behavior:

12

Additional information:

I'm not sure is this a bug or a "feature". What should we do about this case? Is it just one of those "let's instruct people to choose a different scheduler if they want to have the expected behavior"?

For reference:
xstream: staltz/xstream#173
most.js: cujojs/most#414

@trxcllnt
Copy link
Member

While zip is generally applicable to lists, combineLatest is undefined without the time dimension. The current behavior is fine, because the solution is to introduce your own notion of time. Rx generally shies away from defining one for you.

@mattpodwysocki
Copy link
Collaborator

mattpodwysocki commented Mar 6, 2017

@trxcllnt no, this is a bug and a breaking change from RxJS v4.x. Please fix. The expected values from @staltz were how RxJS v4 operated

@mattpodwysocki mattpodwysocki added the bug Confirmed bug label Mar 6, 2017
@trxcllnt
Copy link
Member

trxcllnt commented Mar 6, 2017

@mattpodwysocki you get this behavior from the recursive scheduler in v4. The "fix" in v5 is to use the queue scheduler. If you know of a solution that doesn't impact throughput performance, I'd be happy to change my recommendation.

@benlesh
Copy link
Member

benlesh commented Oct 12, 2017

I tend to agree with @trxcllnt on this one, although I haven't commented. Forcing queue scheduling on an operator in JavaScript is like trying to force the library to mimic some sort of gated/threaded model, which the majority of JS developers know it isn't. Once it's sunk in that Observable.of is synchronous by default the current behavior is obvious. What's outlined as the expected behavior here is less obvious as it deviates from how JavaScript fundamentally works.

Given this issue has been here this long and there's been little interest, I'd say the current behavior is fine and we can close the issue.

The "fix" if you want to change the behavior is to use the queue scheduler on incoming sources.

@c-dante
Copy link

c-dante commented Apr 2, 2018

I like the recommendation to define your own scheduling here. I disagree that 11, 12 is expected when using a synchronous observable like that. I think at best this is an item for the docs to make the Scheduler argument of Observable.of clearer for what behavior it affects.

@brucou
Copy link

brucou commented May 8, 2018

The biggest problem I see with Rxjs v5 semantics is that order matters, which is confusing at least to me.

That is to say

const a$ = Rx.Observable.of(1, 2);
const b$ = Rx.Observable.of(10);

const c$ = Rx.Observable.combineLatest(a$, b$, (a,b)=>a+b);

c$.subscribe(c => console.log(c));

will output 12 as Andre mentioned.

However, switch a and b and you will get 11, 12.

const a$ = Rx.Observable.of(1, 2);
const b$ = Rx.Observable.of(10);

const c$ = Rx.Observable.combineLatest(b$, a$, (a,b)=>a+b);

c$.subscribe(c => console.log(c));

With Rxjs v4, in both cases, the results were 11, 12.

I'd much prefer the rxjs v4 behaviour by default, as you would expect combineLatest by default to be commutative in its arguments. Then specific behaviour with specific schedulers.

@brucou
Copy link

brucou commented May 9, 2018

For what is worth, Bacon.js has the same behavior as rxjs v5: http://jsfiddle.net/wp3xp50e/

@lock lock bot locked as resolved and limited conversation to collaborators Jun 8, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Confirmed bug
Projects
None yet
Development

No branches or pull requests

6 participants