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

debounceTime doesn't work with Subject #1081

Closed
tandu opened this issue Dec 16, 2015 · 9 comments · Fixed by #1083
Closed

debounceTime doesn't work with Subject #1081

tandu opened this issue Dec 16, 2015 · 9 comments · Fixed by #1083

Comments

@tandu
Copy link

tandu commented Dec 16, 2015

We had a discussion here angular/angular#5925. It seems that throttleTime works with Subject and debounceTime doesn't work with Subject.
throttleTime: http://jsfiddle.net/bo6ynqf7/1/
debounceTime: http://jsfiddle.net/8g968xh7/1/

@kwonoj
Copy link
Member

kwonoj commented Dec 16, 2015

Would you elaborate bit more detail of 'doesn't work'? I tried this
http://jsfiddle.net/8g968xh7/2/ which has slightly modified such as below

const { Subject } = Rx;

const button = document.querySelector('button');
const pre = document.querySelector('pre');

var sub = new Subject()

sub.debounceTime(1000)
    .subscribe(_=> pre.innerHTML += _);

button.addEventListener('click', function(){
  sub.next('a');
});

and could see debounceTime emits. Does doesn't work means emitting behavior is not expected, or is it not emitting at all?

@tandu
Copy link
Author

tandu commented Dec 16, 2015

Hm, it seems that sub.next() is working for throttleTime but for debounceTime it should pass any value. So for debounceTime sub.next() is not fired but if you send anything like sub.next(0) or sub.next('a') it works as expected.

@tandu
Copy link
Author

tandu commented Dec 16, 2015

By doesn't work I meant it was not emitting at all.

@kwonoj
Copy link
Member

kwonoj commented Dec 16, 2015

Ah, that's currently expected behavior from implementation of debounceTime(). Looking at implementation,

...
if (this.lastValue != null) {
      this.destination.next(this.lastValue);
      this.lastValue = null;
    }

it only emits if it explicitly have values to be emitted. So while subject emits next without any value, debounceTime simply skips it since there's no value.

note : just clarify, 'currently expected behavior ' simply means it works in those way for now.

@chicoxyzzy
Copy link

looks inconsistent

@kwonoj
Copy link
Member

kwonoj commented Dec 16, 2015

Behavior perspective it does. Few things to be considered are,

  • Subject::next() requires value as paramater, it should not be skipped. So in case of above example, calling next(any) ensures operator behavior.
  • Edge case in this issue is case of emitting undefined as value, since next() is same as next(undefined). This is case shows different behavior compare to RxJS4.

kwonoj added a commit to kwonoj/rxjs that referenced this issue Dec 16, 2015
due to current null check, debounceTime does not allow value
to be undefined. This change updates behavior to align behavior
as same as RxJS4 does.

closes ReactiveX#1081
@staltz
Copy link
Member

staltz commented Dec 16, 2015

@kwonoj debounceTime should have a hasValue-based implementation, like e.g. distinctUntilChanged:

class DistinctUntilChangedSubscriber<T> extends Subscriber<T> {
  private value: T;
  private hasValue: boolean = false;

So this bug is legit it seems.

@kwonoj
Copy link
Member

kwonoj commented Dec 16, 2015

@staltz , Yes, @mattpodwysocki suggested same approach in current PR (#1083 (comment)). I'm updating PR to reflect suggestions.

kwonoj added a commit to kwonoj/rxjs that referenced this issue Dec 16, 2015
due to current null check, debounceTime does not allow value
to be undefined. This change updates behavior to align behavior
as same as RxJS4 does.

closes ReactiveX#1081
@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
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants