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

fix(catch): fix catch typings #2478

Merged
merged 1 commit into from
Apr 3, 2017
Merged

Conversation

ghetolay
Copy link
Contributor

Description:

The catch operator is returning an observable with the type of the innerObservable (observable returned when error occur).
It should be a union of the outerObservable's type and the innerObservable's type :

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

returned type is the union of inputs type
@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.689% when pulling c6c80d9 on ghetolay:catch_typings into 01e1343 on ReactiveX:master.

@kwonoj
Copy link
Member

kwonoj commented Mar 19, 2017

Would you mind share examples of It should be a union of the outerObservable's type?

@david-driscoll
Copy link
Member

Would be great to see the use case for this... catch allows you to handle an error and decide what to do. There is nothing stopping you from doing something like...

o.catch((e, c) => c);

Which would make T -> R. With this change then the output would always be a special union of two potentially different types.

@ghetolay
Copy link
Contributor Author

ghetolay commented Mar 19, 2017

Any example where you would return an observable with a different type on the catch would reveal the need of union. Take the first example given on docs :

Observable.of(1, 2, 3, 4, 5)
   .map(n => {
 	   if (n == 4) {
 	     throw 'four!';
     }
	   return n;
   })
   .catch(err => Observable.of('I', 'II', 'III', 'IV', 'V'))
   .subscribe(x => console.log(x));

x should be of type number | string and not just number.

But I think this apply for all cases because when you use the same type like :

obsv$ = Observable.of(1)
  .catch( err => Observable.of(0) );

the type of obsv$ is Observable<number> but not because the catch returns that type, it's because the union number | number is equal to type number.

@ghetolay
Copy link
Contributor Author

ghetolay commented Mar 19, 2017

With this change then the output would always be a special union of two potentially different types.

@david-driscoll isn't it what exactly happens after a catch ? it's either the source observable or the one defined on catch, you can't know which one and thus can't know for sure the type of emitted item.

@david-driscoll
Copy link
Member

That is actually a fair point, I had forgotten about how that behavior would flow. I have been corrected 👍

Copy link
Member

@david-driscoll david-driscoll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After above comments these changes make sense.

@benlesh benlesh merged commit 840def0 into ReactiveX:master Apr 3, 2017
@ghetolay ghetolay deleted the catch_typings branch April 3, 2017 22:33
@lock
Copy link

lock bot commented Jun 6, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants