suggestion: consider improving the type signature for Observable.catch

[aluanhaddad]

RxJS version: 5.2.0

Code to reproduce:
case 1: Observable<number> decays to Observable<any>

import {Observable} from 'rxjs/Rx';

const o = Observable.of(1)
  .catch((err, caught) => Observable.throw(err))

case 2: Observable<number> decays to Observable<{}>

import {Observable} from 'rxjs/Rx';

const o = Observable.of(1)
  .catch((err, caught) => {
    throw Error(err);
  });

Expected behavior:
For o to have the type Observable<number> in both cases.

Actual behavior:
o has type Observable<any> in case 1.

o has type Observable<{}> in case 2.

This can mask serious bugs such as

function f() {
  const o = Observable.of(1)
    .map(() => {
      console.log(1); 
    }) // programmer likely meant to return something
    .catch((err, caught) => Observable.throw(err));
  return o;
}

Where f should have type Observable<void>, likely due to programmer error, but any absorbs even void.

Additional information:
For better or worse, the catch and rethrow idiom seems to have overtaken RxJS, sometimes even when they only rethrow (not even logging the error) developers insist on explicitly catching and rethrowing all exceptions. I suspect the Angular docs are the primary reason for this, but that is beside the point except to remark that clearer usage guidelines for these operators would be helpful. Catching and only rethrowing doesn't make much sense to me but I am very new to RxJS.

For reference here is what the idiom I see daily (at work and on Stack Overflow) looks like

import { Observable } from 'rxjs/Observable';
import 'rxjs/add/operator/map';
import 'rxjs/add/operator/catch';

export interface Something {
  name: string;
}

export class SomeService {
  getItems() {
    return this.http('api/something')
      .map(x => (x.json() as Something).name) // Observable<string>
      .catch((err, caught) => Observable.throw(err.json())); // Observable<any>
  }
}

The problem with this pattern, and the pattern really is pervasive in the Angular at least, is that chaining catch removes all type information unless the user actually uses catch to recover by supplying a new Observable. If they return the result of an Observable.throw call all further operations receive Observable<any>.

To fix the type issue, I suggest that the signatures of catch and _catch be changed from

export declare function _catch<T, R>(
    selector: (err: any, caught: Observable<T>) => ObservableInput<R>
): Observable<R>;

export interface CatchSignature<T> {
    <R>(selector: (err: any, caught: Observable<T>) => ObservableInput<R>): Observable<R>;
}

to

export declare function _catch<T>(
    this: Observable<T>, selector: (err: any, caught: Observable<T>) => ErrorObservable
): Observable<T>;

export declare function _catch<T, R>(
    this: Observable<T>, selector: (err: any, caught: Observable<T>) => ObservableInput<R>
): Observable<R>;

export interface CatchSignature<T> {
    (selector: (err: any, caught: Observable<T>) => ErrorObservable): Observable<T>;
    <R>(selector: (err: any, caught: Observable<T>) => ObservableInput<R>): Observable<R>;
}

This worked well when I tested it. I suppose the question is whether the idiom of catching and rethrowing is meant to be common for observables.

I would be happy to submit a PR for this if you are interested.

One final note, these could still arguably be more precise as the resulting stream from a catch returning a well-typed observable, e.g. Observable<string> when recovering from an error raised by an Observable<number> is arguably Observable<number | string> but that is perhaps out of scope.

Thank you.

[kwonoj]

I'd be curious of usecase for 2 that instead of returning ErrorObservable throw directly, but otherwise this looks valid approach we'd like to improve. If you have PR, it'd be appreciate.

Observable when recovering from an error raised by an Observable is arguably Observable<number | string> but that is perhaps out of scope.

refer #2478 for this, afaik it's dealing this one.

[aluanhaddad]

@kwonoj great to hear. Thanks for linking #2478.

With respect to usecase 2, I had never actually seen it written that way until I read the code example in the JSDoc comment for the catch operator.
I was actually wondering what the preferred style is. I take it that it is returning ErrorObservable.

[felixfbecker]

Also note that

.catch(err => Observable.throw(err))

is 100% equivalent to

.catch(err => {
  throw err;
})

So the overloaded signature should not only return Observable<T> in the case of return type ErrorObservable, but also in the case of return type never.

I met this a lot because I found a chain of

.catch(err => {
  span.setTag('error', true);
  span.log({ 'event': 'error', 'error.object': err, 'message': err.message, 'stack': err.stack });
  throw err;
})
.finally(() => {
  span.finish();
});

easier to read (because they are closer to their synchronous counterparts and because of #2534) than

.do(() => undefined, err => {
  span.setTag('error', true);
  span.log({ 'event': 'error', 'error.object': err, 'message': err.message, 'stack': err.stack });
})
.finally(() => {
  span.finish();
});

Unfortunately, the former cripples the return type.

[aluanhaddad]

@felixfbecker that's a great point. I also find that to be much more readable and it is mentioned in the JSDoc comment for the catch operator.

Using never is a great idea. If no one has any objections, I will update the PR to cover the use case where throw is used directly by creating such an overload.

[benlesh]

I think this has been addressed in #3395 #3421