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

providing .sample() the same observable it samples produces stale output #2075

Closed
nabati opened this issue Oct 25, 2016 · 4 comments · Fixed by #2077
Closed

providing .sample() the same observable it samples produces stale output #2075

nabati opened this issue Oct 25, 2016 · 4 comments · Fixed by #2077
Assignees
Labels
bug Confirmed bug

Comments

@nabati
Copy link

nabati commented Oct 25, 2016

Thanks for the skeleton @jayphelps

RxJS version:
5.0.0.beta-12

Code to reproduce:
http://jsbin.com/sapuju/4/edit?js,console,output

Expected behavior:
Open the console. 3

Actual behavior:
2

Additional information:

@jayphelps
Copy link
Member

jayphelps commented Oct 25, 2016

@Blesh Is this possibly related to our change in default schedulers? That was my initial hunch, but then I thought that the order should still be the same, even if it's no longer scheduled on the microtask. My latest guess is that this is primary due to the .sample() and the observable provided to .sample() (sampleObservable) both subscribing to the same subject, and in v4 perhaps it has a subtle difference in subscription order compared to v5? i.e. the sampleObservable item$.filter(item => item === 3) is subscribed to before the .sample() operator subscribes to the subject, so when it receives values, .sample() has not yet received that value yet, hence why it outputs 2 instead of 3.

If that's the case, which order is correct is prolly debatable but I think it will only have a practical difference in cases like these so we should prolly reverse the subscription order. ...first glance suggests doing so would be pretty internally awkward though..


For other's reference:

RxJS v4 defaulted to micro-task scheduling (was called AyncScheduler but in v5 this is called AsapScheduler and AsyncScheduler in v5 is different!). In v5, for performance reasons, we default to having no scheduler, doing things now, synchronously. (that was called ImmediateScheduler in v4). This is all confusing, we realize, but the changes were thoroughly bikeshed and made for good reasons.

More info on v5 schedulers

@jayphelps jayphelps added the bug Confirmed bug label Oct 25, 2016
@jayphelps jayphelps changed the title Issue with sample? providing .sample() the same observable it samples produces stale output Oct 25, 2016
@jayphelps
Copy link
Member

jayphelps commented Oct 25, 2016

I think it's more clear to reproduce this without the filter. http://jsbin.com/coreni/edit?js,output

const item$ = new Subject();

item$
  .sample(item$)
  .subscribe(value => {
    console.log(value);
  });

item$.next(1);
item$.next(2);
item$.next(3);

v4: 1 2 3
v5: 1 2

@benlesh benlesh self-assigned this Oct 25, 2016
@benlesh
Copy link
Member

benlesh commented Oct 25, 2016

I'll take a look at this

@lock
Copy link

lock bot commented Jun 6, 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 6, 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

Successfully merging a pull request may close this issue.

3 participants