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

Last value not flushed #12

Closed
loreanvictor opened this issue Jan 30, 2021 · 1 comment · Fixed by #16
Closed

Last value not flushed #12

loreanvictor opened this issue Jan 30, 2021 · 1 comment · Fixed by #16

Comments

@loreanvictor
Copy link

loreanvictor commented Jan 30, 2021

I suspect this issue is related to #2 , but since that thread is specifically about supporting pullables, I thought perhaps a separate issue would do better.

The Issue

Last emitted value is not flushed. Basically the completion signal masks the last value received.

Example

In this example, nothing is logged.

import of from 'callbag-of';
import subscribe from 'callbag-subscribe';
import pipe from 'callbag-pipe';

import { debounce } from 'callbag-debounce';

pipe(
  of(42),
  debounce(1000),
  subscribe(console.log)
)

playground

Proposed Behavior

I could think of two alternatives:

  1. At the emission time for completion signal, also emit latest value.
I: -----(A)----- | ----- | -----(B)----- X
O: ----- | ----- | -----(A)----- | ----- | ----- | ----- (B) X

👉 debounce wait is 2. B is emitted 2 units after completion signal, alongside completion signal.
  e.g., the timeout is reset because of completion signal, but last value is also emitted.
  1. At the debounced time for last emission, emit completion if completion signal is received in the meanwhile.
I: -----(A)----- | ----- | -----(B)----- X
O: ----- | ----- | -----(A)----- | ----- | -----(B) X

👉 debounce wait is 2. B is emitted 2 units after source emits B, alongside completion signal.
   e.g. the timeout is not reset because of completion signal, but it will be emitted alongside last value.

Reasoning

  1. debounce() is (IMHO) the most applicable delay operator, simply because it always emits the latest value (for example throttle() actually loses latest values, etc). However, the current implementation is lossy in edge cases, which takes
    away that applicability.

  2. Debouncing in higher-order streams typically looks like this:

pipe(
  source,
  map(val =>
    pipe(
      of(val),
      debounce(1000),
    )
  ),
  flatten
)

Which does not work with current implementation and would need a separate, delay only operator. It would be much more intuitive if you would not need to change the used operator for debouncing between these two situations.

P.s. in case you are wondering why would someone debounce in a higher-order stream, it is typically for situations where you want to conditionally debounce.

@atomrc
Copy link
Owner

atomrc commented Jun 21, 2021

hi @loreanvictor ,

Thanks for a very complete and comprehensive report. Deeply sorry for my lack of reply.
But I decided to implement this fix.

I choose your second alternative, I though it made a lot of sense (do not delay the last value more and send the termination signal along with it).

This is released with version 4.0.0 already published on NPM.
Thanks again 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants