ReactiveX / rxjs

A reactive programming library for JavaScript
https://rxjs.dev
Apache License 2.0
30.7k stars 3k forks source link

debounce operator with duration selector #493

Closed kwonoj closed 8 years ago

kwonoj commented 9 years ago

While trying to migrate test coverage in RxJS4, could observe difference between signature of debounce operator

RxJS4

Rx.Observable.prototype.debounce(dueTime, [scheduler])
Rx.Observable.prototype.debounce(durationSelector)

while current signature only allows first one debounce<T>(dueTime: number, scheduler: Scheduler = nextTick)

Not sure if this is intended design or not - should operator to be updated, or test coverage for latter can be dropped off?

benlesh commented 9 years ago

Hmmm... I tell you what. Examine the level of complexity with supporting both on the same operator. If it incurs too many conditionals, we should probably seperate them into debounce and debounceTime

kwonoj commented 9 years ago

It'll also works. Just curious if it was intended.. I'll try to scope out changes and create PR based on those.

benlesh commented 9 years ago

Yeah... looking at this, it's probably best to rename what's there to debounceTime and add a debounce operator that takes an Observable as an argument. Likewise, we'll need to do this for throttle and throttleTime ... I'll take a look at that.

benlesh commented 9 years ago

What do you think @kwonoj? Do you agree?

kwonoj commented 9 years ago

What's interesting is, in case of throttle it didn't had durationSelector, (https://github.com/Reactive-Extensions/RxJS/blob/master/doc/api/core/operators/throttle.md)

Rx.Observable.prototype.throttle(windowDuration, [scheduler])

only. For alignment, I think it makes sense.