Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

Reconsider inspectTime #4429

Closed
pcurrivan opened this issue Dec 18, 2018 · 7 comments
Closed

Reconsider inspectTime #4429

pcurrivan opened this issue Dec 18, 2018 · 7 comments

Comments

@pcurrivan
Copy link

Feature Request

Of the five *Time operators, it seems to me you guys removed the most useful one. inspectTime was the only operator that behaved predictably, stayed up to date, and did not make you wait.

Here's a break down of the five *Time operators:

- - - 1 2 3 - - - - - - - - - - - 4 5 6 - - - 7 8 - - - - - - - - 9 - - - -  input sequence

- - - 1 - - - - - - - - - - - - - 4 - - - - - 7 - - - - - - - - - 9 - - - -  throttleTime(4) - does not stay up to date
- - - 0 1 2 3 - - - - - - - - - - 0 1 2 3 - - 0 1 2 3 - - - - - - 0 1 2 3 -  (timer) - emit on 0

- - - - - - - 3 - - - - - - - - - - - - - 6 - - - - - 8 - - - - - - - - - 9  auditTime(4) - makes you wait
- - - 0 1 2 3 4 - - - - - - - - - 0 1 2 3 4 - 0 1 2 3 4 - - - - - 0 1 2 3 4  (timer) - emit on 4

- - - - - - - - - 3 - - - - - - - - - - - - - - - - - - 8 - - - - - - - - 9  debounceTime(4) - makes you wait even more, skips entirely bursts that are close together
- - - 0 0 0 1 2 3 4 - - - - - - - 0 0 0 1 2 3 0 0 1 2 3 4 - - - - 0 1 2 3 4  (timer) - emit on 4, input refreshes timer

- - - - 2 - - - 3 - - - - - - - - - - - 6 - - - 8 - - - - - - - - - - - 9 -  sampleTime(4) - *erratically* makes you wait
0 1 2 3 0 1 2 3 0 1 2 3 0 1 2 3 0 1 2 3 0 1 2 3 0 1 2 3 0 1 2 3 0 1 2 3 0 1  (timer) - emit on 0 if input since last emission, timer restarts automatically

- - - 1 - - - 3 - - - - - - - - - 4 - - - 6 - - - 8 - - - - - - - 9 - - - -  inspectTime(4) - simply limits the frequency.  REMOVED from RxJS https:/ReactiveX/RxJS/commit/17341a4
- - - 0 1 2 3 0 1 2 3 - - - - - - 0 1 2 3 0 1 2 3 0 1 2 3 - - - - 0 1 2 3 -  (timer) - emit on 0 and 4

Is your feature request related to a problem? Please describe.
I'm always frustrated when I have a sequence of user interaction events that cause server requests and I want to limit the frequency of the server requests while keeping a good user experience. A good user experience means:

  • predictability - the same pattern of user interaction should always have the same responsiveness. (sampleTime, for example, has unpredictable responsiveness depending on when the user happens to take an action, modulo the sample timer).
  • stays up-to-date - after a burst of inputs, the last emission should use the latest input. (throttleTime fails here)
  • doesn't make you wait - the first input should be immediately handled. (auditTime and debounceTime fail here. sampleTime fails to a random degree)

Describe the solution you'd like
Restore inspect and inspectTime operators.

Describe alternatives you've considered
The other four *Time operators. It was suggested here that people can use withLatestFrom to somehow get "close" to inspectTime, but it's not clear to me how this is to be done. @staltz claims here that inspectTime can be fully replicated with withLatestFrom. I don't believe this is true. Even if it is true (please educate me if it is so), consider that it might be difficult enough to justify including inspectTime anyway.

(If this is new operator request) describe reason it should be core operator
Again, it seems like you removed the best of the five operators. It doesn't make sense to me.

@pcurrivan
Copy link
Author

pcurrivan commented Jan 10, 2019

Seems like you can just about recreate it with throttleTime:

pipe(
      throttleTime(2000, undefined, {leading: true, trailing: true}),
      throttleTime(1999)
    )

