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

Deprecate retryWhen and repeatWhen #6859

Merged

Conversation

benlesh
Copy link
Member

@benlesh benlesh commented Mar 4, 2022

Deprecates retryWhen and repeatWhen, as the same can be accomplished in a MUCH simpler way with the delay argument to both retry and repeat. Not to be removed until v9 or v10. (A long-long time away, still).

Related to #6367.

@benlesh benlesh added 7.x Issues and PRs for version 6.x 8.x Issues and PRs for version 8.x labels Mar 4, 2022
@benlesh benlesh requested review from cartant and kwonoj March 4, 2022 15:53
@benlesh benlesh mentioned this pull request Mar 4, 2022
40 tasks
@benlesh benlesh merged commit e48e296 into ReactiveX:master Mar 7, 2022
@BenniG82
Copy link

Hi Folks, Hi @benlesh,

I think there may be cases where the current implementation of retryWhen is extremely useful.
See this sample:
https://stackblitz.com/edit/angular-ivy-2pooz5?file=src%2Fapp%2Fapp.component.ts

In this sample retryWhen only retries in cases where the server responds with HTTP-Status 504 (Gateway Timeout).

I racked my brain but I couln't find a solution for this use case without retryWhen.

Sorry if posting a comment to a PR is not desired but this looked like a good place to me :)

@benlesh
Copy link
Member Author

benlesh commented Mar 28, 2022

Hey, @BenniG82... the new retry configuration with delay makes this a lot easier. I've forked your example and simplified it the "new way" here:

https://stackblitz.com/edit/angular-ivy-hewest?file=src/app/app.component.ts

Also, a general "pro tip": Within a function like you'd use in retry, retryWhen, mergeMap, et al, you can just throw error instead of needing to return throwError(() => error). Same outcome, less code.

Basically, notice all I did was take whatever you had in retryWhen(errors => errors.pipe(switchMap(FUNCTION_HERE))) and pass it to retry({ delay: FUNCTION_HERE }).

@BenniG82
Copy link

Hi @benlesh,
thank you very much for pointing this out to me. A colleague of mine and I must be blind: We both looked at https://github.com/ReactiveX/rxjs/blob/master/src/internal/operators/retry.ts and didn't notice the union type (and the JS Doc) for delay in RetryConfig.
Now this deprecation makes sense to me :)

In case someone finds this conversation, this may be useful:
Deprecated retryWhen code:

  loadFromUrl(url: string): Observable<any> {
    return this.httpClient.get(url).pipe(
      retryWhen(this.retryNotifier)
    );
  }

  private retryNotifier(errorAttempt: Observable<any>): Observable<any> {
    return errorAttempt.pipe(
      switchMap((error, retryCount) => {
        console.log(error, retryCount);
        if (error.status === 504 && retryCount < 2) {
          return timer(1000).pipe(first());
        }
        throw error;
      })
    );
  }

Using retry:

  loadFromUrl(url: string): Observable<any> {
    return this.httpClient.get(url).pipe(
      retry({
        count: 2,
        delay: this.retryNotifier,
      })
    );
  }
  retryNotifier(error: any, retryCount: number) {
    console.log(error, retryCount);

    if (error.status === 504) {
      return timer(1000);
    }

    throw error;
  }

Also thank you for the throwError "pro tip" 👍

@m40l
Copy link

m40l commented Sep 14, 2022

Hi @benlesh ,

I'm in a similar situation as BenniG82 where I don't think repeat({delay => observer}) is an adequate replacement for repeatWhen(). I'm trying to use repeat() to repeat a fromFetch request, but I would like to throttle the repeats, like so

https://stackblitz.com/edit/rxjs-tqvnju?file=index.ts

// Original
fromFetch('www.google.com')
  .pipe(repeatWhen(() => fromEvent(document, 'click').pipe(throttleTime(5000))))
  .subscribe(() => console.log('repeatWhen'));

// Suggested non-deprecated alternative
fromFetch('www.google.com')
  .pipe(
    repeat({
      delay: () => fromEvent(document, 'click').pipe(throttleTime(5000)),
    })
  )
  .subscribe(() => console.log('repeat'));

The original works one as would expect, because repeatWhen only generates the observer once, but the new one does not throttle. I think the problem is that repeat resubscribes to the observer generated by delay() after every time fromFetch completes, which is resetting the throttle timer, so the clicks aren't throttled properly.

I think it would be possible to use repeat({ delay() }) if there was a way for the click Observable to globally throttle all its subscribers, but throwing those search terms into google didn't yield anything. I would really appreciate some insight as to how to migrate my code.

@m40l
Copy link

m40l commented Sep 14, 2022

Nevermind! Although it 's slightly unintuitive, I found a clean solution!

fromFetch('www.google.com')
  .pipe(
    repeat({
      delay: () => fromEvent(document, 'click').pipe(skipUntil(timer(5000)))),
    })
  )
  .subscribe(() => console.log('repeat'));

If you know the delay observer will be resubscribed each time, you can use skipUntil to ignore any requests to repeat the fetch for the first 5 seconds after each request.

@berse2212
Copy link

berse2212 commented Oct 6, 2022

Hello everyone, hello @benlesh,

I also have another suspect of retryWhen that I am unable to recreate using retry. I have created this stackblitz: https://stackblitz.com/edit/angular-ivy-nbdqee?file=src/app/retry-interceptor/retry-interceptor.spec.ts

(also see retry-interceptor.ts and retryWhen-interceptor.ts to see both possible variants I have implemented)

Long story short, I can't get the tests running using retry(), but easily with retryWhen(). I am unsure why, but on the one hand using retry sent-Events reset the tries-counter. On the other hand however using retryWhen only HttpResponses seem to reset the counter.

p.s. unfortunately I cannot run stackblitz at work, so I didn't try to get Jasmin to run, since I cannot test the setup anyways. It's probably best is to just copy the files and run it on a local machine to verify the behavior. Sorry for the inconvenience!

@jimbarrett33
Copy link

Replacing repeatWhen() with a "simple" delay doesn't cover a case where you want to repeat, or more specifically stop repeating, based on something other than time. For example, I have a custom operator that has an input/configuration where the caller can specify when to stop repeating a successful HTTP call based on data being returned from call (or when a retry limit is reached).

I may be missing something but overall I guess I just to don't get the deprecation of things that work, have been implemented and used by a lot of people for a long time, just to possibly save a few lines of code to make things more simple.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
7.x Issues and PRs for version 6.x 8.x Issues and PRs for version 8.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants