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

Improving catch, possibly removing retryWhen #141

Closed
benlesh opened this issue Jul 29, 2015 · 5 comments
Closed

Improving catch, possibly removing retryWhen #141

benlesh opened this issue Jul 29, 2015 · 5 comments

Comments

@benlesh
Copy link
Member

benlesh commented Jul 29, 2015

The proposal is a non-breaking change to catch:

EDIT: see comment below

@benlesh
Copy link
Member Author

benlesh commented Jul 29, 2015

cc/ @mattpodwysocki @staltz @trxcllnt for thoughts.

This was something I came up with while reimplementing the retryWhen operator today and discussing @trxcllnt's alternate version of that operator he called retryMany. I realized that both of these could be accomplished with catch if catch simply had a second argument of the source observable. @trxcllnt pointed out that it would have to be the "caught" observable, or really the whatever the last catch call returned, intialized with the source observable. Otherwise on the second catch, it would skip the catch operator all together.

@benlesh
Copy link
Member Author

benlesh commented Jul 29, 2015

@zenparsing and @jhusain may find this issue interesting for their es-observable efforts as well, since it's a basic operator.

@benlesh
Copy link
Member Author

benlesh commented Jul 29, 2015

So in working with this and talking with George Campbell at Netflix, it seems like the optional arguments should/could be:

myObservable.catch((err:any, source:Observable<T>, caught:Observable<any>) => Observable<any>) => Observable<any>

Where the arguments are as follows:

  • error: the error value
  • source: the original source observable
  • caught: the observable that is the result of the initial source.catch() call

With these arguments one can do just about any type of retry or catch handling. However, it's worth pointing out that any of this could be accomplished with closure as well. It's just that inside of the catch operator, we already have access to all of those observables, so we can easily pass them in without any closure.

I'm torn on this one:

Pros:

  • It's a non breaking change to catch
  • It adds very little complexity
  • It enables retryWhen functionality without an additional operator, and is perhaps easier to explain than retryWhen

Cons:

  • While it's probably easier to explain than retryWhen, it's still pretty hard to explain. Like "why return the 'caught' Observable?", etc
  • Could do this with closures already.
  • ???

I'm sure there are more cons... I'm waiting for others to weigh in.

@benlesh
Copy link
Member Author

benlesh commented Jul 29, 2015

To elaborate:

ordinary catch:

myObservable.catch(err => {
  if(isBadError(err)) {
    throw err;
  }
  return Observable.value('something happened, but here is a nice response');
});

retryWhen equivalent:

myObservable.catch((err, source, caught) => {
  if(totallyWannaRetry(err)) {
    return caught;
  } else {
    throw err;
  }
})

@benlesh
Copy link
Member Author

benlesh commented Jul 29, 2015

I'm not sure about the utility of the second argument being the source observable... perhaps it should be removed, or moved to be the 3rd argument?

@trxcllnt trxcllnt closed this as completed Aug 4, 2015
robwormald added a commit to robwormald/RxJS that referenced this issue Aug 31, 2015
…eneration

* 'master' of https://github.com/ReactiveX/RxJS:
  feat(catch): add catch operator, related to ReactiveX#141, closes ReactiveX#130
  Cleans up ReplaySubject naming. Uses splice to trim from events list.
  Make ReplaySubject check subscriber.isUnsubscribed while replaying events.
  feat (ReplaySubject): Add basic ReplaySubject implementation.
  Omit scheduling from source Observables to better compare versions.
  feat (defer): Add Observable.defer
  Updates macro perf URLs relative to new directory hierarchy.
  Adds micro-benchmark performance tests.
  Updates macro perf URLs relative to new directory hierarchy.
  feat (defer): Add Observable.defer
  Adding thisArg and bindCallback
  Adds micro-benchmark performance tests.
  Adds Subscriber instanceof check inside Observable's public subscribe.
  Resolve operator PR merge conflicts.
  Fix TS compiler errors related to SwitchLatestSubscriber
  Cleans up shared-subscriber routing logic in intermediate Subscribers.
  Ups the interval of the takeUntil test.
  feat (takeUntil): add takeUntil operator
@lock lock bot locked as resolved and limited conversation to collaborators Jun 8, 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

No branches or pull requests

2 participants