Throttle fix

[benlesh]

To address #2859

[rxjs-bot]

	Messages
📖

CJS: 1341.7KB, global: 736.8KB (gzipped: 137.9KB), min: 144.4KB (gzipped: 30.7KB)

Generated by 🚫 dangerJS

[kwonoj]

do we need test cases for { leading: true, trailing: false }? seems not needed maybe.

[sod]

Was something wrong with #2749?

[benlesh]

@sod nope... I just didn't see it. sorry.

[benlesh]

Okay, @sod I've reviewed your PR. there were a lot of changes I requested, if you think you can knock those out, that would be great. I'll just ween this PR down to only be the throttle operator, as your fixes were only to throttleTime.

[Parakleta]

This has the same problem that #2749 had initially in that it defers the setup of the throttle observable until after emitting the value, and this can lead to drift in the throttling. I think it needs to look something like:

protected _next(value: T): void {
    this._hasValue = true;
    this._sendValue = value;

    if (!this._throttled) {
        this.throttle(this._leading);
    }
}
private throttle(doSend: boolean) {
    const { _hasValue, _sendValue } = this;
    if (!_hasValue) return;

    let duration, err;
    try {
        duration = this.durationSelector(_sendValue);
    } catch (e) {
        err = e;
    }

    if (doSend) {
        this.destination.next(_sendValue);
        this._hasValue = false;
        this._sendValue = null;
    }

    if (err != null) {
        this.destination.error(err);
    } else {
        this.add(this._throttled = subscribeToResult(this, duration));
    }
}
private throttlingDone() {
    const { _throttled, _trailing } = this;
    if (_throttled) {
        _throttled.unsubscribe();
    }
    this._throttled = null;

    if (_trailing) {
        this.throttle(true);
    }
}

The throttle() method feels untidy to me, but I'm not sure how to make it nicer.

[micha149]

Whats up with this PR?

[benlesh]

@micha149

it needs to be rebased, and I haven't gotten back around to it.

[micha149]

@benlesh thanks for the response. will it be possible to get this into Version 6.0.0? Can I support by rebasing your commit?

[benlesh]

@micha149 Yeah, we're going to get this in. Feel free to rebase if you have the time.

[micha149]

@benlesh I rebased the commit. As far as I know I'm not able to get it into this PR. You can pull from here: micha149@df689e5

[Parakleta]

This still doesn't deal with the issue that a slow next() emission will cause the throttling to drift (or have inconsistent duration) as discussed in #2749. I think swapping lines https://github.com/micha149/rxjs/blob/df689e5550bef58f534d8779b2c7c27d35440089/src/internal/operators/throttle.ts#L113-L114 should fix it.

[benlesh]

Changes coped to #3505

[coveralls]

Coverage remained the same at ?% when pulling ccca46c on benlesh:throttle_fix into d24f5b9 on ReactiveX:master.

[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.