fix(defer) Disallow passing () => undefined to observableFactory (#5449)

* fix(defer) Disallow passing () => undefined to observableFactory

* Fixed tests

* lint

* moar dstlint tests

* address comments

* chore: fix small typo

Co-authored-by: Moshe Kolodny <kolodny@github.com>
Co-authored-by: Ben Lesh <ben@benlesh.com>

BREAKING CHANGE: `defer` no longer allows factories to return `void` or `undefined`. All factories passed to defer must return a proper `ObservableInput`, such as `Observable`, `Promise`, et al. To get the same behavior as you may have relied on previously, `return EMPTY` or `return of()` from the factory.

spec-dtslint/observables/defer-spec.ts

@@ -16,14 +16,33 @@ it('should support union type returns', () => {
   const a = defer(() => Math.random() > 0.5 ? of(123) : of('abc')); // $ExpectType Observable<string | number>
 });
 
-it('should infer correctly with void functions', () => {
-  const a = defer(() => {}); // $ExpectType Observable<never>
+it('should error with void functions', () => {
+  const a = defer(() => {}); // $ExpectError
 });
 
 it('should error if an ObservableInput is not returned', () => {
   const a = defer(() => 42); // $ExpectError
 });
 
+it('should error if function returns undefined', () => {
+  const a = defer(() => undefined); // $ExpectError
+});
+
+it('should error if function returns never', () => {
+  const a = defer(() => { throw new Error(); }); // $ExpectError
+});
+
+
+it('should infer correctly with function that sometimes error', () => {
+  // $ExpectType Observable<number>
+  defer(() => {
+    if (Math.random() > 0.5) {
+       throw new Error();
+    }
+    return of(1, 2, 3);
+  });
+});
+
 it('should infer correctly with functions that sometimes do not return an ObservableInput', () => {
-  const a = defer(() => { if (Math.random() < 0.5) { return of(42); } }); // $ExpectType Observable<number>
+  const a = defer(() => { if (Math.random() < 0.5) { return of(42); } }); // $ExpectError
 });

spec/observables/defer-spec.ts

@@ -74,15 +74,30 @@ describe('defer', () => {
     expectSubscriptions(source.subscriptions).toBe(sourceSubs);
   });
 
-  it('should create an observable when factory throws', () => {
+  it('should create an observable when factory does not throw', () => {
     const e1 = defer(() => {
-      throw 'error';
+      if (1 !== Infinity) {
+        throw 'error';
+      }
+      return of();
     });
     const expected = '#';
 
     expectObservable(e1).toBe(expected);
   });
 
+  it('should error when factory throws', (done) => {
+    const e1 = defer(() => {
+      if (1 + 2 === 3) {
+        throw 'error';
+      }
+      return of();
+    });
+    e1.subscribe({
+      error: () => done()
+    });
+  });
+
   it('should allow unsubscribing early and explicitly', () => {
     const source = hot('--a--b--c--|');
     const sourceSubs = '^     !     ';

spec/operators/race-spec.ts

@@ -175,7 +175,7 @@ describe('race operator', () => {
 
   it('should ignore latter observables if a former one emits immediately', () => {
     const onNext = sinon.spy();
-    const onSubscribe = sinon.spy();
+    const onSubscribe = sinon.spy() as any;
     const e1 = of('a'); // Wins the race
     const e2 = defer(onSubscribe); // Should be ignored
 
@@ -186,7 +186,7 @@ describe('race operator', () => {
 
   it('should ignore latter observables if a former one completes immediately', () => {
     const onComplete = sinon.spy();
-    const onSubscribe = sinon.spy();
+    const onSubscribe = sinon.spy() as any;
     const e1 = EMPTY; // Wins the race
     const e2 = defer(onSubscribe); // Should be ignored
 
@@ -197,7 +197,7 @@ describe('race operator', () => {
 
   it('should ignore latter observables if a former one errors immediately', () => {
     const onError = sinon.spy();
-    const onSubscribe = sinon.spy();
+    const onSubscribe = sinon.spy() as any;
     const e1 = throwError('kaboom'); // Wins the race
     const e2 = defer(onSubscribe); // Should be ignored
 

spec/operators/raceWith-spec.ts

@@ -161,7 +161,7 @@ describe('raceWith operator', () => {
 
   it('should ignore latter observables if a former one emits immediately', () => {
     const onNext = sinon.spy();
-    const onSubscribe = sinon.spy();
+    const onSubscribe = sinon.spy() as any;
     const e1 = of('a'); // Wins the race
     const e2 = defer(onSubscribe); // Should be ignored
 
@@ -172,7 +172,7 @@ describe('raceWith operator', () => {
 
   it('should ignore latter observables if a former one completes immediately', () => {
     const onComplete = sinon.spy();
-    const onSubscribe = sinon.spy();
+    const onSubscribe = sinon.spy() as any;
     const e1 = EMPTY; // Wins the race
     const e2 = defer(onSubscribe); // Should be ignored
 
@@ -183,7 +183,7 @@ describe('raceWith operator', () => {
 
   it('should ignore latter observables if a former one errors immediately', () => {
     const onError = sinon.spy();
-    const onSubscribe = sinon.spy();
+    const onSubscribe = sinon.spy() as any;
     const e1 = throwError('kaboom'); // Wins the race
     const e2 = defer(onSubscribe); // Should be ignored
 

src/internal/observable/defer.ts

@@ -1,7 +1,6 @@
 import { Observable } from '../Observable';
 import { ObservedValueOf, ObservableInput } from '../types';
 import { from } from './from'; // lol
-import { EMPTY } from './empty';
 
 /**
  * Creates an Observable that, on subscribe, calls an Observable factory to
@@ -52,16 +51,18 @@ import { EMPTY } from './empty';
  * @name defer
  * @owner Observable
  */
-export function defer<R extends ObservableInput<any> | void>(observableFactory: () => R): Observable<ObservedValueOf<R>> {
+export function defer<R extends ObservableInput<any>>(
+  observableFactory: [R] extends [undefined] | [void] ? never : () => R
+): Observable<ObservedValueOf<R>> {
   return new Observable<ObservedValueOf<R>>(subscriber => {
-    let input: R | void;
+    let input: R;
     try {
       input = observableFactory();
     } catch (err) {
       subscriber.error(err);
       return undefined;
     }
-    const source = input ? from(input as ObservableInput<ObservedValueOf<R>>) : EMPTY;
+    const source = from(input);
     return source.subscribe(subscriber);
   });
 }