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

delayWhen should use an indefinite delay for notifiers that complete without emitting #3665

Closed
cartant opened this issue May 7, 2018 · 13 comments
Assignees
Labels
bug

Comments

@cartant
Copy link
Collaborator

@cartant cartant commented May 7, 2018

This issue's title has been changed, as the implementation of delayWhen is not correct - see the discussion below.

The original issue's description follows.

RxJS version: 6.1.0

Code to reproduce:

import { EMPTY, of } from "rxjs";
import { delay, delayWhen } from "rxjs/operators";

of(0).pipe(
    delayWhen(() => EMPTY.pipe(delay(0)))
).subscribe(console.log);

Expected behavior:

0 should be emitted and logged to the console.

Actual behavior:

Nothing is emitted and logged to the console.

If the of function is passed 1 instead of 0, 1 is logged to the console.

Additional information:

Looking at delayWhen, I can see another problem (other than #3663) with the implementation. If the value is falsy, and the notifier completes - rather than nexts - the value won't be emitted.

@benlesh
Copy link
Member

@benlesh benlesh commented May 9, 2018

So the semantics here seem off... The notifier completes without notifying, therefor the delay should last indefinitely. This isn't a bug with the library so much as it is with the usage.

@Airblader
Copy link
Contributor

@Airblader Airblader commented May 9, 2018

@benlesh The docs explicitly state that the notifier completing should cause the value to be emitted.

@benlesh
Copy link
Member

@benlesh benlesh commented May 21, 2018

🤦‍♂️

@benlesh
Copy link
Member

@benlesh benlesh commented May 21, 2018

Okay, so we need to fix the issue, but deprecate the behavior.

@Hyumiris
Copy link

@Hyumiris Hyumiris commented May 3, 2019

I have recently run into the same issue(s).

Regarding the deprecation

Even though there is a deprecation warning for Observable<never> this only works if the generated Observable is known to be of type never at compile time.
In every other case the deprecation warning isn't shown and the official documentation still doesn't mention anything about the depreciation.

Regarding the bug

The bug that the behaviour of the delayWhen operator depends on the truthiness of the source value persists as well.

Here is a more concise version of the reproduction code:

import { of, EMPTY } from 'rxjs'; 
import { delayWhen } from 'rxjs/operators';

of(true, false, 0, 1).pipe(
  delayWhen(() => EMPTY)
).subscribe(console.log);

// this logs:
// >> true
// >> 1

@BioPhoton
Copy link
Contributor

@BioPhoton BioPhoton commented Aug 12, 2019

@cartant how can we move forward with this?
Is this PR teambition/teambition-sdk#576 solving the iisue?

@xumepadismal
Copy link

@xumepadismal xumepadismal commented Dec 12, 2019

Hi guys! One more thought regarding the deprecation

Can someone explain to me why the deprecated version of delayWhen uses Observable<never> instead of Observable<void>? All the discussions is about notifiers that "complete without emit" whereas Observable<never> === "emit no items, never complete"...

@Airblader
Copy link
Contributor

@Airblader Airblader commented Dec 12, 2019

The generic is the type of the emitted values. void means it emits but without data, never means it never emits.

@xumepadismal
Copy link

@xumepadismal xumepadismal commented Dec 12, 2019

Ah got it... Thanks @Airblader!

So EMPTY and NEVER has identical type signatures and there is no way to distinguish them and that's why we get the problem described by @Hyumiris (about deprecation). Is it correct?

@cartant cartant changed the title delayWhen won't emit falsy values for notifiers that complete delayWhen should use an indefinite delay for notifiers that complete Feb 16, 2020
@cartant cartant self-assigned this Feb 16, 2020
@benlesh benlesh changed the title delayWhen should use an indefinite delay for notifiers that complete delayWhen should use an indefinite delay for notifiers that complete without emitting May 14, 2020
@benlesh benlesh added the help wanted label May 14, 2020
@benlesh
Copy link
Member

@benlesh benlesh commented May 14, 2020

This is still an issue and we should resolve it before we release v7, IMO.

@benlesh benlesh assigned benlesh and unassigned cartant Oct 7, 2020
@benlesh benlesh removed the help wanted label Oct 7, 2020
benlesh added a commit to benlesh/rxjs that referenced this issue Oct 7, 2020
- Resolves an issue where a duration selector that never emitted a value would cause the value to be emitted, even though there wasn't a "notification".
- `delayWhen` is now based on `mergeMap`. I started off with trying to use `mergeInternals`, but quickly realized it was doing the same thing as `mergeMap`, and I could just use that directly.

I'm not sure if this size optimization is "taking it too far", but I feel that `delayWhen` isn't exactly "hot path" code, so this optimization is fine, given the bundle savings it should yield.

BREAKING CHANGE: `delayWhen` will no longer emit if the duration selector simply completes without a value. Notifiers must notify with a value, not a completion.

fixes ReactiveX#3665
@roger6106
Copy link

@roger6106 roger6106 commented Aug 6, 2021

I've just spent a while tracking down an issue related to this. It appears that the documentation was not updated:

The source value is emitted on the output Observable only when the duration Observable emits a value or completes.

@ducthang-vu
Copy link

@ducthang-vu ducthang-vu commented May 24, 2022

This looks to be still an open issue. Any solution?

@roger6106
Copy link

@roger6106 roger6106 commented May 24, 2022

No one appears to be looking at this issue, so I've opened a new issue for it: #6977.

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

No branches or pull requests

8 participants