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

feat(retry): add config to reset error count on successful emission #4683

Closed
wants to merge 3 commits into from

Conversation

omermizr
Copy link

@omermizr omermizr commented Apr 3, 2019

This PR adds the ability to reset the error counter on successful emissions using the retry
operator.

Description:
The current behavior for retry(n) is to call error if n errors occurred, regardless of
whether or not they were consecutive. Now one would be able to use retry(n, true) to have the
count reset so that only n consecutive errors will cause the observable to fail.

@ci-reporter
Copy link

ci-reporter bot commented Apr 3, 2019

The build is failing

✨ Good work on this PR so far! ✨ Unfortunately, the Travis CI build is failing as of fb7bb58. Here's the output:

npm test
> @reactivex/[email protected] test /home/travis/build/ReactiveX/rxjs
> cross-env TS_NODE_PROJECT=spec/tsconfig.json mocha --opts spec/support/default.opts "spec/**/*-spec.ts"


/home/travis/build/ReactiveX/rxjs/node_modules/ts-node/src/index.ts:250
    return new TSError(diagnosticText, diagnosticCodes)
           ^
TSError: ⨯ Unable to compile TypeScript:
spec/Observable-spec.ts(21,10): error TS2345: Argument of type '() => PromiseConstructorLike' is not assignable to parameter of type '(this: IHookCallbackContext, done: MochaDone) => void | PromiseLike<any>'.
  Type 'PromiseConstructorLike' is not assignable to type 'void | PromiseLike<any>'.
    Type 'PromiseConstructorLike' is not assignable to type 'PromiseLike<any>'.
      Property 'then' is missing in type 'PromiseConstructorLike'.

    at createTSError (/home/travis/build/ReactiveX/rxjs/node_modules/ts-node/src/index.ts:250:12)
    at getOutput (/home/travis/build/ReactiveX/rxjs/node_modules/ts-node/src/index.ts:358:40)
    at Object.compile (/home/travis/build/ReactiveX/rxjs/node_modules/ts-node/src/index.ts:545:11)
    at Module.m._compile (/home/travis/build/ReactiveX/rxjs/node_modules/ts-node/src/index.ts:430:43)
    at Module._extensions..js (internal/modules/cjs/loader.js:712:10)
    at Object.require.extensions.(anonymous function) [as .ts] (/home/travis/build/ReactiveX/rxjs/node_modules/ts-node/src/index.ts:433:12)
    at Module.load (internal/modules/cjs/loader.js:600:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:539:12)
    at Function.Module._load (internal/modules/cjs/loader.js:531:3)
    at Module.require (internal/modules/cjs/loader.js:637:17)
    at require (internal/modules/cjs/helpers.js:22:18)
    at /home/travis/build/ReactiveX/rxjs/node_modules/mocha/lib/mocha.js:231:27
    at Array.forEach (<anonymous>)
    at Mocha.loadFiles (/home/travis/build/ReactiveX/rxjs/node_modules/mocha/lib/mocha.js:228:14)
    at Mocha.run (/home/travis/build/ReactiveX/rxjs/node_modules/mocha/lib/mocha.js:536:10)
    at Object.<anonymous> (/home/travis/build/ReactiveX/rxjs/node_modules/mocha/bin/_mocha:582:18)
    at Module._compile (internal/modules/cjs/loader.js:701:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:712:10)
    at Module.load (internal/modules/cjs/loader.js:600:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:539:12)
    at Function.Module._load (internal/modules/cjs/loader.js:531:3)
    at Function.Module.runMain (internal/modules/cjs/loader.js:754:12)
    at startup (internal/bootstrap/node.js:283:19)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:622:3)

I'm sure you can fix it! If you need help, don't hesitate to ask a maintainer of the project!


This comment was automagically generated by ci-reporter. If you see a problem, open an issue here.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 8312

  • 12 of 12 (100.0%) changed or added relevant lines in 1 file are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.2%) to 96.673%

Files with Coverage Reduction New Missed Lines %
src/internal/OuterSubscriber.ts 1 50.0%
Totals Coverage Status
Change from base Build 8309: -0.2%
Covered Lines: 5259
Relevant Lines: 5440

💛 - Coveralls

@benlesh benlesh added the AGENDA ITEM Flagged for discussion at core team meetings label Apr 4, 2019
@benlesh
Copy link
Member

benlesh commented May 8, 2019

@cartant said we can have this. 👍

@benlesh benlesh removed the AGENDA ITEM Flagged for discussion at core team meetings label May 8, 2019
@benlesh benlesh requested a review from cartant May 8, 2019 18:30
Copy link
Member

@benlesh benlesh left a comment

Choose a reason for hiding this comment

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

Okay... I think we're almost there. Here's what I think we should do though... instead of having two arguments, we have it take either a number or a config object like so:

retry(); // retry forever
retry(5); // retry five times
retry({ count: 5 }); // retry five times
retry({ count: 5, resetOnSuccess: true });  // retry five times, but reset the counter on success
retry{{ resetOnSuccess: true }); // This is the same thing as just retry() lol

@omermizr
Copy link
Author

Something to consider (breaking changes aside ): I think that resetOnSuccess should default to true.
I mainly use retry for polling mechanisms, and I suspect that's a pretty common usage. In that use case, I would most likely want to throw an error only if the stream fails consistently and consecutively (because that probably means there's some issue that isn't transient) - @benlesh WDYT?

ommizrah added 2 commits May 12, 2019 12:14
This PR adds the ability to reset the error counter on successful emissions using the `retry`
operator. The current behavior for `retry(n)` is to call error if n errors occurred, regardless of
whether or not they were consecutive. Now one would be able to use `retry(n, true)` to have the
count reset so that only n consecutive errors will cause the observable to fail.
added overloaded signature to the `retry` operator that accepts a config object
@omermizr
Copy link
Author

omermizr commented Jun 4, 2019

@benlesh ping, I made the suggested changes

Copy link
Member

@benlesh benlesh left a comment

Choose a reason for hiding this comment

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

Just a couple of small things.

package.json Show resolved Hide resolved
spec/Observable-spec.ts Show resolved Hide resolved
@omermizr omermizr requested a review from benlesh October 29, 2019 18:33
@omermizr
Copy link
Author

@benlesh ping

@cartant
Copy link
Collaborator

cartant commented Jan 30, 2020

Closed in favour of #5280 - into which your commits have been cherry picked, @omermizr.

@cartant cartant closed this Jan 30, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Feb 29, 2020
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.

4 participants