You need the second throttleTime or else inputs at, eg. 0, 100, and 2100 will cause emissions at 0, 2000, and 2100 (emissions clump up on the edges because throttleTime with leading:true and trailing:true does not keep a delay timer after the trailing emission).

edit: the above doesn't work. I just made a custom operator to do what I want.

@benlesh
Copy link
Member

benlesh commented Jan 17, 2019

@pcurrivan can you link the operator you created here if it's open source? That way people that are looking for it can find it if they find this issue.

We're always happy to consider these things, but there's also the pressure of getting a lot of flak for having so many operators to start with. 😆 🤷‍♂️

@pcurrivan
Copy link
Author

Sure, here it is. I just wrapped some plain JS I had lying around after I had trouble getting the old inspectTime code to work and I couldn't quickly wrap my head around the abstractions used in existing RxJS operator code.

@BioPhoton
Copy link
Contributor

@pcurrivan maybe you look for throttleTime with the ThrottleConfig object of {leading: true, trailing: true}.

If yes, please close the issue. If no @benlesh should consider either closing this one or give some suggestions for the next steps

@MatthiasKunnen
Copy link
Contributor

@BioPhoton, unfortunately, that configuration does not work the way you think.

source:    a123b12-c-23d-2-ef---
requested: a---b---c---d---e---f
actual:    a---b1---c2---2-e---f

As you can see, throttleTime with leading and trailing disabled does not work as the operator requested in this issue.

See issue #2727 and a proposed fix in form of pr #4727. The problem is that maintainers aren't sure about changing throttleTime.

@adolgoff
Copy link

I had to write my own implementation

import {Observable} from 'rxjs';

export const inspectTime = (delay: number) => (source: Observable<any>) => {
  let prevCallTime: number = null;
  let timeout: NodeJS.Timeout;
  return new Observable<any>((observer) => {
    return source.subscribe({
      next(value) {
        if (!prevCallTime || new Date().getTime() - prevCallTime > delay) {
          prevCallTime = new Date().getTime();
          observer.next(value);
        } else {
          clearTimeout(timeout);
          timeout = setTimeout(() => {
            observer.next(value);
            prevCallTime = new Date().getTime();
          }, delay - (new Date().getTime() - prevCallTime));
        }
      },
      error(err) {
        observer.error(err);
      },
      complete() {
        clearTimeout(timeout);
        observer.complete();
      },
    });
  });
};

@MatthiasKunnen
Copy link
Contributor

@adolgoff, if you want a tested implementation, look at PR #4727.

I'd really like either inspectTime to be reintroduced or aforementioned PR to be merged.

benlesh added a commit to benlesh/rxjs that referenced this issue Sep 2, 2020
…least the throttled amount

Works to align the behavior with expectations set by lodash's throttle

- Updates tests
- Ensures trailing throttle will wait to notify and then complete
- Ensures that every time we emit a value a new throttle period starts

fixes ReactiveX#3712
related ReactiveX#4864
fixes ReactiveX#2727
closes ReactiveX#4727
related ReactiveX#4429
benlesh added a commit to benlesh/rxjs that referenced this issue Sep 3, 2020
…least the throttled amount

Works to align the behavior with expectations set by lodash's throttle

- Updates tests
- Ensures trailing throttle will wait to notify and then complete
- Ensures that every time we emit a value a new throttle period starts

fixes ReactiveX#3712
related ReactiveX#4864
fixes ReactiveX#2727
closes ReactiveX#4727
related ReactiveX#4429
benlesh added a commit that referenced this issue Sep 3, 2020
…least the throttled amount (#5687)

* fix(throttleTime): ensure the spacing between throttles is always at least the throttled amount

Works to align the behavior with expectations set by lodash's throttle

- Updates tests
- Ensures trailing throttle will wait to notify and then complete
- Ensures that every time we emit a value a new throttle period starts

fixes #3712
related #4864
fixes #2727
closes #4727
related #4429

* chore: Address comments and add comments to the code
@benlesh benlesh closed this as completed May 4, 2021
@ReactiveX ReactiveX locked and limited conversation to collaborators May 4, 2021

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants