fix(throttleTime): leading and trailing true should only emit a single value per time frame

[sod]

Fixes #2727

fix 1: throttleTime {leading: true, trailing: true}

Currently it behaves like

'a1234b1234c1234d12e--|'
'a----b1----12----2e--|'

By not scheduling a new throttle when emitting a trailing value.

this PR changes it to:

'a1234b1234c1234d12e--|'
'a----b----c----d----e|'

fix 2: throttleTime {leading: false, trailing: true}

Currently it behaves like

'a-------------a-|'
'----------------|'

First and only value in each timeframe is dropped by not storing the first value when not yet throttled. So this code doesn't console.log anything:

Rx.Observable.of(1)
    .throttleTime(100, undefined, {leading: false, trailing: true})
    .subscribe((value) => console.log(value))

This PR changes the behavior to:

'a-------------a-|'
'-----a-------------a|'

---

I added tests to cover those issues.

[rxjs-bot]

	Messages
📖

CJS: 1342.7KB, global: 737.4KB (gzipped: 137.9KB), min: 144.7KB (gzipped: 30.7KB)

Generated by 🚫 dangerJS

[coveralls]

Coverage remained the same at ?% when pulling 1b6c92a on sod:throttle-time-single-emit into d24f5b9 on ReactiveX:master.

[sod]

@benlesh it's a followup to #2465 ... what do you think? :)

[Parakleta]

I've been testing this, and the only hiccup is that the throttle call should come before the .next() emission because otherwise if the .next() is resolved synchronously it can really mess up the timing.

I suggest changing _next() to have something like:

let emit = false;
if (!this.throttled && this.leading) {
    emit = true;
} else ...

if (!this.throttled) this.throttle();
if (emit) this.destination.next(value);

And also clearThrottle to something like:

if (this.trailing && this._hasTrailingValue) {
    let value = this._trailingValue;
    this._trailingValue = null;
    this._hasTrailingValue = false;
    this.throttle();
    this.destination.next(value);
}

I have also recommended that this PR be replicated to solve #2466 but considering what I have discovered regarding the timing this may not be so simple.

[sod]

How did you test the hiccup? next is not being called when a throttle is present, so I can't see how that happens.

if (!this.throttled && this.leading) {
    this.destination.next(value);
} /* ...

[Parakleta]

Later in the same function is the call to this.throttle(). If next() takes 20ms to complete and our throttle is supposed to be 100ms then it will actually be 120ms between emissions because the setting of throttle doesn't start until after next completes.

[sod]

Thanks, now I understood the issue.

[sod]

rebased & added your feedback

[benlesh]

Can we refactor this a little to be:

if (!this.trottled) {
  this.throttle();
  if (this.leading) {
    this.destination.next(value);
  }
}

if (this.trailing) {
  this._trailingValue = value;
  this._hasTrailingValue = true;
}

I'm not sure in it's current configuration it will work as desired, because of that else if. If it's trailing, we always want to record the value, not only if we've already started throttling.

[sod]

Then throttleTime would emit the same input twice on {leading:true, trailing: true}. I guess in the end it doesn't matter when throttleTime is used in conjunction with distinctUntilChanged, but it feels wrong regardless.

Observable.of(1).throttleTime(100, {leading:true, trailing: true}).subscribe((n) => console.log(n));

// 1
// 1

vs. lodash

fn = _.throttle((n) => console.log(n), 100, {leading:true, trailing: true});
fn(1);

// 1

[benlesh]

this is unnecessary and changes the behavior of the operator. It's not supposed to start throttling again when the throttle is cleared.

[sod]

But this is exactly why I opened the PR. Because currently the operator emits 2 values each timeframe instead of 1.

If the doubleemit is intended, this PR can be closed and #2727 & #2859 rejected.

[benlesh]

hmm.. I think there's a disconnect here. throttle() actually starts the throttle measurement. The, with leading and trailing both true, it should emit the same as it would with leading behavior, but then emit the trailing value at the end.

[benlesh]

Oh, I okay, I actually see what you're saying now. This is to start the throttle again as soon as there's another emission (due to trailing behavior)

[benlesh]

Okay... the more I look at this, the more I think it will have to be a change that lands in v 6.0, because it will be a breaking change for someone. (Albeit a fix)

[benlesh]

this is unnecessary, the unsubscribe() will remove the inner subscription.

[benlesh]

if you could use the time function here and represent the time as marbles above like:

const t1 = '----|';

and then use time(t1) which returns a number.

That would be ideal as the test could be more readable, you can even used commented out ----| marblse to demonstrate where the throttles occur.

Remember, the whole purpose of these tests is to be more readable.

[benlesh]

Please do the same for the other tests.

[benlesh]

@sod can you look at what I've done with throttle in #2864 and try to mirror it as closely as possible to keep the code base consistent?

[lock]

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.