fix(delayWhen): Emit source value if duration selector completes synchronously

[Airblader]

This fixes an issue where delayWhen would not re-emit a source emission if the duration selector
completed synchronously.

fixes #3663

[Airblader]

I guess a problem with this is that this is also true if the selector errors synchronously(?). Is there a better way to check this?

[coveralls]

Coverage decreased (-0.2%) to 96.65% when pulling 6ee62c4 on Airblader:bug-3663 into d7bfc9d on ReactiveX:master.

[benlesh]

Actually, looking at this operator more closely, I suspect the whole thing is implemented incorrectly. We shouldn't be count completions as as notification of the delay being over. If an observable has no nexted values, it has no values, this just seems semantically wrong.

[benlesh]

I'm leaning towards merging this fix, but deprecating the behavior. Because I think the behavior is incorrect.

[benlesh]

cc @kwonoj

[Airblader]

Deprecating it would align it a bit more with operators like takeUntil.

[benlesh]

Okay, @Airblader, can you please rebase this... and then we'll need to add a type signature to it with a @deprecated message on it talking about how the behavior will change.

Basically, an empty observable is an observable that never notifies. So in the future people will want to have something that returns a value.

I think something like this will be work? cc @cartant @david-driscoll

/** @deprecated in future versions, empty notifiers will not trigger delayed behavior */
export function delayWhen<T>(notifier: () => ObservableInput<never>);

[benlesh]

add deprecation message for () => ObservableInput<never> calls

[Airblader]

@benlesh Rebased.

I think something like this will be work?

I've added it, I hope it's OK this way? I took your suggestion, even though the signature is a bit different from the operator. I've also added a note to the documentation itself since it currently explicitly mentions the completion behavior.

[Airblader]

Looks like the proposed signature for deprecation isn't compatible.

[benlesh]

@Airblader it looks like we have a type misalignment between this and the implementation. At first blush it looks okay, but there's something a little off.

This is so close, just straighten that issue out, and we'll get this in. You're going to have to play with the typings a bit. Try calling the method if VS Code with delayWhen(() => EMPTY) and see what you get.

[Airblader]

@benlesh I tried playing around with this a little, but I couldn't find a way to make all cases work (i.e., have deprecation show only if an empty observable is returned, but neither deprecation nor an error otherwise).

[cartant]

@Airblader I've had a quick look at this. The basic problem is that the function didn't previously have overload signatures and - in adding one - you've hidden the function's signature, as when overload signatures are specified, the signature of the function's implementation is no longer visible/callable.

So you need to add two overload signatures, one deprecated and one not:

/* tslint:disable:max-line-length */
/** @deprecated In future versions, empty notifiers will no longer re-emit the source value on the output observable. */
export function delayWhen<T>(notifier: (value: T) => Observable<never>, subscriptionDelay?: Observable<any>);
export function delayWhen<T>(notifier: (value: T) => Observable<any>, subscriptionDelay?: Observable<any>);
/* tslint:disable:max-line-length */

/**
 * Delays the emission of items from the source Observable by a given time span
 * determined by the emissions of another Observable.
 * ...
 */
export function delayWhen<T>(delayDurationSelector: (value: T) => Observable<any>,
                             subscriptionDelay?: Observable<any>): MonoTypeOperatorFunction<T> {
  ...

Also, I would suggest leaving the return value typed as Observable. If the typings could/should be changed to accept functions returning ObservableInput, it would be preferable if that were done separately, perhaps when the test suite is better able to test types - which should be very soon.

[Airblader]

So you need to add two overload signatures, one deprecated and one not:

I tried exactly this solution (with all the details you mentioned, except that yours are missing the return type), but

/** @deprecated In future versions, empty notifiers will no longer re-emit the source value on the output observable. */
export function delayWhen<T>(notifier: (value: T) => Observable<never>, subscriptionDelay?: Observable<any>): MonoTypeOperatorFunction<T>;
export function delayWhen<T>(notifier: (value: T) => Observable<any>, subscriptionDelay?: Observable<any>): MonoTypeOperatorFunction<T>;

fails these tests for me:

// should be marked deprecated, but isn't
of(42).pipe(delayWhen(() => EMPTY));
// should be marked deprecated, but isn't
of(42).pipe(delayWhen(value => EMPTY));
// should not be marked deprecated, but is
const selector: (value: number) => Observable<any> = value => of(1337);
of(42).pipe(delayWhen(selector));

Particularly how the last one happens is beyond me, to be honest.

[cartant]

Yeah, I missed the return type. They should definitely be there.

However, the overloads work fine for me, marking what should be deprecated and not marking what shouldn't.

I tested this by adding the first overload to the delayWhen.d.ts (within node_modules/rxjs/...) in a scratchpad project. What I added should be no different to what's emitted when the two overload declarations are included in the source.

That is, the .d.ts contains this:

import { Observable } from '../Observable';
import { MonoTypeOperatorFunction } from '../types';

/** @deprecated */
export declare function delayWhen<T>(delayDurationSelector: (value: T) => Observable<never>, subscriptionDelay?: Observable<any>): MonoTypeOperatorFunction<T>;
export declare function delayWhen<T>(delayDurationSelector: (value: T) => Observable<any>, subscriptionDelay?: Observable<any>): MonoTypeOperatorFunction<T>;

I'm not sure how you are testing this, but the solution should work just fine. This how all of the other signature-specific deprecations work, so the mechanism should be sound.

[Airblader]

The idea of this deprecation notice, even if we get it to work, would by the way also mark delayWhen(() => NEVER) as deprecated even though it shouldn't as behavior won't change here. To be fair, though, having this in any application makes no sense.

[Airblader]

@cartant I don't doubt that something is fishy on my end here. Creating a new scratch file with

interface Observable<T> {}

const ANY: Observable<any> = {} as Observable<any>;
const NEVER: Observable<never> = {} as Observable<never>;

/** @deprecated */
function delayWhen<T>(notifier: (value: T) => Observable<never>): void;
function delayWhen<T>(notifier: (value: T) => Observable<any>): void;

function delayWhen<T>(notifier: (value: T) => Observable<any>): void {
  return;
}

delayWhen<number>(() => NEVER);
delayWhen<number>(x => NEVER);
delayWhen<number>(() => ANY);
delayWhen<number>(x => ANY);

shows all four calls as deprecated to me. I'll just update the PR as you suggested for now.

[Airblader]

PR is updated as discussed, let's see if Travis is happy now. I'll assume that my IDE is being funky here since I don't have access to my workstation right now and am using a laptop that I don't usually use for development.

[benlesh]

LGTM.